nginx traffic routing double annotationPrefixes

aniketphanse commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

rarecrumb commented 1 year ago

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

That would be great, let's drop that annotationPrefix field altogether and just respect annotations plainly when added.

My issue surrounds certificate generation mainly because annotations need to be added to the ingress in order for cert-manager to generate the cert for the ingress.

See how these prefixes may not match up?

ingress:
  enabled: false
  className: nginx
  annotations:
    external-dns.alpha.kubernetes.io/cloudflare-proxied: "true"
    cert-manager.io/issuer: origin-issuer
    cert-manager.io/issuer-group: cert-manager.k8s.cloudflare.com
    cert-manager.io/issuer-kind: OriginIssuer
    nginx.ingress.kubernetes.io/force-ssl-redirect: "true"
    nginx.ingress.kubernetes.io/proxy-body-size: 10m
    nginx.ingress.kubernetes.io/proxy-connect-timeout: "30"
    nginx.ingress.kubernetes.io/proxy-read-timeout: "30"
    nginx.ingress.kubernetes.io/proxy-send-timeout: "30"

hieu29791 commented 1 year ago

My stable ingress has a lot of annotation, but the canary ingress which has been automatically created by Argo Rollout doesn't. I want to canary ingress will be exactly the same as stable ingress. I'm looking at the annotationPrefix and additionalIngressAnnotations, but it is not working. for example, here is my ingress annotation:

annotationPrefix: ""
additionalIngressAnnotations:
    external-dns.alpha.kubernetes.io/cloudflare-proxied: "true"
    nginx.ingress.kubernetes.io/proxy-body-size: "11m"

I want to set annotationPrefix as empty then the ingress annotation will remain the same as it is defined. But seems like when annotationPrefix is empty, it keeps the default nginx.ingress.kubernetes.io value, any ideas?

cyrilico commented 3 months ago

@zachaller Any sort of agreed upon approach for this? Personally, I feel like still being able to customize a default annotationPrefix for annotations that are configured without one (useful if multiple need to be passed from the same "group"), but respecting any annotations already present in entries from additionalIngressAnnotations (vs injecting annotationPrefix blindly when it's not detected) would be an interesting way to go, and it shouldn't break any current specs, since if we configure annotations with different prefixes, the rollouts controller will inject another one on top and k8s will complain anyway.

Wdyt? Willing to try to tackle this perhaps if the approach sounds good

argoproj / argo-rollouts

nginx traffic routing double annotationPrefixes #2225