-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: updated resource name length #11822
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
base: master
Are you sure you want to change the base?
Conversation
I don't think it's a good idea to just do dirty patches like this on the route spec. If it is ok to extend the limit values, then the same thing should be done for all resources. It's arguable that such a restriction might only be done due to some “inertia” on the part of the API creators to follow what they've done before, which might not really be necessary or justified. The length of the value is meaningless in terms of functionality, and may only affect the speed of etcd synchronization and the amount of memory used by APISIX when large strings are used. Perhaps you could start a discussion to expand the value on all resources. Or perhaps the limit could be moved to a configuration file and allow the user to define it at runtime without modifying the code. What do others think? |
I agree, on changing the route spec, but I found that this is a central space where all of this is gathered. This value is then picked up by multiple places inside the code. It's also important to separate the meaning of the route specification and the Kubernetes resources. My main problem is that although it's implied that the route spec character limit of 100 is also what you can give to your Kubernetes route, this is not correct in the end, leading to obscure infrastructural errors in production. I believe the ideal solution would be
However, I believe this will take much time and effort, and I don't know if that is in the scope of the rest of the team. |
LGTM |
Hi @csotiriou, are you still working on this PR? |
I would like to continue working on this. BUt what is the solution here after all? Should I try and make this value configurable in the schema.lua? I'm wondering if this is possible at all in this file or should I dive deeper in the code to see where it's called and take it from there. any advice on where to start? |
Personally, I prefer this approach: allowing users to redefine in the configuration file. After all, for most users, a 100-byte name is more than enough. |
Actually, that's the problem it isn't 100 bytes at all. It's less than half sometimes. 100 bytes is the technical limit, but internally the Ingress controller concatenates the namespace name, and service name to that, thereby exceeding 100 bytes with ease even if the user has given 30 characters as a name. And that is opaque to the user. The result is in production we will get errors without actually knowing the root cause. |
Hi @csotiriou, any updates? |
0208251
to
81797ac
Compare
I made the change and pushed it just now. If it was up to me I would push it to 512 just to be on the safe side - since the concatenation made by ingress controller means that you are getting 30% the space you think you do. But at this point I will take what I can get :) |
@csotiriou pls rebase the master branch, CI failed now |
865af65
to
e00b159
Compare
@membphis I believe it should be fine now, can you check? |
Hi @csotiriou, I see errors related to this PR in the failing tests, please check. |
@csotiriou any update? |
increased name limit to 256 to cope with Ingress Controller problems
e00b159
to
58158b9
Compare
@SkyeYoung I made the changes a few days ago, and I see that all tests have already passed. |
local rule_name_def = { | ||
type = "string", | ||
maxLength = 100, | ||
maxLength = 256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to PR:
I read the documentation and found that the API previously defined using this schema didn't specify any maximum field lengths.
I think this should be improved in the future, for example, by using OpenAPI documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR increases the maximum length limit for rule names from 100 to 256 characters to accommodate longer names generated by the Ingress Controller, which concatenates namespace names and other factors.
- Increases
maxLength
validation from 100 to 256 characters in the rule name definition - Updates corresponding test cases to validate the new length limit
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
apisix/schema_def.lua | Updates the rule name maximum length validation from 100 to 256 characters |
t/admin/services.t | Updates test cases to reflect the new maximum length limit validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Please evaluate this and see if it's really necessary. For example, does the concatenation of kubernetes resource names to form APISIX resource names still exist in ingress controller v2? If we still use the old way, are the changes in this PR necessary? This requires input from the APISIX ingress controller maintainers. |
The uniqueness of APISIX resources is guaranteed by the id, while the name is mainly for readability. Currently, Ingress Controller v2 still generates APISIX resource names by concatenating Kubernetes resource names. Therefore, increasing the length limit from 100 to 256 is reasonable, as it aligns with Kubernetes’ 253-character limit. |
Description
Fixes #11821
What it does.
It makes the route name accept a name length that is 3 times the default one. That is because in the Ingress Controller, the route name is actually automatically given by concatenating the namespace name + other factors, therefore.
Question: