Skip to content
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

Validate RoleTemplate as non-namespaced #332

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

tomleb
Copy link
Contributor

@tomleb tomleb commented Feb 15, 2024

Issue

This should fix the integration tests in rancher/rancher issue that was introduced in #328 (see also rancher/rancher#40584) which results in the following tests to fail:

test_default_roles.py::test_cluster_create_role_locked
test_default_roles.py::test_project_create_default_role
test_default_roles.py::test_project_create_role_locked
test_default_roles.py::test_user_create_default_role

The error looks like the following:

"rancher.cattle.io.roletemplates.management.cattle.io" denied the request: roletemplate.rules[8].nonResourceURLs: Invalid value: []string{"*"}: namespaced rules cannot apply to non-resource URLs\n\t{\'baseType\': \'error\', \'code\': \'BadRequest\', \'message\': \'admission webhook "rancher.cattle.io.roletemplates.management.cattle.io" denied the request: roletemplate.rules[8].nonResourceURLs: Invalid value: []string{"*"}: namespaced rules cannot apply to non-resource URLs\', \'status\': 400, \'type\': \'error\'}')

The issue is that namespaced resources cannot have nonResourceURLs in them. The true here is passed down to the upstream k8s validation function, which sees the nonResourceURLs in the cluster-owner RoleTemplate.

The fix simply uses the rules from non-namespaced resources, which skips the check for nonResourceURLs. Note this is not perfectly 1:1 with k8s but it's still more checks than we had previously.

@andreas-kupries As mentioned in Slack, feel free to take this as inspiration for a fix, or review it as is. Leaving as draft for now.

Copy link
Contributor

@andreas-kupries andreas-kupries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over it, and reading the explanations, this looks like a good fix to me.
And apologies for missing this namespace vs not when writing the original PR.

Checked and confirmed for myself that RoleTemplates are not namespaced.
Cross-checking GlobalRoles are also not namespaced and their calls to ValidateRules are very much 🆗 , using false outright.

This confirms to me that the change to use false is the correct fix.
✔️ make test passes.

@andreas-kupries andreas-kupries marked this pull request as ready for review February 16, 2024 09:56
@andreas-kupries andreas-kupries requested a review from a team February 16, 2024 09:56
@tomleb tomleb merged commit 92ab3ea into rancher:release/v0.5 Feb 16, 2024
1 check passed
@tomleb tomleb deleted the fix-validation-rt branch February 16, 2024 14:52
andreas-kupries added a commit to andreas-kupries/rancher-webhook that referenced this pull request Mar 11, 2024
Proper validation of GlobalRole `rules`,
plus fix (TomL) to make validation not namespaced.
andreas-kupries added a commit that referenced this pull request Apr 3, 2024
…ion-backport

Backport of PRs #328 and #332, plus go module refresh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants