Skupper cli flag handling for annotations

[c-kruse]

It can be necessary to use the --router-pod-annotations flags to add annotations for just the router pod that contain commas themselves.

For example if I wanted the annotation "foo=x,y,z" to be applied to my resources I would want skupper init to accept the --router-pod-annotations=foo=x,y,z flag. Due to the confusing combination of bash string handling, cli flag parsing in spf13/pflag and our implementation I have not sorted out how to get this to work.

EDIT: Weirdly enough, this works with the --annotations flag ?

[c-kruse]

@nluaces ever run into this before?

[nluaces]

@c-kruse good finding! I will take a look into it.

[nluaces]

It is indeed strange, the parsing of the values from the two flags is exactly the same:

https://github.com/skupperproject/skupper/blob/c26bce4079fffbd19a98656713428fc57e037452/cmd/skupper/skupper_kube_site.go#L49 https://github.com/skupperproject/skupper/blob/c26bce4079fffbd19a98656713428fc57e037452/cmd/skupper/skupper_kube_site.go#L53 https://github.com/skupperproject/skupper/blob/c26bce4079fffbd19a98656713428fc57e037452/cmd/skupper/skupper.go#L390

EDIT: which version are you using?

[c-kruse]

Away from a computer, I was also confused. 1.7.0 (I believe.) Was suspicious about a bit of code in the client that re-reads the site config

[c-kruse]

*not in client. Was talking about pkg/site.ReadSiteConfig. It appears to treat them a little differently.

Had not really followed that lead to see if it made any sense, though.

[lynnemorrison]

@c-kruse I'm trying to understand the annotations.    Does --router-pod-annotations supposed to follow format for kubernets annotations?   I don't see any commas allowed in description below. The commas seem to separate the key/value pairs so you can assign multiple values

From Annotations | Kubernetes

"Annotations are key/value pairs. Valid annotation keys have two segments: an optional prefix and name, separated by a slash (/). The name segment is required and must be 63 characters or less, beginning and ending with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_), dots (.), and alphanumerics between. The prefix is optional. If specified, the prefix must be a DNS subdomain: a series of DNS labels separated by dots (.), not longer than 253 characters in total, followed by a slash (/)."

I was just trying to look at how this is supposed to be formatted and tried a couple of different values:

skupper init --router-pod-annotations foo1=b,foo=a
kubectl get configmap skupper-site -o yaml
apiVersion: v1
data:
  router-pod-annotations: foo1=b,foo=a

I see the code replaces foo=b with foo=a

skupper init --router-pod-annotations foo=b,foo=a
 kubectl get configmap skupper-site -o yaml
apiVersion: v1
data:
  router-pod-annotations: foo=a

I see you can add an = at beginning of annotation or not

skupper init --router-pod-annotations=foo=a
kubectl get configmap skupper-site -o yaml
apiVersion: v1
data:
  router-pod-annotations: foo=a

I'm assuming you want:

skupper init --router-pod-annotations foo=a,b,c
 kubectl get configmap skupper-site -o yaml
apiVersion: v1
data:
  router-pod-annotations: foo=a,b,c

I tried a couple of other annotation fields, you can see below that the code uses the foo=a as a key/value pair then b as a key and c as a key. you can see empty values for key b and c.

skupper init --annotations foo=a,b,c --router-service-annotations test=b --controller-pod-annotation foo=x,y,z
kubectl get configmap skupper-site -o yaml
apiVersion: v1
data:
   controller-pod-annotations: foo=x,y=,z=
   router-service-annotations: test=b
kind: ConfigMap
metadata:
    annotations:
      b: ""
      c: ""
      foo: a

You can see if you don't provide an "=" when setting an annotation, it uses the value as in key

skupper init --annotations 1,2,3 --router-service-annotations b,c,d --controller-pod-annotation x,y,z
kubectl get configmap skupper-site -o yaml
  controller-pod-annotations: x=,y=,z=
  router-service-annotations: b=,c=,d=
 metadata:
  annotations:
    "1": ""
    "2": ""
    "3": ""

[c-kruse]

Hi @lynnemorrison. Thanks for looking in to this! Really appreciate the insight to look at the data in the skupper-site configmap! That helps me understand this a little better.

