Skip to content

Enable OwnerReferencesPermissionEnforcement in e2e and improve test failure diagnostic#389

Merged
k8s-ci-robot merged 3 commits into
kubernetes-sigs:mainfrom
yongruilin:agentsb-e2e
Mar 12, 2026
Merged

Enable OwnerReferencesPermissionEnforcement in e2e and improve test failure diagnostic#389
k8s-ci-robot merged 3 commits into
kubernetes-sigs:mainfrom
yongruilin:agentsb-e2e

Conversation

@yongruilin

@yongruilin yongruilin commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

This PR improves e2e test coverage for RBAC correctness and makes test failures easier to diagnose.

Enable OwnerReferencesPermissionEnforcement admission plugin in e2e kind cluster

  • This admission plugin enforces that controllers must have finalizers subresource permission to set blockOwnerDeletion in ownerReferences
  • It is not enabled by default in Kubernetes but is commonly enabled in hardened clusters (e.g. OpenShift)
  • Without it, e2e tests miss RBAC issues like controllers/sandbox_controller: add RBAC for finalizers #377

Add missing finalizers RBAC for extensions controllers

  • Add sandboxclaims/finalizers and sandboxwarmpools/finalizers RBAC markers so the SandboxClaim and SandboxWarmPool controllers work with OwnerReferencesPermissionEnforcement enabled
  • Regenerate extensions-rbac.generated.yaml

Dump controller logs on e2e test failure

Fix e2e watch timeout propagation

  • Watch/MustWatch now accept a context.Context parameter so the timeout set by WaitForObject actually propagates to the watch loop
  • Cap WaitForObject and PollUntilObjectMatches at DefaultTimeout (60s) even when the parent context has a longer deadline
  • Previously, tests hung for 10 minutes until Go's test timeout panic killed the process, bypassing all cleanup (including log dumping). Now tests fail gracefully in ~60s with controller logs in the output.
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 10, 2026
@netlify

netlify Bot commented Mar 10, 2026

Copy link
Copy Markdown

Deploy Preview for agent-sandbox canceled.

Name Link
🔨 Latest commit 23144a9
🔍 Latest deploy log https://app.netlify.com/projects/agent-sandbox/deploys/69b1f83103c9be0009f97418
@yongruilin yongruilin changed the title Enable OwnerReferencesPermissionEnforcement in e2e kind cluster Mar 11, 2026
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 11, 2026
@yongruilin

Copy link
Copy Markdown
Contributor Author

Have expected error

        2026-03-11T00:19:51Z	ERROR	Error creating sandbox for claim	{"controller": "sandboxclaim", "controllerGroup": "extensions.agents.x-k8s.io", "controllerKind": "SandboxClaim", "SandboxClaim": {"name":"python-sandbox-claim","namespace":"python-sandbox-claim-test-1773188350549572278"}, "namespace": "python-sandbox-claim-test-1773188350549572278", "name": "python-sandbox-claim", "reconcileID": "dd7da7ad-b016-49e2-bdcc-df39a2011d60", "claimName": "python-sandbox-claim", "error": "sandbox create error: sandboxes.agents.x-k8s.io \"python-sandbox-claim\" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: , <nil>"}

/hold for #377

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2026
This admission plugin enforces that users must have finalizers
permission on the owner resource to set blockOwnerDeletion in
ownerReferences. Enabling it in e2e ensures RBAC issues like
the one fixed in kubernetes-sigs#377 are caught by tests.
@yongruilin

Copy link
Copy Markdown
Contributor Author

/hold cancel
#377 merged

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2026
@yongruilin yongruilin changed the title Enable OwnerReferencesPermissionEnforcement in e2e and dump controller logs on failure Mar 11, 2026
@yongruilin

Copy link
Copy Markdown
Contributor Author

/retest

@natasha41575 natasha41575 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

/lgtm

Comment thread test/e2e/framework/context.go Outdated
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2026
@natasha41575

Copy link
Copy Markdown

/assign @janetkuo

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2026
@natasha41575

Copy link
Copy Markdown

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2026
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2026
- Add dumpControllerLogs to afterEach: on test failure, fetch
  controller logs and write full logs to the artifacts directory
  (available in Prow) and print last 42 lines to test output.
- Fix Watch/MustWatch to accept context parameter so the timeout
  set by WaitForObject actually propagates to the watch loop.
- Cap WaitForObject and PollUntilObjectMatches at DefaultTimeout
  (60s) even when the parent context has a longer deadline, so
  tests fail gracefully and cleanup functions run.
- Fix WatchSet to use context.Background for the watchLoop so it
  outlives any individual caller's context and stays alive for
  the duration of the test.
Add sandboxclaims/finalizers and sandboxwarmpools/finalizers RBAC
markers so the SandboxClaim and SandboxWarmPool controllers can set
blockOwnerDeletion in ownerReferences when
OwnerReferencesPermissionEnforcement is enabled.

@janetkuo janetkuo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2026
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aditya-shantanu, janetkuo, yongruilin

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

The pull request process is described here

Details 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2026
@k8s-ci-robot k8s-ci-robot merged commit d75bf57 into kubernetes-sigs:main Mar 12, 2026
10 checks passed
chw120 pushed a commit to chw120/agent-sandbox that referenced this pull request Mar 13, 2026
…ailure diagnostic (kubernetes-sigs#389)

* Enable OwnerReferencesPermissionEnforcement in e2e kind cluster

This admission plugin enforces that users must have finalizers
permission on the owner resource to set blockOwnerDeletion in
ownerReferences. Enabling it in e2e ensures RBAC issues like
the one fixed in kubernetes-sigs#377 are caught by tests.

* Dump controller logs on e2e test failure and fix watch timeout

- Add dumpControllerLogs to afterEach: on test failure, fetch
  controller logs and write full logs to the artifacts directory
  (available in Prow) and print last 42 lines to test output.
- Fix Watch/MustWatch to accept context parameter so the timeout
  set by WaitForObject actually propagates to the watch loop.
- Cap WaitForObject and PollUntilObjectMatches at DefaultTimeout
  (60s) even when the parent context has a longer deadline, so
  tests fail gracefully and cleanup functions run.
- Fix WatchSet to use context.Background for the watchLoop so it
  outlives any individual caller's context and stays alive for
  the duration of the test.

* Add finalizers RBAC for extensions controllers

Add sandboxclaims/finalizers and sandboxwarmpools/finalizers RBAC
markers so the SandboxClaim and SandboxWarmPool controllers can set
blockOwnerDeletion in ownerReferences when
OwnerReferencesPermissionEnforcement is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

5 participants