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": ""
}