Chart: Admission controller name too long about ingress-nginx HOT 22 OPEN

/remove-kind bug
/kind support

• its a spec that is NOT defined by this controller
• its inherited from the K8S KEP
• its not a bug

from ingress-nginx.

There is also likely a previous issue that explains usage of the 63 chars and how many chars are available for the name

from ingress-nginx.

#7442 (comment)

from ingress-nginx.

The bug is not about the error the bug is that the trunc function is not working here for some reason it should have truncated the name but for some reason, it did not, probably there is some other logic somewhere that blocks it. I've confirmed that the truck function itself is working so it's not something in Helm itself

from ingress-nginx.

ok.

How did you install. Can you provide the exact command as used in real. Asking because I see this

% helm install ingress-nginx-2-01234567890123456789012345678901234567890123456789 ingress-nginx/ingress-nginx
--namespace ingress-nginx-2
--set controller.ingressClassResource.name=nginx-two
--set controller.ingressClass=nginx-two
--set controller.ingressClassResource.controllerValue="example.com/ingress-nginx-2"
--set controller.ingressClassResource.enabled=true
--set controller.ingressClassByName=true
Error: INSTALLATION FAILED: release name "ingress-nginx-2-01234567890123456789012345678901234567890123456789": invalid release name, must match regex ^a-z0-9?(.a-z0-9?)*$ and the length must not be longer than 53

from ingress-nginx.

Nothing too fancy
helm install my_very_long_release_name ingress-nginx/ingress-nginx -n ingress-nginx --create-namespace -f values.yaml

it actually being installed using argocd but the release name is long, it's composed of a ${tenant}-${clusterName}-${environment}

values.yaml

controller:
  ingressClassResource:
    name: my-nginx-class
  scope:
    namespaceSelector: "nginx=this"
  service:
    loadBalancerIP: SOME_IP
    externalTrafficPolicy: Local
  autoscaling:
    enabled: true
    minReplicas: 2

  topologySpreadConstraints:
    - maxSkew: 1
      topologyKey: topology.kubernetes.io/zone
      whenUnsatisfiable: DoNotSchedule
      labelSelector:
        matchLabels:
          app.kubernetes.io/name: ingress-nginx

  resources:
    requests:
      memory: 200Mi

from ingress-nginx.

Thank you for the info. It creates action item.

Can you try a helm install with as close a template match to the argo install please. Need to know the error message as I can not reproduce a Argo install to debug this.

from ingress-nginx.

This is the exact command that matches the ArgoCD install
Please keep in mind that argo is running helm template and applies the resulting objects to Kubernetes.
so it might have something to do with the helm template mechanism

from ingress-nginx.

How do I test and reproduce what you said about trunc function not working ?
I thought that seeing your helm command, with the exact string-literal, that Argo would render, for those name prefix vars, would help. But neither you provided that debug info nor I can guess the literals used in the hypothetical helm command.

I also tested a potentially similar helm command and sent you the command and output, by simulating a 60+ chars name prefix. But the error message there is not hinting at truncating name. The error message in my test is actually denying creation of a helm-release, which I think is expected and fair behavior.

Regardless of the tool being Argo or Helm or Flux or Kubectl or even the in-cluster-client, all of them will ultimately post some JSON payload to API-Server. So to get a action-item from this triaging, that shows that trunc function is not working, I had assumed, I could look at your hypothetical helm command, that actually uses the real and complete string-literal, for the prefix (and the output of that command).

from ingress-nginx.

As i said argo is doing helm template and applies the resulting object

helm template vary-long-company-name-production-us-west1-gke ingress-nginx/ingress-nginx -f values.yaml --debug -s templates/controller-service-webhook.yaml

you can see that the resulting name is longer than 63 chars

from ingress-nginx.

Thank you. That helps.

Now any chance you can do the helm install with this and copy/paste both the command you typed and also the output of the command. I think this request will come across to you as pointless. But to me this will concur with the test I did.

from ingress-nginx.

command:

helm install vary-long-company-name-production-us-west1-gke ingress-nginx/ingress-nginx -f /tmp/values.yaml

Output:

Error: 1 error occurred:
	* Service "vary-long-company-name-production-us-west1-gke-ingress-nginx-co-admission" is invalid: metadata.name: Invalid value: "vary-long-company-name-production-us-west1-gke-ingress-nginx-co-admission": must be no more than 63 characters

command:

helm template vary-long-company-name-production-us-west1-gke ingress-nginx/ingress-nginx -f /tmp/values.yaml  -s templates/controller-service-webhook.yaml

Output:

---
# Source: ingress-nginx/templates/controller-service-webhook.yaml
apiVersion: v1
kind: Service
metadata:
  labels:
    helm.sh/chart: ingress-nginx-4.10.1
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/instance: vary-long-company-name-production-us-west1-gke
    app.kubernetes.io/version: "1.10.1"
    app.kubernetes.io/part-of: ingress-nginx
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: controller
  name: vary-long-company-name-production-us-west1-gke-ingress-nginx-co-admission
  namespace: default
spec:
  type: ClusterIP
  ports:
    - name: https-webhook
      port: 443
      targetPort: webhook
      appProtocol: https
  selector:
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/instance: vary-long-company-name-production-us-west1-gke
    app.kubernetes.io/component: controller

from ingress-nginx.

thank you very much @OriBenHur-akeyless this helps.
This concurs with what is done is CI pipeline althought I have to check if there is a test for 63+ chars.

Also it looks like there is a bug in dealing with this use-case because the I am confused if truncating was attempted. As visible, the string "co" in the name generally renders as the full word "controller". Checking

@Gacko any comments. I think the truncating is conditional and we dont have a test-case where we check for this use-case.

@OriBenHur-akeyless its not my place to tell you what to do and your design seems reasonable by normal management standards. But what we have here is a commentable situation. I think that regardless of the truncating getting fixed in the template of the controller by the project, because any service name anywhere in any system needs to integrate with service discovery mechanisms, the vast populace has not faced this situation owing to using other ways to get uniqe self-explanatory names. So you could also consider shorter prefixes in your design. I think your design helps a ton as a label in monitoring and tracing obviously but its a tradeoff when it comes to RFCs.

Please wait for any other comments. I am waiting for @Gacko 's remarks as well. I will also check if the trunc of helm https://helm.sh/docs/chart_template_guide/function_list/#trunc has to honor some conditions, unlike the trunc of linux shell command

from ingress-nginx.

/triage accepted
/kind bug

from ingress-nginx.

/priority important-longterm

from ingress-nginx.

As per this suggestion: #7442 (comment)
Would you accept a PR truncating the name to 52 characters?

I'm happy to make that change if needed.

from ingress-nginx.

cc @tao12345666333 @rikatz
I am not the best person to comment on such a PR

from ingress-nginx.

No, please do not truncate to a hardcoded length. We added truncating for the admission webhook job here: #10523. I already noticed this is not happening for other names while working on this PR, so in the end it's not perfect, yes, but neither is truncating, as this can also lead to ambiguous names.

from ingress-nginx.

Does it make sense to add some sort of validation? If some names are longer than allowed, get Helm to bail out?

from ingress-nginx.

Hm, I wouldn't go this way and rather try to align the way we are doing it for the admission webhook jobs (including a template which truncates names) for other occurrences. I know, this isn't perfect as it could lead to ambiguous names, but still better than the current situation.

from ingress-nginx.

/assign

from ingress-nginx.

/retitle Chart: Admission controller name too long

from ingress-nginx.