From 82c2d6692ea5bdcfe7fd777ef2a6415b0b3a00ec Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Wed, 16 Oct 2024 14:08:12 +0000 Subject: [PATCH] Remove temporary helm values * `statefulsetRunnerTemporarySetPodSeccompProfile` * `jobTaskRunnerTemporarySetPodSeccompProfile` Those helm values were introduced to avoid workloads restart on Korifi upgrade. With the upcoming release workloads would be restarted by an unrelated change anyway, so it is a convenient moment to get rid of those. issue #3164 Co-authored-by: Georgi Sabev --- README.helm.md | 2 - controllers/config/config.go | 6 +-- controllers/main.go | 6 +-- helm/korifi/controllers/configmap.yaml | 4 -- helm/korifi/values.schema.json | 8 --- helm/korifi/values.yaml | 2 - .../controllers/integration/suite_test.go | 1 - .../taskworkload_controller_test.go | 3 ++ .../controllers/taskworkload_controller.go | 38 ++++++------- .../taskworkload_controller_test.go | 54 +++++-------------- .../buildworkload_controller_test.go | 4 +- .../controllers/appworkload_to_stset.go | 18 ++----- .../controllers/appworkload_to_stset_test.go | 32 ++++------- .../controllers/integration/suite_test.go | 2 +- 14 files changed, 50 insertions(+), 130 deletions(-) diff --git a/README.helm.md b/README.helm.md index 4f539e60a..a205eea19 100644 --- a/README.helm.md +++ b/README.helm.md @@ -92,7 +92,6 @@ Here are all the values that can be set for the chart: - `requests`: Resource requests. - `cpu` (_String_): CPU request. - `memory` (_String_): Memory request. - - `temporarySetPodSeccompProfile` (_Boolean_): Sets the pod .spec.securityContext.seccompProfile to RuntimeDefault. Setting this flag to true will cause a restart of all previously running pods. - `kpackImageBuilder`: - `builderReadinessTimeout` (_String_): The time that the kpack Builder will be waited for if not in ready state, berfore the build workload fails. See [`time.ParseDuration`](https://pkg.go.dev/time#ParseDuration) for details on the format, an additional `d` suffix for days is supported. - `builderRepository` (_String_): Container image repository to store the `ClusterBuilder` image. Required when `clusterBuilderName` is not provided. @@ -134,5 +133,4 @@ Here are all the values that can be set for the chart: - `requests`: Resource requests. - `cpu` (_String_): CPU request. - `memory` (_String_): Memory request. - - `temporarySetPodSeccompProfile` (_Boolean_): Sets the pod .spec.securityContext.seccompProfile to RuntimeDefault. Setting this flag to true will cause a restart of all previously running pods. - `systemImagePullSecrets` (_Array_): List of `Secret` names to be used when pulling Korifi system images from private registries diff --git a/controllers/config/config.go b/controllers/config/config.go index f0877635d..348d650ca 100644 --- a/controllers/config/config.go +++ b/controllers/config/config.go @@ -30,11 +30,7 @@ type ControllerConfig struct { SpaceFinalizerAppDeletionTimeout *int32 `yaml:"spaceFinalizerAppDeletionTimeout"` // job-task-runner - JobTTL string `yaml:"jobTTL"` - JobTaskRunnerTemporarySetPodSeccompProfile bool `yaml:"jobTaskRunnerTemporarySetPodSeccompProfile"` - - // statefulset-runner - StatefulsetRunnerTemporarySetPodSeccompProfile bool `yaml:"statefulsetRunnerTemporarySetPodSeccompProfile"` + JobTTL string `yaml:"jobTTL"` // kpack-image-builder ClusterBuilderName string `yaml:"clusterBuilderName"` diff --git a/controllers/main.go b/controllers/main.go index 90a09bdbd..a62eaffa6 100644 --- a/controllers/main.go +++ b/controllers/main.go @@ -393,7 +393,6 @@ func main() { mgr.GetScheme(), jobtaskrunnercontrollers.NewStatusGetter(mgr.GetClient()), jobTTL, - controllerConfig.JobTaskRunnerTemporarySetPodSeccompProfile, ) if err = taskWorkloadReconciler.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "TaskWorkload") @@ -405,10 +404,7 @@ func main() { if err = statefulsetcontrollers.NewAppWorkloadReconciler( mgr.GetClient(), mgr.GetScheme(), - statefulsetcontrollers.NewAppWorkloadToStatefulsetConverter( - mgr.GetScheme(), - controllerConfig.StatefulsetRunnerTemporarySetPodSeccompProfile, - ), + statefulsetcontrollers.NewAppWorkloadToStatefulsetConverter(mgr.GetScheme()), statefulsetcontrollers.NewPDBUpdater(mgr.GetClient()), controllersLog, ).SetupWithManager(mgr); err != nil { diff --git a/helm/korifi/controllers/configmap.yaml b/helm/korifi/controllers/configmap.yaml index a4d70a6a5..562578be0 100644 --- a/helm/korifi/controllers/configmap.yaml +++ b/helm/korifi/controllers/configmap.yaml @@ -53,10 +53,6 @@ data: {{- end }} {{- if .Values.jobTaskRunner.include }} jobTTL: {{ required "jobTTL is required" .Values.jobTaskRunner.jobTTL }} - jobTaskRunnerTemporarySetPodSeccompProfile: {{ .Values.jobTaskRunner.temporarySetPodSeccompProfile }} - {{- end }} - {{- if .Values.statefulsetRunner.include }} - statefulsetRunnerTemporarySetPodSeccompProfile: {{ .Values.statefulsetRunner.temporarySetPodSeccompProfile }} {{- end }} networking: gatewayNamespace: {{ .Release.Namespace }}-gateway diff --git a/helm/korifi/values.schema.json b/helm/korifi/values.schema.json index f18634f98..993da4528 100644 --- a/helm/korifi/values.schema.json +++ b/helm/korifi/values.schema.json @@ -469,10 +469,6 @@ "description": "Number of replicas.", "type": "integer" }, - "temporarySetPodSeccompProfile": { - "description": "Sets the pod .spec.securityContext.seccompProfile to RuntimeDefault. Setting this flag to true will cause a restart of all previously running pods.", - "type": "boolean" - }, "resources": { "description": "[`ResourceRequirements`](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.25/#resourcerequirements-v1-core) for the API.", "type": "object", @@ -521,10 +517,6 @@ "description": "Number of replicas.", "type": "integer" }, - "temporarySetPodSeccompProfile": { - "description": "Sets the pod .spec.securityContext.seccompProfile to RuntimeDefault. Setting this flag to true will cause a restart of all previously running pods.", - "type": "boolean" - }, "resources": { "description": "[`ResourceRequirements`](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.25/#resourcerequirements-v1-core) for the API.", "type": "object", diff --git a/helm/korifi/values.yaml b/helm/korifi/values.yaml index 0c7574c20..932d4c0b3 100644 --- a/helm/korifi/values.yaml +++ b/helm/korifi/values.yaml @@ -112,7 +112,6 @@ kpackImageBuilder: statefulsetRunner: include: true replicas: 1 - temporarySetPodSeccompProfile: false resources: limits: cpu: 500m @@ -124,7 +123,6 @@ statefulsetRunner: jobTaskRunner: include: true replicas: 1 - temporarySetPodSeccompProfile: false resources: limits: cpu: 500m diff --git a/job-task-runner/controllers/integration/suite_test.go b/job-task-runner/controllers/integration/suite_test.go index e5735cb7a..a6763bd35 100644 --- a/job-task-runner/controllers/integration/suite_test.go +++ b/job-task-runner/controllers/integration/suite_test.go @@ -82,7 +82,6 @@ var _ = BeforeSuite(func() { k8sManager.GetScheme(), controllers.NewStatusGetter(k8sManager.GetClient()), time.Minute, - false, ) err = taskWorkloadReconciler.SetupWithManager(k8sManager) Expect(err).NotTo(HaveOccurred()) diff --git a/job-task-runner/controllers/integration/taskworkload_controller_test.go b/job-task-runner/controllers/integration/taskworkload_controller_test.go index 4a6161545..a5f947d46 100644 --- a/job-task-runner/controllers/integration/taskworkload_controller_test.go +++ b/job-task-runner/controllers/integration/taskworkload_controller_test.go @@ -77,6 +77,9 @@ var _ = Describe("Job TaskWorkload Controller Integration Test", func() { Expect(podSpec.RestartPolicy).To(Equal(corev1.RestartPolicyNever)) Expect(podSpec.SecurityContext).To(Equal(&corev1.PodSecurityContext{ RunAsNonRoot: tools.PtrTo(true), + SeccompProfile: &corev1.SeccompProfile{ + Type: corev1.SeccompProfileTypeRuntimeDefault, + }, })) Expect(podSpec.AutomountServiceAccountToken).To(Equal(tools.PtrTo(false))) Expect(podSpec.ImagePullSecrets).To(ConsistOf(corev1.LocalObjectReference{Name: "my-image-secret"})) diff --git a/job-task-runner/controllers/taskworkload_controller.go b/job-task-runner/controllers/taskworkload_controller.go index 08eb22f24..f2d1f9e01 100644 --- a/job-task-runner/controllers/taskworkload_controller.go +++ b/job-task-runner/controllers/taskworkload_controller.go @@ -51,12 +51,11 @@ type TaskStatusGetter interface { // TaskWorkloadReconciler reconciles a TaskWorkload object type TaskWorkloadReconciler struct { - k8sClient client.Client - logger logr.Logger - scheme *runtime.Scheme - statusGetter TaskStatusGetter - jobTTL time.Duration - jobTaskRunnerTemporarySetPodSeccompProfile bool + k8sClient client.Client + logger logr.Logger + scheme *runtime.Scheme + statusGetter TaskStatusGetter + jobTTL time.Duration } func NewTaskWorkloadReconciler( @@ -65,7 +64,6 @@ func NewTaskWorkloadReconciler( scheme *runtime.Scheme, statusGetter TaskStatusGetter, jobTTL time.Duration, - jobTaskRunnerTemporarySetPodSeccompProfile bool, ) *k8s.PatchingReconciler[korifiv1alpha1.TaskWorkload, *korifiv1alpha1.TaskWorkload] { taskReconciler := TaskWorkloadReconciler{ k8sClient: k8sClient, @@ -73,7 +71,6 @@ func NewTaskWorkloadReconciler( scheme: scheme, statusGetter: statusGetter, jobTTL: jobTTL, - jobTaskRunnerTemporarySetPodSeccompProfile: jobTaskRunnerTemporarySetPodSeccompProfile, } return k8s.NewPatchingReconciler[korifiv1alpha1.TaskWorkload, *korifiv1alpha1.TaskWorkload](logger, k8sClient, &taskReconciler) @@ -135,9 +132,9 @@ func (r TaskWorkloadReconciler) getOrCreateJob(ctx context.Context, logger logr. } func (r TaskWorkloadReconciler) createJob(ctx context.Context, logger logr.Logger, taskWorkload *korifiv1alpha1.TaskWorkload) (*batchv1.Job, error) { - job := WorkloadToJob(taskWorkload, int32(r.jobTTL.Seconds()), r.jobTaskRunnerTemporarySetPodSeccompProfile) - err := controllerutil.SetControllerReference(taskWorkload, job, r.scheme) + job, err := r.workloadToJob(taskWorkload) if err != nil { + logger.Info("failed to convert task workload to job", "reason", err) return nil, err } @@ -154,11 +151,7 @@ func (r TaskWorkloadReconciler) createJob(ctx context.Context, logger logr.Logge return job, nil } -func WorkloadToJob( - taskWorkload *korifiv1alpha1.TaskWorkload, - jobTTL int32, - jobTaskRunnerTemporarySetPodSeccompProfile bool, -) *batchv1.Job { +func (r *TaskWorkloadReconciler) workloadToJob(taskWorkload *korifiv1alpha1.TaskWorkload) (*batchv1.Job, error) { job := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ Name: taskWorkload.Name, @@ -168,12 +161,15 @@ func WorkloadToJob( BackoffLimit: tools.PtrTo(int32(0)), Parallelism: tools.PtrTo(int32(1)), Completions: tools.PtrTo(int32(1)), - TTLSecondsAfterFinished: tools.PtrTo(jobTTL), + TTLSecondsAfterFinished: tools.PtrTo(int32(r.jobTTL.Seconds())), Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ RestartPolicy: corev1.RestartPolicyNever, SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: tools.PtrTo(true), + SeccompProfile: &corev1.SeccompProfile{ + Type: corev1.SeccompProfileTypeRuntimeDefault, + }, }, AutomountServiceAccountToken: tools.PtrTo(false), ImagePullSecrets: taskWorkload.Spec.ImagePullSecrets, @@ -199,12 +195,12 @@ func WorkloadToJob( }, } - if jobTaskRunnerTemporarySetPodSeccompProfile { - job.Spec.Template.Spec.SecurityContext.SeccompProfile = &corev1.SeccompProfile{ - Type: corev1.SeccompProfileTypeRuntimeDefault, - } + err := controllerutil.SetControllerReference(taskWorkload, job, r.scheme) + if err != nil { + return nil, err } - return job + + return job, nil } func (r *TaskWorkloadReconciler) updateTaskWorkloadStatus(ctx context.Context, taskWorkload *korifiv1alpha1.TaskWorkload, job *batchv1.Job) error { diff --git a/job-task-runner/controllers/taskworkload_controller_test.go b/job-task-runner/controllers/taskworkload_controller_test.go index 67c091f01..5d983f133 100644 --- a/job-task-runner/controllers/taskworkload_controller_test.go +++ b/job-task-runner/controllers/taskworkload_controller_test.go @@ -8,10 +8,10 @@ import ( korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/job-task-runner/controllers" "code.cloudfoundry.org/korifi/job-task-runner/controllers/fake" + "code.cloudfoundry.org/korifi/tools/k8s" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -27,16 +27,16 @@ var _ = Describe("TaskworkloadController", func() { var ( statusGetter *fake.TaskStatusGetter - reconcileResult ctrl.Result - reconcileErr error - req ctrl.Request - taskWorkload *korifiv1alpha1.TaskWorkload - getTaskWorkloadError error - createdJob *batchv1.Job - existingJob *batchv1.Job - getExistingJobError error - createJobError error - jobTaskRunnerTemporarySetPodSeccompProfile bool + reconciler *k8s.PatchingReconciler[korifiv1alpha1.TaskWorkload, *korifiv1alpha1.TaskWorkload] + reconcileResult ctrl.Result + reconcileErr error + req ctrl.Request + taskWorkload *korifiv1alpha1.TaskWorkload + getTaskWorkloadError error + createdJob *batchv1.Job + existingJob *batchv1.Job + getExistingJobError error + createJobError error ) BeforeEach(func() { @@ -98,13 +98,12 @@ var _ = Describe("TaskworkloadController", func() { Reason: "something", }}, nil) - jobTaskRunnerTemporarySetPodSeccompProfile = false + reconciler = controllers.NewTaskWorkloadReconciler(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)), fakeClient, scheme.Scheme, statusGetter, time.Hour) req = ctrl.Request{NamespacedName: client.ObjectKeyFromObject(taskWorkload)} }) JustBeforeEach(func() { - reconciler := controllers.NewTaskWorkloadReconciler(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)), fakeClient, scheme.Scheme, statusGetter, time.Hour, jobTaskRunnerTemporarySetPodSeccompProfile) reconcileResult, reconcileErr = reconciler.Reconcile(context.Background(), req) }) @@ -225,33 +224,4 @@ var _ = Describe("TaskworkloadController", func() { Expect(reconcileErr).To(MatchError(ContainSubstring("get-conditions-error"))) }) }) - - Describe("jobTaskRunnerTemporarySetPodSeccompProfile", func() { - var ( - job *batchv1.Job - jobTaskRunnerTemporarySetPodSeccompProfile bool - ) - - BeforeEach(func() { - jobTaskRunnerTemporarySetPodSeccompProfile = false - }) - - JustBeforeEach(func() { - job = controllers.WorkloadToJob(taskWorkload, 123, jobTaskRunnerTemporarySetPodSeccompProfile) - }) - - It("does not set spec.securityContext.seccompProfile", func() { - Expect(job.Spec.Template.Spec.SecurityContext.SeccompProfile).To(BeNil()) - }) - - When("jobTaskRunnerTemporarySetPodSeccompProfile is set to true", func() { - BeforeEach(func() { - jobTaskRunnerTemporarySetPodSeccompProfile = true - }) - - It("sets spec.securityContext.seccompProfile to RuntimeDefault", func() { - Expect(job.Spec.Template.Spec.SecurityContext.SeccompProfile.Type).To(Equal(corev1.SeccompProfileTypeRuntimeDefault)) - }) - }) - }) }) diff --git a/kpack-image-builder/controllers/buildworkload_controller_test.go b/kpack-image-builder/controllers/buildworkload_controller_test.go index 1fd9e7552..8330144f8 100644 --- a/kpack-image-builder/controllers/buildworkload_controller_test.go +++ b/kpack-image-builder/controllers/buildworkload_controller_test.go @@ -1153,9 +1153,7 @@ var _ = Describe("BuildWorkloadReconciler", func() { }) When("failure during generateDropletStatus call", func() { - var ( - build *buildv1alpha2.Build - ) + var build *buildv1alpha2.Build BeforeEach(func() { fakeImageConfigGetter.ConfigReturns(image.Config{}, errors.New("fake error")) buildWorkload = buildWorkloadObject(buildWorkloadGUID, namespaceGUID, source, env, services, reconcilerName, buildpacks) diff --git a/statefulset-runner/controllers/appworkload_to_stset.go b/statefulset-runner/controllers/appworkload_to_stset.go index 8e58711b6..66f0d47e1 100644 --- a/statefulset-runner/controllers/appworkload_to_stset.go +++ b/statefulset-runner/controllers/appworkload_to_stset.go @@ -20,17 +20,12 @@ import ( ) type AppWorkloadToStatefulsetConverter struct { - scheme *runtime.Scheme - statefulsetRunnerTemporarySetPodSeccompProfile bool + scheme *runtime.Scheme } -func NewAppWorkloadToStatefulsetConverter( - scheme *runtime.Scheme, - statefulsetRunnerTemporarySetPodSeccompProfile bool, -) *AppWorkloadToStatefulsetConverter { +func NewAppWorkloadToStatefulsetConverter(scheme *runtime.Scheme) *AppWorkloadToStatefulsetConverter { return &AppWorkloadToStatefulsetConverter{ scheme: scheme, - statefulsetRunnerTemporarySetPodSeccompProfile: statefulsetRunnerTemporarySetPodSeccompProfile, } } @@ -147,6 +142,9 @@ func (r *AppWorkloadToStatefulsetConverter) Convert(appWorkload *korifiv1alpha1. ImagePullSecrets: appWorkload.Spec.ImagePullSecrets, SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: tools.PtrTo(true), + SeccompProfile: &corev1.SeccompProfile{ + Type: corev1.SeccompProfileTypeRuntimeDefault, + }, }, ServiceAccountName: ServiceAccountName, }, @@ -154,12 +152,6 @@ func (r *AppWorkloadToStatefulsetConverter) Convert(appWorkload *korifiv1alpha1. }, } - if r.statefulsetRunnerTemporarySetPodSeccompProfile { - statefulSet.Spec.Template.Spec.SecurityContext.SeccompProfile = &corev1.SeccompProfile{ - Type: corev1.SeccompProfileTypeRuntimeDefault, - } - } - statefulSet.Spec.Template.Spec.AutomountServiceAccountToken = tools.PtrTo(false) statefulSet.Spec.Selector = statefulSetLabelSelector(appWorkload) diff --git a/statefulset-runner/controllers/appworkload_to_stset_test.go b/statefulset-runner/controllers/appworkload_to_stset_test.go index e3bdce07c..cf9efa261 100644 --- a/statefulset-runner/controllers/appworkload_to_stset_test.go +++ b/statefulset-runner/controllers/appworkload_to_stset_test.go @@ -19,10 +19,9 @@ import ( var _ = Describe("AppWorkload to StatefulSet Converter", func() { var ( - statefulSet *appsv1.StatefulSet - appWorkload *korifiv1alpha1.AppWorkload - converter *controllers.AppWorkloadToStatefulsetConverter - statefulsetRunnerTemporarySetPodSeccompProfile bool + statefulSet *appsv1.StatefulSet + appWorkload *korifiv1alpha1.AppWorkload + converter *controllers.AppWorkloadToStatefulsetConverter ) BeforeEach(func() { @@ -85,15 +84,11 @@ var _ = Describe("AppWorkload to StatefulSet Converter", func() { }, } - statefulsetRunnerTemporarySetPodSeccompProfile = false + converter = controllers.NewAppWorkloadToStatefulsetConverter(scheme.Scheme) }) JustBeforeEach(func() { var err error - converter = controllers.NewAppWorkloadToStatefulsetConverter( - scheme.Scheme, - statefulsetRunnerTemporarySetPodSeccompProfile, - ) statefulSet, err = converter.Convert(appWorkload) Expect(err).NotTo(HaveOccurred()) @@ -270,6 +265,11 @@ var _ = Describe("AppWorkload to StatefulSet Converter", func() { Expect(*statefulSet.Spec.Template.Spec.SecurityContext.RunAsNonRoot).To(BeTrue()) }) + It("should set the seccomp profile on the pod", func() { + Expect(statefulSet.Spec.Template.Spec.SecurityContext.SeccompProfile).NotTo(BeNil()) + Expect(*statefulSet.Spec.Template.Spec.SecurityContext.SeccompProfile).To(Equal(corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault})) + }) + It("should set topology spread constraint", func() { topologySpreadConstraints := statefulSet.Spec.Template.Spec.TopologySpreadConstraints Expect(topologySpreadConstraints).To(HaveLen(2)) @@ -375,18 +375,4 @@ var _ = Describe("AppWorkload to StatefulSet Converter", func() { }) } }) - - It("does not set spec.securityContext.seccompProfile", func() { - Expect(statefulSet.Spec.Template.Spec.SecurityContext.SeccompProfile).To(BeNil()) - }) - - When("statefulsetRunnerTemporarySetPodSeccompProfile is set to true", func() { - BeforeEach(func() { - statefulsetRunnerTemporarySetPodSeccompProfile = true - }) - - It("sets spec.securityContext.seccompProfile to RuntimeDefault", func() { - Expect(statefulSet.Spec.Template.Spec.SecurityContext.SeccompProfile.Type).To(Equal(corev1.SeccompProfileTypeRuntimeDefault)) - }) - }) }) diff --git a/statefulset-runner/controllers/integration/suite_test.go b/statefulset-runner/controllers/integration/suite_test.go index fd3a0b196..20fa3b5d6 100644 --- a/statefulset-runner/controllers/integration/suite_test.go +++ b/statefulset-runner/controllers/integration/suite_test.go @@ -74,7 +74,7 @@ var _ = BeforeSuite(func() { appWorkloadReconciler := NewAppWorkloadReconciler( k8sManager.GetClient(), k8sManager.GetScheme(), - NewAppWorkloadToStatefulsetConverter(k8sManager.GetScheme(), false), + NewAppWorkloadToStatefulsetConverter(k8sManager.GetScheme()), NewPDBUpdater(k8sManager.GetClient()), ctrl.Log.WithName("statefulset-runner").WithName("AppWorkload"), )