diff --git a/pkg/library/overrides/containers.go b/pkg/library/overrides/containers.go index 40ad0d3ca..326cd5410 100644 --- a/pkg/library/overrides/containers.go +++ b/pkg/library/overrides/containers.go @@ -33,19 +33,18 @@ func ApplyContainerOverrides(component *dw.Component, container *corev1.Containe if err := component.Attributes.GetInto(constants.ContainerOverridesAttribute, override); err != nil { return nil, fmt.Errorf("failed to parse %s attribute on component %s: %w", constants.ContainerOverridesAttribute, component.Name, err) } - override = restrictContainerOverride(override) - - overrideBytes, err := json.Marshal(override) - if err != nil { - return nil, fmt.Errorf("error applying container overrides: %w", err) + if err := restrictContainerOverride(override); err != nil { + return nil, fmt.Errorf("failed to parse %s attribute on component %s: %w", constants.ContainerOverridesAttribute, component.Name, err) } + overrideJSON := component.Attributes[constants.ContainerOverridesAttribute] + originalBytes, err := json.Marshal(container) if err != nil { return nil, fmt.Errorf("failed to marshal container to yaml: %w", err) } - patchedBytes, err := strategicpatch.StrategicMergePatch(originalBytes, overrideBytes, &corev1.Container{}) + patchedBytes, err := strategicpatch.StrategicMergePatch(originalBytes, overrideJSON.Raw, &corev1.Container{}) if err != nil { return nil, fmt.Errorf("failed to apply container overrides: %w", err) } @@ -54,24 +53,41 @@ func ApplyContainerOverrides(component *dw.Component, container *corev1.Containe if err := json.Unmarshal(patchedBytes, patched); err != nil { return nil, fmt.Errorf("error applying container overrides: %w", err) } - // Applying the patch will overwrite the container's name as corev1.Container.Name + // Applying the patch will overwrite the container's name and image as corev1.Container.Name // does not have the omitempty json tag. patched.Name = container.Name + patched.Image = container.Image return patched, nil } // restrictContainerOverride unsets fields on a container that should not be // considered for container overrides. These fields are generally available to // set as fields on the container component itself. -func restrictContainerOverride(override *corev1.Container) *corev1.Container { - result := override.DeepCopy() - result.Name = "" - result.Image = "" - result.Command = nil - result.Args = nil - result.Ports = nil - result.VolumeMounts = nil - result.Env = nil - - return result +func restrictContainerOverride(override *corev1.Container) error { + invalidField := "" + if override.Name != "" { + invalidField = "name" + } + if override.Image != "" { + invalidField = "image" + } + if override.Command != nil { + invalidField = "command" + } + if override.Args != nil { + invalidField = "args" + } + if override.Ports != nil { + invalidField = "ports" + } + if override.VolumeMounts != nil { + invalidField = "volumeMounts" + } + if override.Env != nil { + invalidField = "env" + } + if invalidField != "" { + return fmt.Errorf("cannot use container-overrides to override container %s", invalidField) + } + return nil } diff --git a/pkg/library/overrides/pods.go b/pkg/library/overrides/pods.go index 09701435f..a6f8af236 100644 --- a/pkg/library/overrides/pods.go +++ b/pkg/library/overrides/pods.go @@ -18,6 +18,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/strategicpatch" @@ -50,17 +51,15 @@ func ApplyPodOverrides(workspace *common.DevWorkspaceWithConfig, deployment *app // will be interpreted as "delete all containers" as the serialized patch will include "containers": null. // To avoid this, save the original containers and reset them at the end. originalContainers := patched.Spec.Template.Spec.Containers + // Save fields we do not allow to be configured in pod-overrides + originalInitContainers := patched.Spec.Template.Spec.InitContainers + originalVolumes := patched.Spec.Template.Spec.Volumes patchedTemplateBytes, err := json.Marshal(patched.Spec.Template) if err != nil { return nil, fmt.Errorf("failed to marshal deployment to yaml: %w", err) } for _, override := range overrides { - patchBytes, err := json.Marshal(override) - if err != nil { - return nil, fmt.Errorf("error applying pod overrides: %w", err) - } - - patchedTemplateBytes, err = strategicpatch.StrategicMergePatch(patchedTemplateBytes, patchBytes, &corev1.PodTemplateSpec{}) + patchedTemplateBytes, err = strategicpatch.StrategicMergePatch(patchedTemplateBytes, override.Raw, &corev1.PodTemplateSpec{}) if err != nil { return nil, fmt.Errorf("error applying pod overrides: %w", err) } @@ -72,6 +71,8 @@ func ApplyPodOverrides(workspace *common.DevWorkspaceWithConfig, deployment *app } patched.Spec.Template = patchedPodSpecTemplate patched.Spec.Template.Spec.Containers = originalContainers + patched.Spec.Template.Spec.InitContainers = originalInitContainers + patched.Spec.Template.Spec.Volumes = originalVolumes return patched, nil } @@ -79,21 +80,28 @@ func ApplyPodOverrides(workspace *common.DevWorkspaceWithConfig, deployment *app // present in the DevWorkspace. The order of elements is // 1. Pod overrides defined on Container components, in the order they appear in the DevWorkspace // 2. Pod overrides defined in the global attributes field (.spec.template.attributes) -func getPodOverrides(workspace *common.DevWorkspaceWithConfig) ([]corev1.PodTemplateSpec, error) { - var allOverrides []corev1.PodTemplateSpec +func getPodOverrides(workspace *common.DevWorkspaceWithConfig) ([]apiext.JSON, error) { + var allOverrides []apiext.JSON for _, component := range workspace.Spec.Template.Components { if component.Attributes.Exists(constants.PodOverridesAttribute) { override := corev1.PodTemplateSpec{} - err := component.Attributes.GetInto(constants.PodOverridesAttribute, &override) - if err != nil { + // Check format of pod-overrides to detect errors early + if err := component.Attributes.GetInto(constants.PodOverridesAttribute, &override); err != nil { return nil, fmt.Errorf("failed to parse %s attribute on component %s: %w", constants.PodOverridesAttribute, component.Name, err) } - // Do not allow overriding containers - override.Spec.Containers = nil - override.Spec.InitContainers = nil - override.Spec.Volumes = nil - allOverrides = append(allOverrides, override) + // Do not allow overriding containers or volumes + if override.Spec.Containers != nil { + return nil, fmt.Errorf("cannot use pod-overrides to override pod containers (component %s)", component.Name) + } + if override.Spec.InitContainers != nil { + return nil, fmt.Errorf("cannot use pod-overrides to override pod initContainers (component %s)", component.Name) + } + if override.Spec.Volumes != nil { + return nil, fmt.Errorf("cannot use pod-overrides to override pod volumes (component %s)", component.Name) + } + patchData := component.Attributes[constants.PodOverridesAttribute] + allOverrides = append(allOverrides, patchData) } } if workspace.Spec.Template.Attributes.Exists(constants.PodOverridesAttribute) { @@ -103,10 +111,17 @@ func getPodOverrides(workspace *common.DevWorkspaceWithConfig) ([]corev1.PodTemp return nil, fmt.Errorf("failed to parse %s attribute for workspace: %w", constants.PodOverridesAttribute, err) } // Do not allow overriding containers or volumes - override.Spec.Containers = nil - override.Spec.InitContainers = nil - override.Spec.Volumes = nil - allOverrides = append(allOverrides, override) + if override.Spec.Containers != nil { + return nil, fmt.Errorf("cannot use pod-overrides to override pod containers") + } + if override.Spec.InitContainers != nil { + return nil, fmt.Errorf("cannot use pod-overrides to override pod initContainers") + } + if override.Spec.Volumes != nil { + return nil, fmt.Errorf("cannot use pod-overrides to override pod volumes") + } + patchData := workspace.Spec.Template.Attributes[constants.PodOverridesAttribute] + allOverrides = append(allOverrides, patchData) } return allOverrides, nil } diff --git a/pkg/library/overrides/testdata/container-overrides/container-cannot-set-restricted-fields.yaml b/pkg/library/overrides/testdata/container-overrides/container-cannot-set-restricted-fields.yaml index ac308c2df..a95725786 100644 --- a/pkg/library/overrides/testdata/container-overrides/container-cannot-set-restricted-fields.yaml +++ b/pkg/library/overrides/testdata/container-overrides/container-cannot-set-restricted-fields.yaml @@ -36,17 +36,4 @@ input: output: - container: - name: test-component - image: test-image - command: ["original"] - args: ["original"] - ports: - - name: original-port - containerPort: 8080 - volumeMounts: - - name: original-volume - mountPath: original-mountPath - env: - - name: original_env - value: original_val + errRegexp: "cannot use container-overrides to override container env" diff --git a/pkg/library/overrides/testdata/container-overrides/overrides-can-override-securityContext.yaml b/pkg/library/overrides/testdata/container-overrides/overrides-can-override-securityContext.yaml new file mode 100644 index 000000000..562c20a05 --- /dev/null +++ b/pkg/library/overrides/testdata/container-overrides/overrides-can-override-securityContext.yaml @@ -0,0 +1,26 @@ +name: "container overrides can override securityContext" + +input: + component: + name: test-component + attributes: + container-overrides: + securityContext: + runAsUser: 1001 + container: + image: test-image + + container: + name: test-component + image: test-image + securityContext: + runAsUser: 1000 + runAsGroup: 2000 + +output: + container: + name: test-component + image: test-image + securityContext: + runAsUser: 1001 + runAsGroup: 2000 diff --git a/pkg/library/overrides/testdata/container-overrides/overrides-can-use-delete-directive.yaml b/pkg/library/overrides/testdata/container-overrides/overrides-can-use-delete-directive.yaml new file mode 100644 index 000000000..1b3dcbcd0 --- /dev/null +++ b/pkg/library/overrides/testdata/container-overrides/overrides-can-use-delete-directive.yaml @@ -0,0 +1,24 @@ +name: "container overrides can use $patch: delete to delete fields rather than merging" + +input: + component: + name: test-component + attributes: + container-overrides: + securityContext: + $patch: delete + container: + image: test-image + + container: + name: test-component + image: test-image + securityContext: + runAsUser: 1000 + runAsGroup: 2000 + +output: + container: + name: test-component + image: test-image + securityContext: {} diff --git a/pkg/library/overrides/testdata/container-overrides/overrides-can-use-replace-directive.yaml b/pkg/library/overrides/testdata/container-overrides/overrides-can-use-replace-directive.yaml new file mode 100644 index 000000000..0188f336f --- /dev/null +++ b/pkg/library/overrides/testdata/container-overrides/overrides-can-use-replace-directive.yaml @@ -0,0 +1,26 @@ +name: "container overrides can use $patch: replace to overwrite fields rather than merging" + +input: + component: + name: test-component + attributes: + container-overrides: + securityContext: + runAsUser: 1001 + $patch: replace + container: + image: test-image + + container: + name: test-component + image: test-image + securityContext: + runAsUser: 1000 + runAsGroup: 2000 + +output: + container: + name: test-component + image: test-image + securityContext: + runAsUser: 1001 diff --git a/pkg/library/overrides/testdata/pod-overrides/error_cannot-override-volumes.yaml b/pkg/library/overrides/testdata/pod-overrides/error_cannot-override-volumes.yaml new file mode 100644 index 000000000..881a28a95 --- /dev/null +++ b/pkg/library/overrides/testdata/pod-overrides/error_cannot-override-volumes.yaml @@ -0,0 +1,25 @@ +name: "Returns error when attempting to patch pod volumes" + +input: + workspace: + attributes: + pod-overrides: + spec: + volumes: + - name: test-volume + components: + - name: test-component + container: + image: test-image + + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + +output: + errRegexp: "cannot use pod-overrides to override pod volumes" diff --git a/pkg/library/overrides/testdata/pod-overrides/overrides-can-override-securityContext.yaml b/pkg/library/overrides/testdata/pod-overrides/overrides-can-override-securityContext.yaml new file mode 100644 index 000000000..ede06d4f4 --- /dev/null +++ b/pkg/library/overrides/testdata/pod-overrides/overrides-can-override-securityContext.yaml @@ -0,0 +1,41 @@ +name: "Pod overrides can override securityContext" + +input: + workspace: + attributes: + pod-overrides: + spec: + securityContext: + runAsUser: 1001 + components: + - name: test-component + container: + image: test-image + + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + securityContext: + fsGroup: 2000 + runAsGroup: 3000 + runAsUser: 1000 + + +output: + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + securityContext: + fsGroup: 2000 + runAsGroup: 3000 + runAsUser: 1001 diff --git a/pkg/library/overrides/testdata/pod-overrides/overrides-can-use-delete-directive.yaml b/pkg/library/overrides/testdata/pod-overrides/overrides-can-use-delete-directive.yaml new file mode 100644 index 000000000..22182b5d7 --- /dev/null +++ b/pkg/library/overrides/testdata/pod-overrides/overrides-can-use-delete-directive.yaml @@ -0,0 +1,38 @@ +name: "Pod overrides can use $patch: delete to delete fields rather than merging" + +input: + workspace: + attributes: + pod-overrides: + spec: + securityContext: + $patch: delete + components: + - name: test-component + container: + image: test-image + + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + securityContext: + fsGroup: 2000 + runAsGroup: 3000 + runAsUser: 1000 + + +output: + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + securityContext: {} diff --git a/pkg/library/overrides/testdata/pod-overrides/overrides-can-use-replace-directive.yaml b/pkg/library/overrides/testdata/pod-overrides/overrides-can-use-replace-directive.yaml new file mode 100644 index 000000000..cf50127a9 --- /dev/null +++ b/pkg/library/overrides/testdata/pod-overrides/overrides-can-use-replace-directive.yaml @@ -0,0 +1,40 @@ +name: "Pod overrides can use $patch: replace to overwrite fields rather than merging" + +input: + workspace: + attributes: + pod-overrides: + spec: + securityContext: + runAsUser: 1001 + $patch: replace + components: + - name: test-component + container: + image: test-image + + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + securityContext: + fsGroup: 2000 + runAsGroup: 3000 + runAsUser: 1000 + + +output: + podTemplateSpec: + metadata: + labels: + controller.devfile.io/devworkspace-id: test-id + spec: + containers: + - name: test-component + image: test-image + securityContext: + runAsUser: 1001