Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
✨ Add classNamespace to topology #11352
base: main
Are you sure you want to change the base?
✨ Add classNamespace to topology #11352
Changes from all commits
1dea02a
ccc32d1
65db336
5d9542b
b4b6131
e16aa76
3639364
1a88697
f8e49a0
902e582
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please add a minimum and maximum length to all raw string types, since this is a namespace, it follows DNS1123Subdomain and should also be validated as such (there's a regex for this). The maximum length is 253 and the minimum is 1.
Since you have
omitempty
, you can safely add theMinLength
, it will prevent anyone using an unstructured client from persistingclassNamespace: ""
which would then otherwise not round trip through a structured requestSome generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
besides the on admission check which is nice, do we have any rbac recommendation for this?
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.
VAP supports secondary authz for added RBAC control, and VAP binding can be used with label selectors, which addresses #5673 (comment). From the proposal itself, it seems using the policy for added restriction on top of RBAC is within the scope.
Webhook allows to use paramRef of any kind, which can be potentially explored with specific CRD to further restrict access beyond admission, with a controller acting on that resource.
Currently, this is just an example of how an on top policy can be defined (if needed) in k8s 1.30+, where a user may decide to use different policy mechanisms to further restrict access, including a more granular RBAC. I’m thinking to showcase it as an option, but to not enforce any specific solution within this PR.
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.
There is also an RBAC recommendation in https://kccncna2024.sched.com/event/1hoyX, if a talk considered to be one.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Wondering if we should update the index, so it contains both namespace and name of the cluster class
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.
Thinking a little bit more about this, I think we change this index so performance in the webhook are optimized and also the resulting code will be more readable (both here and in clusterClassToCluster)
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.
It seems to me that in this method there is not enough information to know cluster namespace due to duality of the namespace reference.
If an index is added, there will be another
Client.List
call forclient.MatchingFields{index.ClusterClassNamespaceField: clusterClass.Namespace}
, and here I’m not sure how would an empty field be treated - matching an empty string or matching no selector at all?If the first one the case, then the method will combine two list responses, and it will be more readable. If not, it will only increase the number of calls.
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.
Ok, seems it is not an issue, as the index can use
GetClass()
method which resolves ambiguity. Should be added now.