Skip to content

fix: split EPP RBAC into cluster and namespaced scoped permission #1071

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chewong
Copy link
Member

@chewong chewong commented Jun 26, 2025

Fixes #224

Testing:

helm upgrade -i epp ./config/charts/inferencepool

# in a curl pod
curl 10.0.227.159/v1/models
{"object":"list","data":[{"id":"...
Split EPP RBAC into cluster and namespaced scoped permission

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2025
Copy link

netlify bot commented Jun 26, 2025

Deploy Preview for gateway-api-inference-extension ready!

Name Link
🔨 Latest commit b77dcea
🔍 Latest deploy log https://app.netlify.com/projects/gateway-api-inference-extension/deploys/6876aa9b8168f300088d651c
😎 Deploy Preview https://deploy-preview-1071--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@nirrozenbaum
Copy link
Contributor

thanks for the PR 👍 .

can you update also here and here?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2025
@danehans
Copy link
Contributor

I think the test utils/suite need to be updated too:

$ grep -ri ClusterRole .
./config/charts/inferencepool/templates/rbac.yaml:kind: ClusterRole
./config/charts/inferencepool/templates/rbac.yaml:kind: ClusterRoleBinding
./config/charts/inferencepool/templates/rbac.yaml:  kind: ClusterRole
./config/manifests/inferencepool-resources.yaml:kind: ClusterRole
./config/manifests/inferencepool-resources.yaml:kind: ClusterRoleBinding
./config/manifests/inferencepool-resources.yaml:  kind: ClusterRole
./test/testdata/metrics-rbac.yaml:kind: ClusterRole
./test/testdata/metrics-rbac.yaml:kind: ClusterRoleBinding
./test/testdata/metrics-rbac.yaml:  kind: ClusterRole
./test/testdata/inferencepool-e2e.yaml:kind: ClusterRole
./test/testdata/inferencepool-e2e.yaml:kind: ClusterRoleBinding
./test/testdata/inferencepool-e2e.yaml:  kind: ClusterRole
./test/e2e/epp/e2e_suite_test.go:	// Wait for the clusterrole to exist.
./test/e2e/epp/e2e_suite_test.go:		return k8sClient.Get(ctx, types.NamespacedName{Name: "pod-read"}, &rbacv1.ClusterRole{})
./test/e2e/epp/e2e_suite_test.go:	// Wait for the clusterrolebinding to exist.
./test/e2e/epp/e2e_suite_test.go:		return k8sClient.Get(ctx, types.NamespacedName{Name: "pod-read-binding"}, &rbacv1.ClusterRoleBinding{})
./test/utils/utils.go:	binding := &rbacv1.ClusterRoleBinding{
./test/utils/utils.go:	role := &rbacv1.ClusterRole{
./test/utils/utils.go:	metricsReaderBinding := &rbacv1.ClusterRoleBinding{
./test/utils/utils.go:	metricsReaderRole := &rbacv1.ClusterRole{
./Makefile:manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
...
./site-src/guides/metrics-and-observability.md:To scrape metrics, the client needs a ClusterRole with the following rule:
./site-src/guides/metrics-and-observability.md:kind: ClusterRole
./site-src/guides/metrics-and-observability.md:kind: ClusterRoleBinding
./site-src/guides/metrics-and-observability.md:  kind: ClusterRole
...

@danehans
Copy link
Contributor

This will be a breaking change for implementations. @nirrozenbaum @kfswain @ahg-g we need a way to ensure breaking changes are highlighted when a release is cut.

@danehans
Copy link
Contributor

nit: Update comment from "ClusterRole" to "Role" or "RBAC":

./Makefile:manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 27, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chewong
Once this PR has been reviewed and has the lgtm label, please ask for approval from danehans. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@danehans
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 27, 2025
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@nirrozenbaum
Copy link
Contributor

nirrozenbaum commented Jun 29, 2025

This will be a breaking change for implementations. @nirrozenbaum @kfswain @ahg-g we need a way to ensure breaking changes are highlighted when a release is cut.

how about adding a label (gie-area/breaking-change) and label the PR. then add instructions to whoever is doing the release to go over the list of PRs and highlight what's needed?

@kfswain
Copy link
Collaborator

kfswain commented Jun 30, 2025

This will be a breaking change for implementations. @nirrozenbaum @kfswain @ahg-g we need a way to ensure breaking changes are highlighted when a release is cut.

how about adding a label (gie-area/breaking-change) and label the PR. then add instructions to whoever is doing the release to go over the list of PRs and highlight what's needed?

We could also tag breaking PRs in a comment on the release tracking issue/milestone

@chewong
Copy link
Member Author

chewong commented Jul 1, 2025

Raised a PR in test-infra to create a new label called kind/breaking-change - kubernetes/test-infra#35069

@nirrozenbaum nirrozenbaum added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jul 7, 2025
@ahg-g
Copy link
Contributor

ahg-g commented Jul 8, 2025

anything pending on merging this PR?

@chewong
Copy link
Member Author

chewong commented Jul 8, 2025

Added release note in the PR description. Nothing pending on merging this PR.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EPP: Use Dedicated Service Account
6 participants