Allow sandboxclaim's metadata to be passed to the sandbox #173

peterzhongyi · 2025-11-20T18:00:19Z

This allows a user to create a sandbox with customized labels.

k8s-ci-robot · 2025-11-20T18:00:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: peterzhongyi
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. 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:

OWNERS

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 · 2025-11-20T18:00:30Z

Hi @peterzhongyi. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2025-11-20T18:00:35Z

✅ Deploy Preview for agent-sandbox canceled.

Name	Link
🔨 Latest commit	`75d1a71`
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/6920a9e6c1304e00084a52fe

vicentefb · 2025-11-20T18:55:55Z

extensions/controllers/sandboxclaim_controller.go

+			Labels:      claim.Labels,
+			Annotations: claim.Annotations,
 		},
 	}


lgtm, not blocking but do you mind updating the sandbox_claim.yaml https://github.com/kubernetes-sigs/agent-sandbox/blob/main/extensions/examples/sandboxclaim.yaml to show that we can pass labels/annotations through the claim as well , thanks!

janetkuo · 2025-11-20T19:01:24Z

/ok-to-test

barney-s · 2025-11-21T22:38:07Z

extensions/controllers/sandboxclaim_controller.go

-			Name:      claim.Name,
+			Namespace:   claim.Namespace,
+			Name:        claim.Name,
+			Labels:      claim.Labels,


Is the goal to be passed on the pod as well ?
if so this is not sufficient.

We need to be careful about modifying pod labels, see #174 (comment)

janetkuo · 2025-11-21T23:23:23Z

extensions/controllers/sandboxclaim_controller.go

+			Namespace:   claim.Namespace,
+			Name:        claim.Name,
+			Labels:      claim.Labels,
+			Annotations: claim.Annotations,


Filter internal annotations instead of blindly copying everything that might be misleading. For example, filtering out the "last applied" annotation.

Also, this only copies the map reference. The best practice is to deep copy maps, ref

agent-sandbox/controllers/sandbox_controller.go

Lines 393 to 399 in 2e61902

for k, v := range sandbox.Spec.PodTemplate.ObjectMeta.Labels {

labels[k] = v

}

annotations := map[string]string{}

for k, v := range sandbox.Spec.PodTemplate.ObjectMeta.Annotations {

annotations[k] = v

}

barney-s

This PR introduces a valuable feature by propagating metadata from the SandboxClaim to the Sandbox. However, there are a few critical issues that need to be addressed before this can be merged.

Shallow Copy: The labels and annotations are being copied by reference, not by value. This can lead to concurrent map access issues or unintended modifications. A deep copy is required.
Annotation Filtering: The implementation copies all annotations, including internal ones that should not be propagated (e.g., kubectl.kubernetes.io/last-applied-configuration). A filtering mechanism is needed.
Testing: While a unit test has been added, it would be beneficial to expand it to cover the filtering logic and also add an e2e test to validate the end-to-end flow.

Existing issue:

Lack of an OwnerReference on the created Sandbox

barney-s · 2025-12-02T23:33:50Z

extensions/controllers/sandboxclaim_controller.go

-			Name:      claim.Name,
+			Namespace:   claim.Namespace,
+			Name:        claim.Name,
+			Labels:      claim.Labels,


This is a shallow copy, not a deep copy. Assigning claim.Labels directly makes sandbox.ObjectMeta.Labels a reference to the same map. Any modification to the claim's labels by another controller could unintentionally alter the sandbox's labels, leading to unpredictable behavior.

A new map should be created and the key-value pairs copied over. For example:

newLabels := make(map[string]string) for k, v := range claim.Labels { newLabels[k] = v } sandbox.ObjectMeta.Labels = newLabels

barney-s · 2025-12-02T23:33:50Z

extensions/controllers/sandboxclaim_controller.go

+			Namespace:   claim.Namespace,
+			Name:        claim.Name,
+			Labels:      claim.Labels,
+			Annotations: claim.Annotations,


Similar to the labels, this is a shallow copy for annotations. More importantly, blindly copying all annotations can cause issues. For instance, the kubectl.kubernetes.io/last-applied-configuration annotation should not be carried over from the claim to the sandbox, as it could interfere with kubectl apply operations on the sandbox object itself.

You should implement a filtering mechanism to exclude known internal or problematic annotations before assigning them.

barney-s · 2025-12-02T23:33:50Z

extensions/examples/sandboxclaim.yaml

+  labels:
+    test-label: test-value
+  annotations:
+    test-annotation: test-annotation-value


While this example is functionally correct, it could be more illustrative. Consider adding a more realistic annotation that a user might want to use, for example description: "My custom sandbox environment". This helps users better understand the intended use of the new feature.

barney-s · 2025-12-02T23:33:50Z

extensions/controllers/sandboxclaim_controller.go

 	}

 	logger.Info("creating sandbox from template", "template", template.Name)
 	sandbox := &v1alpha1.Sandbox{


The created Sandbox is missing an OwnerReference that points to the SandboxClaim. Without this, the Sandbox will not be garbage collected when the SandboxClaim is deleted, and it breaks the ownership chain that is fundamental to Kubernetes controllers.

You should set the OwnerReference on the Sandbox's ObjectMeta.

barney-s · 2025-12-02T23:33:51Z

extensions/controllers/sandboxclaim_controller_test.go

 	}

+	claimWithMetadata := claim.DeepCopy()
+	claimWithMetadata.Labels = map[string]string{


The test should be more comprehensive. It should also handle cases where claim.Labels or claim.Annotations are nil to ensure the controller doesn't panic.

barney-s · 2025-12-02T23:33:51Z

extensions/controllers/sandboxclaim_controller_test.go

-		expectError       bool
-		expectedCondition metav1.Condition
+		name                string
+		existingObjects     []client.Object


This test does not validate that a deep copy of the labels and annotations was performed. To properly test this, you should modify the claim.Labels and claim.Annotations maps after the createSandbox function is called, and then verify that the labels and annotations on the created sandbox object have not changed.

k8s-ci-robot requested a review from janetkuo November 20, 2025 18:00

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 20, 2025

k8s-ci-robot requested a review from justinsb November 20, 2025 18:00

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 20, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 20, 2025

vicentefb reviewed Nov 20, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 20, 2025

peterzhongyi force-pushed the b2 branch from 29aece6 to 66a07e6 Compare November 20, 2025 19:20

Allow sandboxclaim's metadata to be passed to the sandbox

75d1a71

peterzhongyi force-pushed the b2 branch from 66a07e6 to 75d1a71 Compare November 21, 2025 18:05

barney-s reviewed Nov 21, 2025

View reviewed changes

janetkuo reviewed Nov 21, 2025

View reviewed changes

barney-s reviewed Dec 2, 2025

View reviewed changes

	for k, v := range sandbox.Spec.PodTemplate.ObjectMeta.Labels {
	labels[k] = v
	}
	annotations := map[string]string{}
	for k, v := range sandbox.Spec.PodTemplate.ObjectMeta.Annotations {
	annotations[k] = v
	}

Allow sandboxclaim's metadata to be passed to the sandbox #173

Are you sure you want to change the base?

Allow sandboxclaim's metadata to be passed to the sandbox #173

Conversation

peterzhongyi commented Nov 20, 2025

Uh oh!

k8s-ci-robot commented Nov 20, 2025

Uh oh!

k8s-ci-robot commented Nov 20, 2025

Uh oh!

netlify bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for agent-sandbox canceled.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janetkuo commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barney-s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented Nov 20, 2025 •

edited

Loading