The specific context this came up in was with attempting to get skupper running in a specific configuration with Istio. We were looking to use this configuration option in particular to exclude some of the inter-router ports from the mesh's domain: https://istio.io/latest/docs/reference/config/annotations/#SidecarTrafficExcludeInboundPorts.

As for valid k8s annotations, you are correct that no commas are allowed in the annotation key by the spec you shared - but the seem to be rather common in annotation values.

There are 3+ bits of software in play here - I think I'm starting to understand the latter part of the chain thanks to your skupper-site cm insight.

1. the shell we are running the skupper commands from. Often shells strip quotes from arguments before it passes them to execv(...). This is how I experiment when I am not sure what a shell is going to do to argv: python -c "import sys; print(sys.argv[1:])" "foo=a,b,c" \"foo=a,b,c\"
2. The go library spf13/pflag we use under the hood to handle flags. This does some trickery with how it interprets string array arguments by treating the input as a csv for cases similar to ours.
3. The cli writes the router pod annotations to the skupper-site configmap - potentially in an ambiguous way?
4. the skupper site-controller reads the annotations from the configmap and creates the router pod with the annotations as it interprets them?

Putting those two together I can get us to this point:

skupper init --router-pod-annotations=\"foo=x,y,z\" --router-pod-annotations glim=glam
k get cm skupper-site -oyaml | grep annotations
  router-pod-annotations: foo=x,y,z,glim=glam
k get pods -l application=skupper-router -ojsonpath="{.items[].metadata.annotations}" | jq
{
  "foo": "x",
  "glim": "glam",
  "y": "",
  "z": ""
}

[lynnemorrison]

The solution that has been proposed is to add a configurable separator between annotations. If the user doesn't specify the configurable separator, it will be defaulted to a ","

skupper init --router-pod-annotations service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags=Environment=dev@skupper-separator@foo=x,y,z --annotation-separator @skupper-separator@

kubectl get deployment skupper-router -o yaml

template: metadata: annotations: foo: a,b,c prometheus.io/port: "9090" prometheus.io/scrape: "true" service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: Environment=dev

[lynnemorrison]

@ajssmith One thing @c-kruse found with the new implementation is that if the user specifies the flag multiple times we only accept the last value. I'm not sure if this is an issue or not.

This is how it worked in version 1.8.3

skupper init --router-pod-annotations a1=foo --router-pod-annotations a2=foo --> annotations: a1: foo a2: foo

New Code:

New code patched with StringVars instead of StringSliceVars - picks last invocation of the flag instead of accepting multiples

./skupper init --router-pod-annotations a1=foo --router-pod-annotations a2=foo --> annotations: a2: foo

But you can add multiple annotations using just one flag in the new code skupper init --router-pod-annotations a1=foo, a2=foo --> annotations: a1: foo a2: foo

or skupper init --router-pod-annotations a1=foo@@a2=foo --annotation-separator "@@" --> annotations: a1: foo a2: foo

[lynnemorrison]

There is also a second part of the failure of parsing the annotations that will be addressed in fixing this issue:

When parsing the user-provided router-service-annotations, Skupper first splits the field by commas into entries, then for each entry splits that by = into parts.

For each entry, it then takes the parts[0] and parts[1] as the annotation to apply.

This is the annotation trying to be added: service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags="Name=ISEP20240220-1637-related-lb-eks-us-east-1a-00"

There are 3 parts when split by = parts[0]: [service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags] parts[1]: "Name parts[2]: ISEP20240220-1637-related-lb-eks-us-east-1a-00"

Due to skupper's logic, it therefore thinks the annotation it should apply is: service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: "Name

when it should be: service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags=http://service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: "Name=ISEP20240220-1637-related-lb-eks-us-east-1a-00"

[marcelloale]

Hello, while working with skupper init, I noticed that the --ingress-annotations flag seems to be missing in the latest version (1.9.2).

I found this PR which appears to have removed the flag, possibly by accident (screenshot attached). In the documentation, the --ingress-annotations flag is still listed, but when I try to run:

skupper init --ingress ingress --ingress-annotations 'key1=value1,key2=value2'

I get the error:

Error: unknown flag: --ingress-annotations

Has this flag been deprecated or renamed? Is there an alternative way to set ingress annotations during initialization?

Thanks in advance!

[lynnemorrison]

@nluaces this does look like the cmd flag ingress-annotations was left off by mistake. I can address this issue next?

[nluaces]

@lynnemorrison sure!