nginx traffic routing double annotationPrefixes

[aniketphanse]

Checklist:

• [x] I've included steps to reproduce the bug.
• [x] I've inclued the version of argo rollouts.

Describe the bug

When setting additionalIngressAnnotations in nginx trafficRouting, argo-rollouts adds the annotationPrefix always even when the additional annotations have a prefix resulting in two prefixes, which is invalid.

Steps to reproduce the bug

• Create a rollouts config object with additionalIngressAnnotations as shown below:

apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
  ...
spec:
  strategy:
    canary:
      ...
      trafficRouting:
        nginx:
          additionalIngressAnnotations:
            additional.prefix/test-annotation: foo
            ingress.kubernetes.io/affinity: cookie
          annotationPrefix: ingress.kubernetes.io
          stableIngress: ci-dashboard

• Error displayed:

error patching canary ingress `ci-dashboard-ci-dashboard-canary`: Ingress.extensions "ci-dashboard-ci-dashboard-canary" is invalid: metadata.annotations: Invalid value: "ingress.kubernetes.io/additional.prefix/test-annotation": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')

• Try setting the annotationPrefix to null or ""
• The following error is displayed:

error patching canary ingress `ci-dashboard-ci-dashboard-canary`: Ingress.extensions "ci-dashboard-ci-dashboard-canary" is invalid: metadata.annotations: Invalid value: "nginx.ingress.kubernetes.io/additional.prefix/test-annotation": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')

Expected behavior

The ingress is created properly without the annotationPrefix being appended to the additionalIngressAnnotations that already have a prefix.

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    additional.prefix/test-annotation: foo
    ingress.kubernetes.io/affinity: cookie

Version

v1.1.0 but the issue persists on the latest version - https://github.com/argoproj/argo-rollouts/blob/master/rollout/trafficrouting/nginx/nginx.go#L121-L123

Logs

# Paste the logs from the rollout controller

time="2022-09-01T15:15:37Z" level=error msg="rollout syncHandler error: error patching canary ingress `ci-dashboard-ci-dashboard-canary`: Ingress.extensions \"ci-dashboard-ci-dashboard-canary\" is invalid: [..., metadata.annotations: Invalid value: \"nginx.ingress.kubernetes.io/additional.prefix/test-annotation\": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName'), metadata.annotations: Invalid value: \"nginx.ingress.kubernetes.io/ingress.kubernetes.io/affinity\": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName'), ...]" namespace=staging rollout=ci-dashboard

---

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

[zachaller]

Based on the documentation https://argoproj.github.io/argo-rollouts/features/traffic-management/nginx/ this seems to be the correct behavior. What are you trying to accomplish by adding an additional prefix that dose not match the ingress controller prefix?

[codejnki]

Hi @zachaller, we have other services which scrape metrics based on annotations on the ingress object. This interferes with the ability to add additional labels that have their own prefixes.

[zachaller]

Yea this is currently not supported and we would need to probably add another field something like additionalAnnotations and/or additionalLabels to maintain backwards compatibility.

[Vignesh-Au]

Thanks @zachaller!

Can you please let us know an ETA for your team to make this change?

Also, we would like to know if your team would be open to PRs from us for the change.

Thanks again!

[zachaller]

Thinking about another solution for this vs adding a new field the code could be modified such that if annotationPrefix is an empty string we let the additionalIngressAnnotations as is