diff --git a/apis/apps/defaults/v1alpha1.go b/apis/apps/defaults/v1alpha1.go index 9a15eaeb80..89baa72574 100644 --- a/apis/apps/defaults/v1alpha1.go +++ b/apis/apps/defaults/v1alpha1.go @@ -37,11 +37,11 @@ func SetDefaultsSidecarSet(obj *v1alpha1.SidecarSet) { setSidecarSetUpdateStrategy(&obj.Spec.UpdateStrategy) for i := range obj.Spec.InitContainers { - setSidecarDefaultContainer(&obj.Spec.InitContainers[i]) + setDefaultSidecarContainer(&obj.Spec.InitContainers[i], v1alpha1.AfterAppContainerType) } for i := range obj.Spec.Containers { - setDefaultSidecarContainer(&obj.Spec.Containers[i]) + setDefaultSidecarContainer(&obj.Spec.Containers[i], v1alpha1.BeforeAppContainerType) } //default setting volumes @@ -74,9 +74,9 @@ func SetDefaultRevisionHistoryLimit(revisionHistoryLimit **int32) { } } -func setDefaultSidecarContainer(sidecarContainer *v1alpha1.SidecarContainer) { +func setDefaultSidecarContainer(sidecarContainer *v1alpha1.SidecarContainer, injectPolicy v1alpha1.PodInjectPolicyType) { if sidecarContainer.PodInjectPolicy == "" { - sidecarContainer.PodInjectPolicy = v1alpha1.BeforeAppContainerType + sidecarContainer.PodInjectPolicy = injectPolicy } if sidecarContainer.UpgradeStrategy.UpgradeType == "" { sidecarContainer.UpgradeStrategy.UpgradeType = v1alpha1.SidecarContainerColdUpgrade @@ -85,7 +85,7 @@ func setDefaultSidecarContainer(sidecarContainer *v1alpha1.SidecarContainer) { sidecarContainer.ShareVolumePolicy.Type = v1alpha1.ShareVolumePolicyDisabled } - setSidecarDefaultContainer(sidecarContainer) + setDefaultContainer(sidecarContainer) } func setSidecarSetUpdateStrategy(strategy *v1alpha1.SidecarSetUpdateStrategy) { @@ -102,7 +102,7 @@ func setSidecarSetUpdateStrategy(strategy *v1alpha1.SidecarSetUpdateStrategy) { } } -func setSidecarDefaultContainer(sidecarContainer *v1alpha1.SidecarContainer) { +func setDefaultContainer(sidecarContainer *v1alpha1.SidecarContainer) { container := &sidecarContainer.Container v1.SetDefaults_Container(container) for i := range container.Ports { diff --git a/pkg/webhook/pod/mutating/sidecarset.go b/pkg/webhook/pod/mutating/sidecarset.go index 194738c095..21528d0778 100644 --- a/pkg/webhook/pod/mutating/sidecarset.go +++ b/pkg/webhook/pod/mutating/sidecarset.go @@ -152,10 +152,7 @@ func (h *PodCreateHandler) sidecarsetMutatingPod(ctx context.Context, req admiss sort.SliceStable(sidecarInitContainers, func(i, j int) bool { return sidecarInitContainers[i].Name < sidecarInitContainers[j].Name }) - // TODO, implement PodInjectPolicy for initContainers - for _, initContainer := range sidecarInitContainers { - pod.Spec.InitContainers = append(pod.Spec.InitContainers, initContainer.Container) - } + pod.Spec.InitContainers = mergeSidecarContainers(pod.Spec.InitContainers, sidecarInitContainers) // 2. inject containers pod.Spec.Containers = mergeSidecarContainers(pod.Spec.Containers, sidecarContainers) // 3. inject volumes @@ -287,7 +284,7 @@ func mergeSidecarContainers(origins []corev1.Container, injected []*appsv1alpha1 case appsv1alpha1.AfterAppContainerType: afterAppContainers = append(afterAppContainers, sidecar.Container) default: - beforeAppContainers = append(beforeAppContainers, sidecar.Container) + afterAppContainers = append(afterAppContainers, sidecar.Container) } } origins = append(beforeAppContainers, origins...) diff --git a/pkg/webhook/pod/mutating/sidecarset_test.go b/pkg/webhook/pod/mutating/sidecarset_test.go index 517c661180..f3f6f83062 100644 --- a/pkg/webhook/pod/mutating/sidecarset_test.go +++ b/pkg/webhook/pod/mutating/sidecarset_test.go @@ -23,6 +23,7 @@ import ( "os" "path/filepath" "reflect" + "sort" "testing" "github.com/openkruise/kruise/apis" @@ -946,6 +947,7 @@ func TestSidecarSetTransferEnv(t *testing.T) { func testSidecarSetTransferEnv(t *testing.T, sidecarSetIn *appsv1alpha1.SidecarSet) { podIn := pod1.DeepCopy() + sidecarSetIn.Spec.InitContainers[0].PodInjectPolicy = "" decoder := admission.NewDecoder(scheme.Scheme) client := fake.NewClientBuilder().WithObjects(sidecarSetIn).WithIndex( &appsv1alpha1.SidecarSet{}, fieldindex.IndexNameForSidecarSetNamespace, fieldindex.IndexSidecarSet, @@ -958,7 +960,7 @@ func testSidecarSetTransferEnv(t *testing.T, sidecarSetIn *appsv1alpha1.SidecarS t.Fatalf("inject sidecar into pod failed, err: %v", err) } if len(podOut.Spec.InitContainers[1].Env) != 2 { - t.Fatalf("expect 2 envs but got %v", len(podOut.Spec.InitContainers[0].Env)) + t.Fatalf("expect 2 envs but got %v", len(podOut.Spec.InitContainers[1].Env)) } if podOut.Spec.InitContainers[1].Env[1].Value != "world2" { t.Fatalf("expect env with value 'world2' but got %v", podOut.Spec.Containers[0].Env[1].Value) @@ -1129,6 +1131,17 @@ func TestMergeSidecarContainers(t *testing.T) { expectContainerLen: 4, expectedContainers: []string{"new-sidecar-1", "sidecar-1", "app-container", "sidecar-2"}, }, + { + name: "origin no sidecar, sidecar have new containers", + getOrigins: func() []corev1.Container { + return nil + }, + getInjected: func() []*appsv1alpha1.SidecarContainer { + return sidecarContainers + }, + expectContainerLen: 3, + expectedContainers: []string{"new-sidecar-1", "sidecar-1", "sidecar-2"}, + }, } for _, cs := range cases { @@ -1255,6 +1268,134 @@ func TestInjectInitContainer(t *testing.T) { } } +func TestInjectInitContainerSort(t *testing.T) { + cases := []struct { + name string + getOrigins func() []corev1.Container + getInjected func() []*appsv1alpha1.SidecarContainer + expectContainerLen int + expectedContainers []string + }{ + { + name: "origins nil, inject containers(a, b, c)", + getOrigins: func() []corev1.Container { + return nil + }, + getInjected: func() []*appsv1alpha1.SidecarContainer { + return []*appsv1alpha1.SidecarContainer{ + { + Container: corev1.Container{ + Name: "a-init", + }, + }, + { + Container: corev1.Container{ + Name: "c-init", + }, + }, + { + Container: corev1.Container{ + Name: "b-init", + }, + }, + } + }, + expectContainerLen: 3, + expectedContainers: []string{"a-init", "b-init", "c-init"}, + }, + { + name: "origin init, inject containers(a, b, c)", + getOrigins: func() []corev1.Container { + return []corev1.Container{ + { + Name: "app1-init", + }, + { + Name: "app2-init", + }, + } + }, + getInjected: func() []*appsv1alpha1.SidecarContainer { + return []*appsv1alpha1.SidecarContainer{ + { + Container: corev1.Container{ + Name: "a-init", + }, + }, + { + Container: corev1.Container{ + Name: "c-init", + }, + }, + { + Container: corev1.Container{ + Name: "b-init", + }, + }, + } + }, + expectContainerLen: 5, + expectedContainers: []string{"app1-init", "app2-init", "a-init", "b-init", "c-init"}, + }, + { + name: "origins nil, inject containers(a, b, c)-2", + getOrigins: func() []corev1.Container { + return []corev1.Container{ + { + Name: "app1-init", + }, + { + Name: "app2-init", + }, + } + }, + getInjected: func() []*appsv1alpha1.SidecarContainer { + return []*appsv1alpha1.SidecarContainer{ + { + Container: corev1.Container{ + Name: "a-init", + }, + PodInjectPolicy: appsv1alpha1.AfterAppContainerType, + }, + { + Container: corev1.Container{ + Name: "c-init", + }, + PodInjectPolicy: appsv1alpha1.BeforeAppContainerType, + }, + { + Container: corev1.Container{ + Name: "b-init", + }, + PodInjectPolicy: appsv1alpha1.BeforeAppContainerType, + }, + } + }, + expectContainerLen: 5, + expectedContainers: []string{"b-init", "c-init", "app1-init", "app2-init", "a-init"}, + }, + } + + for _, cs := range cases { + t.Run(cs.name, func(t *testing.T) { + origins := cs.getOrigins() + injected := cs.getInjected() + sort.SliceStable(injected, func(i, j int) bool { + return injected[i].Name < injected[j].Name + }) + finals := mergeSidecarContainers(origins, injected) + if len(finals) != cs.expectContainerLen { + t.Fatalf("expect %d containers but got %v", cs.expectContainerLen, len(finals)) + } + for index, cName := range cs.expectedContainers { + if finals[index].Name != cName { + t.Fatalf("expect index(%d) container(%s) but got %s", index, cName, finals[index].Name) + } + } + }) + } +} + func newAdmission(op admissionv1.Operation, object, oldObject runtime.RawExtension, subResource string) admission.Request { return admission.Request{ AdmissionRequest: newAdmissionRequest(op, object, oldObject, subResource), diff --git a/pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go b/pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go index 8dbf4dc5b9..4c1584c0f1 100644 --- a/pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go +++ b/pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go @@ -90,6 +90,9 @@ func (h *SidecarSetCreateUpdateHandler) validateSidecarSet(obj *appsv1alpha1.Sid if older != nil { allErrs = append(allErrs, validateSidecarContainerConflict(obj.Spec.Containers, older.Spec.Containers, field.NewPath("spec.containers"))...) } + if len(allErrs) != 0 { + return allErrs + } // iterate across all containers in other sidecarsets to avoid duplication of name sidecarSets := &appsv1alpha1.SidecarSetList{} if err := h.Client.List(context.TODO(), sidecarSets, &client.ListOptions{}); err != nil { diff --git a/test/e2e/apps/containerrecreate.go b/test/e2e/apps/containerrecreate.go index bac02d2190..113ee6d85e 100644 --- a/test/e2e/apps/containerrecreate.go +++ b/test/e2e/apps/containerrecreate.go @@ -20,12 +20,11 @@ import ( "fmt" "time" - "github.com/openkruise/kruise/pkg/util" - "github.com/onsi/ginkgo" "github.com/onsi/gomega" appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" kruiseclientset "github.com/openkruise/kruise/pkg/client/clientset/versioned" + "github.com/openkruise/kruise/pkg/util" utilpodreadiness "github.com/openkruise/kruise/pkg/util/podreadiness" "github.com/openkruise/kruise/test/e2e/framework" v1 "k8s.io/api/core/v1" @@ -33,6 +32,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" clientset "k8s.io/client-go/kubernetes" + "k8s.io/klog/v2" podutil "k8s.io/kubernetes/pkg/api/v1/pod" utilpointer "k8s.io/utils/pointer" ) @@ -216,6 +216,7 @@ var _ = SIGDescribe("ContainerRecreateRequest", func() { gomega.Expect(err).NotTo(gomega.HaveOccurred()) return crr.Status.Phase }, 60*time.Second, 3*time.Second).Should(gomega.Equal(appsv1alpha1.ContainerRecreateRequestCompleted)) + klog.Infof("CRR info(%s)", util.DumpJSON(crr)) gomega.Expect(crr.Status.CompletionTime).ShouldNot(gomega.BeNil()) gomega.Expect(crr.Status.ContainerRecreateStates).Should(gomega.Equal([]appsv1alpha1.ContainerRecreateRequestContainerRecreateState{ {Name: "app", Phase: appsv1alpha1.ContainerRecreateRequestSucceeded, IsKilled: true}, diff --git a/test/e2e/apps/sidecarset.go b/test/e2e/apps/sidecarset.go index 1667cea830..6acfd4a6f4 100644 --- a/test/e2e/apps/sidecarset.go +++ b/test/e2e/apps/sidecarset.go @@ -1328,6 +1328,8 @@ var _ = SIGDescribe("SidecarSet", func() { sidecarSetIn.Spec.Containers = sidecarSetIn.Spec.Containers[:1] ginkgo.By(fmt.Sprintf("Creating SidecarSet %s", sidecarSetIn.Name)) sidecarSetIn, _ = tester.CreateSidecarSet(sidecarSetIn) + gomega.Expect(sidecarSetIn.Spec.InitContainers[0].PodInjectPolicy).To(gomega.Equal(appsv1alpha1.AfterAppContainerType)) + gomega.Expect(sidecarSetIn.Spec.Containers[0].PodInjectPolicy).To(gomega.Equal(appsv1alpha1.BeforeAppContainerType)) time.Sleep(time.Second) // create deployment diff --git a/test/e2e/framework/containerrecreate_util.go b/test/e2e/framework/containerrecreate_util.go index ce48465aa0..8e5fd82d07 100644 --- a/test/e2e/framework/containerrecreate_util.go +++ b/test/e2e/framework/containerrecreate_util.go @@ -23,9 +23,11 @@ import ( "github.com/onsi/gomega" appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" kruiseclientset "github.com/openkruise/kruise/pkg/client/clientset/versioned" + "github.com/openkruise/kruise/pkg/util" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientset "k8s.io/client-go/kubernetes" + "k8s.io/klog/v2" ) type ContainerRecreateTester struct { @@ -85,6 +87,7 @@ func (t *ContainerRecreateTester) CreateTestCloneSetAndGetPods(randStr string, r gomega.Expect(err).NotTo(gomega.HaveOccurred()) for i := range podList.Items { p := &podList.Items[i] + klog.Infof("Pod(%s/%s/%s) status(%s)", p.Namespace, p.Name, p.UID, util.DumpJSON(p.Status)) pods = append(pods, p) } return