test: add RBAC e2e tests for create and delete endpoints#1066
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sradco The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change maps Kubernetes authentication and authorization errors to HTTP 401/403 responses, forces scoped clients to use bearer tokens, adds short-lived E2E identities, and introduces alert-rule RBAC tests for create and bulk-delete operations. ChangesRBAC authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RBACTests
participant E2EFramework
participant KubernetesAPI
participant ManagementRouter
RBACTests->>E2EFramework: Create scoped ServiceAccount identity
E2EFramework->>KubernetesAPI: Create RBAC resources and request token
KubernetesAPI-->>E2EFramework: Return short-lived bearer token
RBACTests->>ManagementRouter: Send alert-rule request with token
ManagementRouter->>KubernetesAPI: Authorize alert-rule operation
KubernetesAPI-->>ManagementRouter: Allow or return Unauthorized/Forbidden
ManagementRouter-->>RBACTests: Return operation status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/e2e/rbac_test.go (2)
201-202: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed sleep for cache sync is flaky.
time.Sleep(5 * time.Second)is a fixed guess at how long the "relabeled rules cache" takes to sync. Under load this could be flaky (test proceeds before sync completes) or unnecessarily slow. Consider polling with a bounded retry/timeout (e.g., repeatedly attempt the first delete and back off) instead of a hardcoded sleep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/rbac_test.go` around lines 201 - 202, Replace the fixed time.Sleep in the relabeled rules cache synchronization flow with bounded polling and retry backoff, using the first delete attempt as the readiness check. Stop when the delete succeeds or the retry timeout is reached, while preserving the test’s existing failure behavior after exhaustion.
19-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider table-driven structure for repeated user/namespace test cases.
Both
TestRBAC_CreateAlertRuleandTestRBAC_DeleteAlertRulerepeat near-identicalt.Runblocks per user (A/B/C) that only differ in token, namespace/rule, and expected status. As per coding guidelines,**/*_test.goshould "use table-driven tests when feasible." A slice of{name, token, target, wantStatus}cases iterated witht.Runwould reduce duplication and make it easier to add new RBAC scenarios.Also applies to: 107-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/rbac_test.go` around lines 19 - 105, Refactor TestRBAC_CreateAlertRule and TestRBAC_DeleteAlertRule to use table-driven cases for the repeated user/namespace scenarios. Define each case with its name, token, target namespace/rule, and expected HTTP status, then iterate with t.Run while preserving the existing assertions and scenario coverage.Source: Coding guidelines
test/e2e/framework/framework.go (1)
261-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated TokenRequest construction.
The
expSeconds := int64(3600)/TokenRequestconstruction is duplicated verbatim betweenCreateScopedUserandCreateUnprivilegedUser. Consider extracting a small shared helper (e.g.f.createServiceAccountToken(ctx, namespace, name)) to avoid drift between the two.Also applies to: 289-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/framework/framework.go` around lines 261 - 264, Extract the duplicated expSeconds and authv1.TokenRequest construction from CreateScopedUser and CreateUnprivilegedUser into a shared helper such as createServiceAccountToken, accepting the context, namespace, and service-account name. Update both callers to use the helper while preserving their existing token request behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/framework/framework.go`:
- Around line 224-278: Update CreateScopedUser and CreateUnprivilegedUser to
roll back resources created before any subsequent API call fails. Reuse or
invoke cleanup logic for the ServiceAccount, Role, and RoleBinding as applicable
before returning errors from Role, RoleBinding, or CreateToken operations,
including the SA-only rollback in CreateUnprivilegedUser. Preserve existing
error wrapping and successful provisioning behavior.
In `@test/e2e/rbac_test.go`:
- Line 34: Handle all flagged deferred cleanup calls in the affected test
functions and helpers, including cleanupY, cleanupZ, userA.Cleanup,
userB.Cleanup, and resp.Body.Close. Explicitly discard or log each returned
error using the project’s established pattern so errcheck passes, while
preserving the existing defer behavior.
---
Nitpick comments:
In `@test/e2e/framework/framework.go`:
- Around line 261-264: Extract the duplicated expSeconds and authv1.TokenRequest
construction from CreateScopedUser and CreateUnprivilegedUser into a shared
helper such as createServiceAccountToken, accepting the context, namespace, and
service-account name. Update both callers to use the helper while preserving
their existing token request behavior.
In `@test/e2e/rbac_test.go`:
- Around line 201-202: Replace the fixed time.Sleep in the relabeled rules cache
synchronization flow with bounded polling and retry backoff, using the first
delete attempt as the readiness check. Stop when the delete succeeds or the
retry timeout is reached, while preserving the test’s existing failure behavior
after exhaustion.
- Around line 19-105: Refactor TestRBAC_CreateAlertRule and
TestRBAC_DeleteAlertRule to use table-driven cases for the repeated
user/namespace scenarios. Define each case with its name, token, target
namespace/rule, and expected HTTP status, then iterate with t.Run while
preserving the existing assertions and scenario coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4023d351-5886-44ad-8253-460e6697a620
📒 Files selected for processing (4)
internal/managementrouter/router.gointernal/managementrouter/router_test.gotest/e2e/framework/framework.gotest/e2e/rbac_test.go
7f804d6 to
b35d3f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 61-63: Update the Makefile target declarations so unit-test
depends on test-backend, while test-backend alone owns the go test recipe. Keep
both targets phony and ensure invoking either target does not duplicate backend
test execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5c1aba6a-9392-41c4-a5b6-b0fe1d508daa
📒 Files selected for processing (8)
Makefileinternal/managementrouter/router.gointernal/managementrouter/router_test.gotest/e2e/create_alert_rule_test.gotest/e2e/delete_alert_rule_test.gotest/e2e/framework/framework.gotest/e2e/helpers_test.gotest/e2e/rbac_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/managementrouter/router_test.go
- test/e2e/framework/framework.go
- internal/managementrouter/router.go
- test/e2e/rbac_test.go
|
/test images |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/k8s/user_scoped_client_test.go`:
- Around line 23-40: Extend the test around newUserScopedClientsets to inspect
the derived rest.Config, not just confirm base remains unchanged. Extract the
config-building logic or inject a client factory, then assert the derived config
uses “user-token” and clears CertData, KeyData, CertFile, and KeyFile while
preserving the existing base-config immutability checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a89992f7-5fd2-4755-b411-fe27a7ed01e9
📒 Files selected for processing (10)
Makefileinternal/managementrouter/router.gointernal/managementrouter/router_test.gopkg/k8s/user_scoped_client.gopkg/k8s/user_scoped_client_test.gotest/e2e/create_alert_rule_test.gotest/e2e/delete_alert_rule_test.gotest/e2e/framework/framework.gotest/e2e/helpers_test.gotest/e2e/rbac_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/managementrouter/router_test.go
- test/e2e/helpers_test.go
- test/e2e/delete_alert_rule_test.go
- internal/managementrouter/router.go
- test/e2e/framework/framework.go
- test/e2e/create_alert_rule_test.go
- test/e2e/rbac_test.go
78f6450 to
70b220b
Compare
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
7cad5e7 to
bf55e26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/k8s/user_scoped_client_test.go (1)
9-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood fix — now asserts on the derived config directly.
This addresses the prior feedback: it now verifies
cfg.BearerToken,TLSClientConfigclearing, andInsecure/Hostpreservation on the derived config, in addition to base-config immutability.One gap:
buildUserScopedConfig(context snippet) only clearsCertData/CertFile/KeyData/KeyFile, leavingCAData/CAFileuntouched — worth asserting those are preserved too, since a future regression that also strips CA verification data would silently break TLS trust to the API server and this test wouldn't catch it.♻️ Suggested additional assertions
if !cfg.TLSClientConfig.Insecure { t.Error("derived Insecure should be preserved as true") } + if !bytes.Equal(cfg.TLSClientConfig.CAData, base.TLSClientConfig.CAData) { + t.Error("derived CAData should be preserved for server verification") + } if cfg.Host != base.Host { t.Errorf("derived Host = %q, want %q", cfg.Host, base.Host) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/k8s/user_scoped_client_test.go` around lines 9 - 64, Extend TestBuildUserScopedConfig to assert that the derived cfg preserves the base TLSClientConfig CAData and CAFile values. Initialize those fields in the base configuration with representative values, then verify cfg.TLSClientConfig.CAData and CAFile remain unchanged while the client certificate and key fields are cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/k8s/user_scoped_client.go`:
- Around line 20-28: Update buildUserScopedConfig to create the copied
configuration through rest.AnonymousClientConfig(baseConfig) before assigning
BearerToken, ensuring username/password, auth and exec providers, impersonation,
and custom transports or wrappers are removed while preserving the existing TLS
credential clearing.
In `@test/e2e/rbac_test.go`:
- Line 248: Wrap the deferred resp.Body.Close() calls in tryDeleteAlertRule and
createAlertRuleWithToken with the repository’s accepted error-handling pattern
so errcheck passes at both sites.
---
Nitpick comments:
In `@pkg/k8s/user_scoped_client_test.go`:
- Around line 9-64: Extend TestBuildUserScopedConfig to assert that the derived
cfg preserves the base TLSClientConfig CAData and CAFile values. Initialize
those fields in the base configuration with representative values, then verify
cfg.TLSClientConfig.CAData and CAFile remain unchanged while the client
certificate and key fields are cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 66dfa17a-3fc6-49cd-9957-860325add327
📒 Files selected for processing (10)
Makefileinternal/managementrouter/router.gointernal/managementrouter/router_test.gopkg/k8s/user_scoped_client.gopkg/k8s/user_scoped_client_test.gotest/e2e/create_alert_rule_test.gotest/e2e/delete_alert_rule_test.gotest/e2e/framework/framework.gotest/e2e/helpers_test.gotest/e2e/rbac_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- test/e2e/helpers_test.go
- internal/managementrouter/router_test.go
- internal/managementrouter/router.go
- test/e2e/delete_alert_rule_test.go
- test/e2e/create_alert_rule_test.go
- Makefile
- test/e2e/framework/framework.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/k8s/user_scoped_client.go (1)
21-27: 🔒 Security & Privacy | 🟠 MajorUse
rest.AnonymousClientConfigto make this bearer-token-only.
rest.CopyConfigpreserves inheritedUsername/Password,AuthProvider,ExecProvider, impersonation, and custom transports/wrappers. Clearing only certificate fields does not prevent the client from authenticating as the base identity, so these scoped clients can still bypass the user’s RBAC. Start fromrest.AnonymousClientConfig(baseConfig)before assigningBearerToken.This repeats an unresolved finding from the previous review.
#!/bin/bash set -eu CLIENT_GO_DIR="$(go list -m -f '{{.Dir}}' k8s.io/client-go)" rg -n -C5 \ 'func AnonymousClientConfig|func CopyConfig|AuthProvider|ExecProvider|WrapTransport|Impersonate' \ "$CLIENT_GO_DIR/rest"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/k8s/user_scoped_client.go` around lines 21 - 27, Update the config initialization in the user-scoped client constructor to use rest.AnonymousClientConfig(baseConfig) instead of rest.CopyConfig(baseConfig), then assign the user BearerToken while retaining the existing certificate-field clearing. Ensure inherited credentials, providers, impersonation, and transport wrappers are excluded so authentication is bearer-token-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/k8s/user_scoped_client.go`:
- Around line 21-27: Update the config initialization in the user-scoped client
constructor to use rest.AnonymousClientConfig(baseConfig) instead of
rest.CopyConfig(baseConfig), then assign the user BearerToken while retaining
the existing certificate-field clearing. Ensure inherited credentials,
providers, impersonation, and transport wrappers are excluded so authentication
is bearer-token-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 09ce9ecb-37a7-44e8-9436-e3671321ee9e
📒 Files selected for processing (10)
Makefileinternal/managementrouter/router.gointernal/managementrouter/router_test.gopkg/k8s/user_scoped_client.gopkg/k8s/user_scoped_client_test.gotest/e2e/create_alert_rule_test.gotest/e2e/delete_alert_rule_test.gotest/e2e/framework/framework.gotest/e2e/helpers_test.gotest/e2e/rbac_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/managementrouter/router_test.go
- test/e2e/helpers_test.go
- pkg/k8s/user_scoped_client_test.go
- internal/managementrouter/router.go
- test/e2e/create_alert_rule_test.go
- test/e2e/framework/framework.go
- test/e2e/delete_alert_rule_test.go
- test/e2e/rbac_test.go
9497b5d to
9fe95bf
Compare
|
/retest |
|
/test e2e-monitoring |
3 similar comments
|
/test e2e-monitoring |
|
/test e2e-monitoring |
|
/test e2e-monitoring |
|
/override ci/prow/e2e-monitoring |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/e2e-monitoring DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| return &ScopedUser{Token: token, Cleanup: func() error { rollback(); return nil }}, nil | ||
| } | ||
|
|
||
| // CreateUnprivilegedUser creates a ServiceAccount with no RBAC permissions. |
There was a problem hiding this comment.
(nit) anonymous rather than unprivileged?
| } | ||
| defer func() { _ = userA.Cleanup() }() | ||
|
|
||
| userB, err := f.CreateScopedUser(ctx, "e2e-rbac-del-b", nsY, |
There was a problem hiding this comment.
| userB, err := f.CreateScopedUser(ctx, "e2e-rbac-del-b", nsY, | |
| userScopedToNamespaceY, err := f.CreateScopedUser(ctx, "e2e-rbac-del-b", nsY, |
openshift-ci
Bot
added
the
needs-rebase
delete endpoints Add e2e tests verifying that the management API enforces Kubernetes RBAC for create and delete alert rule operations. Three user profiles are tested: unprivileged (expects 403), namespace-scoped (succeeds in own namespace, denied elsewhere), and cluster-admin (succeeds everywhere). Also fixes a critical bug in newUserScopedClientsets: when the base rest.Config uses client certificates (common in CI kubeconfigs), CopyConfig preserved them. Since Kubernetes authenticates via client certs when both certs and bearer token are present, user RBAC was bypassed entirely. Use AnonymousClientConfig to strip all auth so the API server authenticates exclusively via the user's bearer token. Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: AI Assistant <noreply@cursor.com>
openshift-ci
Bot
removed
the
needs-rebase
|
/override-sticky ci/prow/e2e-monitoring |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/e2e-monitoring These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@sradco: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
CreateScopedUser,CreateUnprivilegedUser) for creating ServiceAccounts with specific Role/RoleBinding permissionsparseError(previously returned generic 500)newUserScopedClientsets: clears client certificate fields from the copied rest.Config so the API server authenticates exclusively via the user's bearer token (client certs take precedence over bearer tokens in Kubernetes, which bypassed RBAC entirely when the base config used cert auth)parseErrorbranches and the client cert clearing behaviorTest plan
go test ./internal/managementrouter/...passes (unit tests for parseError)go test ./pkg/k8s/...passes (unit test for client cert clearing)go test -tags e2e ./test/e2e/...passes in CI with real cluster (RBAC enforcement verified)go test ./...locally skips e2e entirely (no env vars needed)Signed-off-by: Shirly Radco sradco@redhat.com
Summary by CodeRabbit
Bug Fixes
authentication failed).insufficient permissions) without leaking API details.Tests
Chores
e2ebuild tag and updated the E2E test command accordingly.