-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chart: Admission controller name too long #11350
Comments
/remove-kind bug
|
There is also likely a previous issue that explains usage of the 63 chars and how many chars are available for the name |
The bug is not about the error the bug is that the |
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 |
Nothing too fancy it actually being installed using argocd but the release name is long, it's composed of a
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 |
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. |
This is the exact command that matches the ArgoCD install |
How do I test and reproduce what you said about trunc function not working ? 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). |
As i said argo is doing helm template and applies the resulting object
you can see that the resulting name is longer than 63 chars |
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. |
command: helm install vary-long-company-name-production-us-west1-gke ingress-nginx/ingress-nginx -f /tmp/values.yaml Output:
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:
|
thank you very much @OriBenHur-akeyless this helps. 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 |
/triage accepted |
/priority important-longterm |
As per this suggestion: #7442 (comment) I'm happy to make that change if needed. |
cc @tao12345666333 @rikatz |
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. |
Does it make sense to add some sort of validation? If some names are longer than allowed, get Helm to bail out? |
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. |
/assign |
/retitle Chart: Admission controller name too long |
What happened:
I'm getting this error while trying to deploy ingress-nginx
Service "<some_prfix>-<cluster_name>-ingress-nginx-controller-admission" is invalid: metadata.name: Invalid value: "<some_prfix>-<cluster_name>-ingress-nginx-controller-admission": must be no more than 63 characters
What you expected to happen:
The name should be truncated on the 63-char
seems like this part is not working for some reason
ingress-nginx/charts/ingress-nginx/templates/_helpers.tpl
Lines 99 to 101 in 51847ac
The text was updated successfully, but these errors were encountered: