fix(kubernetes): reject re-pointing BatchSandbox.spec.poolRef to another pool #1434
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,7 +96,12 @@ spec: | |
| metadata: | ||
| type: object | ||
| spec: | ||
| description: |- | ||
| BatchSandboxSpec defines the desired state of BatchSandbox. | ||
| spec.poolRef may only be set on an unbound sandbox ("" or "*"), or cleared to | ||
| detach; re-pointing a bound sandbox to a different pool is rejected because | ||
| the previous pool would recycle the in-use pods while the stale allocation | ||
| record blocks the new pool from supplying replacements. | ||
| properties: | ||
| expireTime: | ||
| description: |- | ||
|
|
@@ -148,6 +153,12 @@ spec: | |
| required: | ||
| - replicas | ||
| type: object | ||
| x-kubernetes-validations: | ||
|
There was a problem hiding this comment.
On clusters that this chart still advertises as supported ( Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Thanks for the catch — I verified the details before responding:
Given that, this PR intentionally degrades gracefully rather than narrowing the declared support window ( Whether to raise the chart |
||
| - message: spec.poolRef cannot be re-pointed to a different pool; clear | ||
| it first to detach | ||
| rule: '!has(oldSelf.poolRef) || size(oldSelf.poolRef) == 0 || oldSelf.poolRef | ||
| == ''*'' || !has(self.poolRef) || size(self.poolRef) == 0 || self.poolRef | ||
| == oldSelf.poolRef' | ||
| status: | ||
| description: BatchSandboxStatus defines the observed state of BatchSandbox. | ||
| properties: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,235 @@ | ||
| // Copyright 2025 Alibaba Group Holding Ltd. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package controller | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| v1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/fields" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/rand" | ||
| "k8s.io/client-go/util/retry" | ||
| "k8s.io/utils/ptr" | ||
| kclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| "github.com/alibaba/OpenSandbox/sandbox-k8s/internal/utils/fieldindex" | ||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
|
|
||
| sandboxv1alpha1 "github.com/alibaba/OpenSandbox/sandbox-k8s/apis/sandbox/v1alpha1" | ||
| ) | ||
|
|
||
| // Regression tests for the spec.poolRef CEL transition rule. Re-pointing a | ||
| // bound BatchSandbox to a different pool used to make the previous pool | ||
| // recycle the in-use pod while the stale allocation record blocked the new | ||
| // pool from supplying a replacement, leaving the sandbox permanently starved. | ||
| var _ = Describe("BatchSandbox poolRef immutability", func() { | ||
| var ( | ||
| timeout = 15 * time.Second | ||
| interval = 1 * time.Second | ||
| ) | ||
|
|
||
| Context("when a bound BatchSandbox attempts to re-point its poolRef", func() { | ||
| var ( | ||
| poolAName types.NamespacedName | ||
| poolBName types.NamespacedName | ||
| ) | ||
|
|
||
| newPool := func(name types.NamespacedName) *sandboxv1alpha1.Pool { | ||
| return &sandboxv1alpha1.Pool{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name.Name, | ||
| Namespace: name.Namespace, | ||
| }, | ||
| Spec: sandboxv1alpha1.PoolSpec{ | ||
| Template: &v1.PodTemplateSpec{ | ||
| Spec: v1.PodSpec{ | ||
| Containers: []v1.Container{ | ||
| { | ||
| Name: "main", | ||
| Image: "example.com", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| CapacitySpec: sandboxv1alpha1.CapacitySpec{ | ||
| PoolMin: 0, | ||
| PoolMax: 2, | ||
| BufferMin: 1, | ||
| BufferMax: 1, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| markPoolPodsReady := func(g Gomega, name types.NamespacedName) { | ||
| pool := &sandboxv1alpha1.Pool{} | ||
| g.Expect(k8sClient.Get(ctx, name, pool)).To(Succeed()) | ||
| pods := &v1.PodList{} | ||
| g.Expect(k8sClient.List(ctx, pods, &kclient.ListOptions{ | ||
| Namespace: name.Namespace, | ||
| FieldSelector: fields.SelectorFromSet(fields.Set{fieldindex.IndexNameForOwnerRefUID: string(pool.UID)}), | ||
| })).To(Succeed()) | ||
| for i := range pods.Items { | ||
| pod := &pods.Items[i] | ||
| if pod.DeletionTimestamp != nil || pod.Status.Phase == v1.PodRunning { | ||
| continue | ||
| } | ||
| pod.Status.Phase = v1.PodRunning | ||
| pod.Status.Conditions = []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionTrue}} | ||
| g.Expect(k8sClient.Status().Update(ctx, pod)).To(Succeed()) | ||
| } | ||
| } | ||
|
|
||
| waitPoolReady := func(name types.NamespacedName) { | ||
| Eventually(func(g Gomega) { | ||
| pool := &sandboxv1alpha1.Pool{} | ||
| g.Expect(k8sClient.Get(ctx, name, pool)).To(Succeed()) | ||
| g.Expect(pool.Status.ObservedGeneration).To(Equal(pool.Generation)) | ||
| g.Expect(pool.Status.Total).To(BeNumerically(">=", 1)) | ||
| markPoolPodsReady(g, name) | ||
| }, timeout, interval).Should(Succeed()) | ||
| } | ||
|
|
||
| BeforeEach(func() { | ||
| poolAName = types.NamespacedName{Name: "immu-pool-a-" + rand.String(8), Namespace: "default"} | ||
| poolBName = types.NamespacedName{Name: "immu-pool-b-" + rand.String(8), Namespace: "default"} | ||
| Expect(k8sClient.Create(ctx, newPool(poolAName))).To(Succeed()) | ||
| Expect(k8sClient.Create(ctx, newPool(poolBName))).To(Succeed()) | ||
| waitPoolReady(poolAName) | ||
| waitPoolReady(poolBName) | ||
| }) | ||
|
|
||
| AfterEach(func() { | ||
| for _, name := range []types.NamespacedName{poolAName, poolBName} { | ||
| pool := &sandboxv1alpha1.Pool{} | ||
| if err := k8sClient.Get(ctx, name, pool); err == nil { | ||
| Expect(k8sClient.Delete(ctx, pool)).To(Succeed()) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| It("rejects the re-point and keeps the existing allocation intact", func() { | ||
| bsbxName := types.NamespacedName{ | ||
| Name: "immu-switch-" + rand.String(8), | ||
| Namespace: "default", | ||
| } | ||
| batchSandbox := &sandboxv1alpha1.BatchSandbox{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: bsbxName.Name, | ||
| Namespace: bsbxName.Namespace, | ||
| }, | ||
| Spec: sandboxv1alpha1.BatchSandboxSpec{ | ||
| Replicas: ptr.To(int32(1)), | ||
| PoolRef: poolAName.Name, | ||
| }, | ||
| } | ||
| Expect(k8sClient.Create(ctx, batchSandbox)).To(Succeed()) | ||
|
|
||
| By("waiting for pool A to allocate a pod to the sandbox") | ||
| var allocatedPod string | ||
| Eventually(func(g Gomega) { | ||
| g.Expect(k8sClient.Get(ctx, bsbxName, batchSandbox)).To(Succeed()) | ||
| alloc, err := getSandboxAllocation(batchSandbox) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(alloc.Pods).To(HaveLen(1)) | ||
| allocatedPod = alloc.Pods[0] | ||
| }, timeout, interval).Should(Succeed()) | ||
|
|
||
| By("attempting to re-point spec.poolRef from pool A to pool B") | ||
| err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| latest := &sandboxv1alpha1.BatchSandbox{} | ||
| if err := k8sClient.Get(ctx, bsbxName, latest); err != nil { | ||
| return err | ||
| } | ||
| latest.Spec.PoolRef = poolBName.Name | ||
| return k8sClient.Update(ctx, latest) | ||
| }) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("cannot be re-pointed")) | ||
|
|
||
| By("verifying the sandbox keeps its pod and binding") | ||
| Consistently(func(g Gomega) { | ||
| pod := &v1.Pod{} | ||
| g.Expect(k8sClient.Get(ctx, types.NamespacedName{ | ||
| Name: allocatedPod, | ||
| Namespace: bsbxName.Namespace, | ||
| }, pod)).To(Succeed()) | ||
| g.Expect(pod.DeletionTimestamp).To(BeNil()) | ||
|
|
||
| g.Expect(k8sClient.Get(ctx, bsbxName, batchSandbox)).To(Succeed()) | ||
| g.Expect(batchSandbox.Spec.PoolRef).To(Equal(poolAName.Name)) | ||
| alloc, err := getSandboxAllocation(batchSandbox) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(alloc.Pods).To(ConsistOf(allocatedPod)) | ||
| }, 5*time.Second, interval).Should(Succeed()) | ||
|
|
||
| Expect(k8sClient.Delete(ctx, batchSandbox)).To(Succeed()) | ||
| }) | ||
|
|
||
| It("still allows binding an unbound sandbox once and detaching afterwards", func() { | ||
| bsbxName := types.NamespacedName{ | ||
| Name: "immu-bind-" + rand.String(8), | ||
| Namespace: "default", | ||
| } | ||
| batchSandbox := &sandboxv1alpha1.BatchSandbox{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: bsbxName.Name, | ||
| Namespace: bsbxName.Namespace, | ||
| }, | ||
| Spec: sandboxv1alpha1.BatchSandboxSpec{ | ||
| Replicas: ptr.To(int32(0)), | ||
| }, | ||
| } | ||
| Expect(k8sClient.Create(ctx, batchSandbox)).To(Succeed()) | ||
|
|
||
| By("binding the unbound sandbox to pool A is allowed") | ||
| Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| latest := &sandboxv1alpha1.BatchSandbox{} | ||
| if err := k8sClient.Get(ctx, bsbxName, latest); err != nil { | ||
| return err | ||
| } | ||
| latest.Spec.PoolRef = poolAName.Name | ||
| return k8sClient.Update(ctx, latest) | ||
| })).To(Succeed()) | ||
|
|
||
| By("re-pointing the now-bound sandbox to pool B is rejected") | ||
| err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| latest := &sandboxv1alpha1.BatchSandbox{} | ||
| if err := k8sClient.Get(ctx, bsbxName, latest); err != nil { | ||
| return err | ||
| } | ||
| latest.Spec.PoolRef = poolBName.Name | ||
| return k8sClient.Update(ctx, latest) | ||
| }) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("cannot be re-pointed")) | ||
|
|
||
| By("clearing poolRef to detach is allowed") | ||
| Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| latest := &sandboxv1alpha1.BatchSandbox{} | ||
| if err := k8sClient.Get(ctx, bsbxName, latest); err != nil { | ||
| return err | ||
| } | ||
| latest.Spec.PoolRef = "" | ||
| return k8sClient.Update(ctx, latest) | ||
| })).To(Succeed()) | ||
|
|
||
| Expect(k8sClient.Delete(ctx, batchSandbox)).To(Succeed()) | ||
| }) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: this rule raises the project Kubernetes compatibility floor to >= 1.25, conflicting with what the repo claims and tests today.
x-kubernetes-validationswitholdSelf(transition rules) requires Kubernetes >= 1.25 to be accepted and enforced (GA in 1.29). On 1.23/1.24 it is behind theValidationRulesfeature gate (off by default), and on < 1.23 the field is rejected outright — the CRD cannot be installed on those clusters at all.But this repo currently:
.github/workflows/kubernetes-test.yml), and the e2e suite installs the CRDs viamake install(test/e2e/e2e_test.go) — those legs will fail at CRD apply.kubeVersion: ">=1.21.1-0"incharts/opensandbox-controller/Chart.yaml, says "Kubernetes 1.21.1+" in the chart README, and defaults local e2e toKIND_K8S_VERSION=v1.22.4(kubernetes/Makefile).So the "Breaking Changes: None" claim in the PR description is not accurate — this effectively drops support for < 1.25 clusters. We need a maintainer decision before merge:
Chart.yamlkubeVersion, chart README, CI e2e matrix, local e2e default), orThe rule logic itself is correct for the documented transitions (initial bind,
"*"-> name write-back, detach, no-op); the concern is purely deployment-surface compatibility.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks — you're right. My earlier statement that the older-version CI matrix continued to pass was not supported; this PR has only run the auto-label check.
I also checked the existing validation infrastructure. OpenSandbox does not currently enable a validating webhook, so replacing the CEL rule with one would add a new deployment surface (webhook service/certificates and Helm/Kustomize wiring). A controller-side rollback cannot reject the update at admission time and may race with the old pool's orphan cleanup.
I agree that raising the Kubernetes support floor should not be done implicitly in this bug fix. Would you prefer introducing validating-webhook support for this invariant, or handling pool-to-pool re-pointing as an explicit safe controller transition? I'm happy to revise the PR once the intended direction is clear.
AI usage disclosure: I used OpenAI Codex to inspect the repository's existing validation/controller paths and help draft this response; I reviewed the conclusions before posting.