diff --git a/pkg/library/flatten/flatten.go b/pkg/library/flatten/flatten.go index 02d938f11..a4f26cc05 100644 --- a/pkg/library/flatten/flatten.go +++ b/pkg/library/flatten/flatten.go @@ -68,6 +68,11 @@ func ResolveDevWorkspace(workspace *dw.DevWorkspaceTemplateSpec, tooling Resolve return resolvedDW, &warnings, nil } + err = resolveWorkspaceEnvVar(resolvedDW) + if err != nil { + return nil, nil, fmt.Errorf("failed to process workspaceEnv: %w", err) + } + return resolvedDW, nil, nil } diff --git a/pkg/library/flatten/flatten_test.go b/pkg/library/flatten/flatten_test.go index 938027ead..293f59147 100644 --- a/pkg/library/flatten/flatten_test.go +++ b/pkg/library/flatten/flatten_test.go @@ -28,15 +28,7 @@ func TestResolveDevWorkspaceKubernetesReference(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - WorkspaceNamespace: "test-ignored", - K8sClient: testClient, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -59,14 +51,8 @@ func TestResolveDevWorkspaceInternalRegistry(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testRegistry := &testutil.FakeInternalRegistry{ - Plugins: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - InternalRegistry: testRegistry, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -89,15 +75,8 @@ func TestResolveDevWorkspacePluginRegistry(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -120,15 +99,8 @@ func TestResolveDevWorkspacePluginURI(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -150,21 +122,8 @@ func TestResolveDevWorkspaceParents(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - WorkspaceNamespace: "test-ignored", - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -189,20 +148,9 @@ func TestResolveDevWorkspaceMissingDefaults(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "") + testResolverTools.InternalRegistry = nil + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -224,21 +172,8 @@ func TestResolveDevWorkspaceAnnotations(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines devworkspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - WorkspaceNamespace: "default-namespace", - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -260,21 +195,31 @@ func TestResolveDevWorkspaceTemplateNamespaceRestriction(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines devworkspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - WorkspaceNamespace: "test-namespace", + testResolverTools := getTestingTools(tt.Input, "test-namespace") + + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) + if tt.Output.ErrRegexp != nil && assert.Error(t, err) { + assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") + } else { + if !assert.NoError(t, err, "Should not return error") { + return + } + assert.Truef(t, cmp.Equal(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts), + "DevWorkspace should match expected output:\n%s", + cmp.Diff(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts)) } + }) + } +} + +func TestResolveDevWorkspaceWorkspaceEnv(t *testing.T) { + tests := testutil.LoadAllTestsOrPanic(t, "testdata/workspace-env") + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + // sanity check: input defines components + assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines devworkspace with no components") + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -289,3 +234,27 @@ func TestResolveDevWorkspaceTemplateNamespaceRestriction(t *testing.T) { }) } } + +func getTestingTools(input testutil.TestInput, testNamespace string) ResolverTools { + testHttpGetter := &testutil.FakeHTTPGetter{ + DevfileResources: input.DevfileResources, + DevWorkspaceResources: input.DevWorkspaceResources, + Errors: input.Errors, + } + testK8sClient := &testutil.FakeK8sClient{ + DevWorkspaceResources: input.DevWorkspaceResources, + Errors: input.Errors, + } + testRegistry := &testutil.FakeInternalRegistry{ + Plugins: input.DevWorkspaceResources, + + Errors: input.Errors, + } + return ResolverTools{ + Context: context.Background(), + InternalRegistry: testRegistry, + K8sClient: testK8sClient, + HttpClient: testHttpGetter, + WorkspaceNamespace: testNamespace, + } +} diff --git a/pkg/library/flatten/helper.go b/pkg/library/flatten/helper.go index 75f3d05e8..206a4f106 100644 --- a/pkg/library/flatten/helper.go +++ b/pkg/library/flatten/helper.go @@ -17,6 +17,7 @@ import ( "reflect" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/constants" ) // resolutionContextTree is a recursive structure representing information about the devworkspace that is @@ -64,3 +65,15 @@ func formatImportCycle(end *resolutionContextTree) string { } return cycle } + +func getOriginalNameForComponent(component dw.Component) string { + if component.Attributes.Exists(constants.PluginSourceAttribute) { + var err error + componentName := component.Attributes.GetString(constants.PluginSourceAttribute, &err) + if err != nil { + return component.Name + } + return componentName + } + return component.Name +} diff --git a/pkg/library/flatten/internal/testutil/common.go b/pkg/library/flatten/internal/testutil/common.go index af59c77b0..0568d6946 100644 --- a/pkg/library/flatten/internal/testutil/common.go +++ b/pkg/library/flatten/internal/testutil/common.go @@ -34,6 +34,9 @@ var WorkspaceTemplateDiffOpts = cmp.Options{ cmpopts.SortSlices(func(a, b string) bool { return strings.Compare(a, b) > 0 }), + cmpopts.SortSlices(func(a, b dw.EnvVar) bool { + return strings.Compare(a.Name, b.Name) > 0 + }), // TODO: Devworkspace overriding results in empty []string instead of nil cmpopts.IgnoreFields(dw.DevWorkspaceEvents{}, "PostStart", "PreStop", "PostStop"), } diff --git a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml new file mode 100644 index 000000000..b1097a361 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml @@ -0,0 +1,80 @@ +name: "Adds workspace environment variables from plugin" + +input: + devworkspace: + components: + + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + + - name: regular-container + container: + image: test-image-regular + + - name: init-container + container: + image: test-image-init + + commands: + - id: make-init + apply: + component: init-container + + events: + prestart: [make-init] + + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + workspaceEnv: + TEST_ENV_1: TEST_VAL_1 + TEST_ENV_2: TEST_VAL_2 + container: + name: test-container + image: test-image + +output: + devworkspace: + components: + - name: plugin-a + attributes: + controller.devfile.io/imported-by: test-plugin + workspaceEnv: + TEST_ENV_1: TEST_VAL_1 + TEST_ENV_2: TEST_VAL_2 + container: + name: test-container + image: test-image + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: regular-container + container: + image: test-image-regular + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: init-container + container: + image: test-image-init + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + commands: + - id: make-init + apply: + component: init-container + events: + prestart: [make-init] diff --git a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml new file mode 100644 index 000000000..85c277f06 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml @@ -0,0 +1,84 @@ +name: "Adds JSON workspace environment variables from plugin " + +input: + devworkspace: + components: + + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + + - name: regular-container + container: + image: test-image-regular + + - name: init-container + container: + image: test-image-init + + commands: + - id: make-init + apply: + component: init-container + + events: + prestart: [make-init] + + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + workspaceEnv: + { + "TEST_ENV_1": "TEST_VAL_1", + "TEST_ENV_2": "TEST_VAL_2", + } + container: + name: test-container + image: test-image + +output: + devworkspace: + components: + - name: plugin-a + attributes: + controller.devfile.io/imported-by: test-plugin + workspaceEnv: + { + "TEST_ENV_1": "TEST_VAL_1", + "TEST_ENV_2": "TEST_VAL_2", + } + container: + name: test-container + image: test-image + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: regular-container + container: + image: test-image-regular + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: init-container + container: + image: test-image-init + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + commands: + - id: make-init + apply: + component: init-container + events: + prestart: [make-init] diff --git a/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml b/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml new file mode 100644 index 000000000..9dfd56aa5 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml @@ -0,0 +1,31 @@ +name: "Returns an error when there are conflicting definitions of workspace env var" + +input: + devworkspace: + components: + - name: test-container + attributes: + workspaceEnv: + TEST_ENV_2: NOT_TEST_VAL_2 + container: + image: test-image + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + workspaceEnv: + TEST_ENV_1: TEST_VAL_1 + TEST_ENV_2: TEST_VAL_2 + container: + name: test-container + image: test-image + +output: + errRegexp: "failed to process workspaceEnv: conflicting definition of environment variable TEST_ENV_2 in components 'test-plugin' and 'test-container'" diff --git a/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml b/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml new file mode 100644 index 000000000..54fe3aaf9 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml @@ -0,0 +1,29 @@ +name: "Returns an error when workspaceEnv cannot be read as []EnvVar" + +input: + devworkspace: + components: + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + # Note this fails to deserialize because it's just a yaml string, not a list. + # JSON can be used here if you remove the pipe + workspaceEnv: | + { + "TEST_ENV_1": "TEST_ENV_1", + "TEST_ENV_2": "TEST_ENV_2", + } + container: + name: test-container + image: test-image + +output: + errRegexp: "failed to read attribute workspaceEnv on component test-plugin" diff --git a/pkg/library/flatten/workspaceEnv.go b/pkg/library/flatten/workspaceEnv.go new file mode 100644 index 000000000..4b09ecd44 --- /dev/null +++ b/pkg/library/flatten/workspaceEnv.go @@ -0,0 +1,82 @@ +// +// Copyright (c) 2019-2021 Red Hat, Inc. +// This program and the accompanying materials are made +// available under the terms of the Eclipse Public License 2.0 +// which is available at https://www.eclipse.org/legal/epl-2.0/ +// +// SPDX-License-Identifier: EPL-2.0 +// +// Contributors: +// Red Hat, Inc. - initial API and implementation +// + +package flatten + +import ( + "fmt" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" +) + +const ( + // WorkspaceEnvAttribute is an attribute that specifies a set of environment variables provided by a component + // that should be added to all workspace containers. The structure of the attribute value should be a map of strings + // to strings, e.g. + // + // attributes: + // workspaceEnv: + // - name: ENV_1 + // value: VAL_1 + // - name: ENV_2 + // value: VAL_2 + WorkspaceEnvAttribute = "workspaceEnv" +) + +func resolveWorkspaceEnvVar(flattenedDW *dw.DevWorkspaceTemplateSpec) error { + workspaceEnv, err := collectWorkspaceEnv(flattenedDW) + if err != nil { + return err + } + + for idx, component := range flattenedDW.Components { + if component.Container != nil { + flattenedDW.Components[idx].Container.Env = append(component.Container.Env, workspaceEnv...) + } + } + + return nil +} + +func collectWorkspaceEnv(flattenedDW *dw.DevWorkspaceTemplateSpec) ([]dw.EnvVar, error) { + workspaceEnvMap := map[string]string{} + + // Bookkeeping map so that we can format error messages in case of conflict + envVarToComponent := map[string]string{} + + for _, component := range flattenedDW.Components { + if !component.Attributes.Exists(WorkspaceEnvAttribute) { + continue + } + + componentEnvMap := map[string]string{} + err := component.Attributes.GetInto(WorkspaceEnvAttribute, &componentEnvMap) + if err != nil { + return nil, fmt.Errorf("failed to read attribute %s on component %s: %w", WorkspaceEnvAttribute, getOriginalNameForComponent(component), err) + } + + for name, value := range componentEnvMap { + if existingVal, exists := workspaceEnvMap[name]; exists && existingVal != value { + return nil, fmt.Errorf("conflicting definition of environment variable %s in components '%s' and '%s'", + name, envVarToComponent[name], component.Name) + } + workspaceEnvMap[name] = value + envVarToComponent[name] = getOriginalNameForComponent(component) + } + } + + var workspaceEnv []dw.EnvVar + for name, value := range workspaceEnvMap { + workspaceEnv = append(workspaceEnv, dw.EnvVar{Name: name, Value: value}) + } + return workspaceEnv, nil +}