Add support for containerContributions

pkg/constants/attributes.go

@@ -89,4 +89,18 @@ const (
 	// EndpointURLAttribute is an attribute added to endpoints to denote the endpoint on the cluster that
 	// was created to route to this endpoint
 	EndpointURLAttribute = "controller.devfile.io/endpoint-url"
+
+	// ContainerContributionAttribute defines a container component as a container contribution that should be merged
+	// into an existing container in the devfile if possible. If no suitable container exists, this component
+	// is treated as a regular container component
+	ContainerContributionAttribute = "controller.devfile.io/container-contribution"
+
+	// MergeContributionAttribute defines a container component as a target for merging a container contribution. If
+	// present on a container component, any container contributions will be merged into that container. If multiple
+	// container components have the merge-contribution attribute, the first one will be used and all others ignored.
+	MergeContributionAttribute = "controller.devfile.io/merge-contribution"
+
+	// MergedContributionsAttribute is applied as an attribute onto a component to list the components from the unflattened
+	// DevWorkspace that have been merged into the current component. The contributions are listed in a comma-separated list.
+	MergedContributionsAttribute = "controller.devfile.io/merged-contributions"
 )

pkg/library/flatten/flatten.go

@@ -64,6 +64,14 @@ func ResolveDevWorkspace(workspace *dw.DevWorkspaceTemplateSpec, tooling Resolve
 		return resolvedDW, &warnings, nil
 	}
 
+	if needsMerge, err := needsContainerContributionMerge(resolvedDW); needsMerge {
+		if err := mergeContainerContributions(resolvedDW); err != nil {
+			return nil, nil, err
+		}
+	} else if err != nil {
+		return nil, nil, err
+	}
+
 	return resolvedDW, nil, nil
 }
 

pkg/library/flatten/flatten_test.go

@@ -211,6 +211,29 @@ func TestMergesDuplicateVolumeComponents(t *testing.T) {
 	}
 }
 
+func TestMergeContainerContributions(t *testing.T) {
+	tests := testutil.LoadAllTestsOrPanic(t, "testdata/container-contributions")
+	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 workspace 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")
+			} 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 getTestingTools(input testutil.TestInput, testNamespace string) ResolverTools {
 	testHttpGetter := &testutil.FakeHTTPGetter{
 		DevfileResources:      input.DevfileResources,

pkg/library/flatten/helper.go

@@ -19,6 +19,9 @@ import (
 	"fmt"
 	"reflect"
 
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+
 	dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
 )
 
@@ -67,3 +70,100 @@ func formatImportCycle(end *resolutionContextTree) string {
 	}
 	return cycle
 }
+
+func parseResourcesFromComponent(component *dw.Component) (*corev1.ResourceRequirements, error) {
+	if component.Container == nil {
+		return nil, fmt.Errorf("attemped to parse resource requirements from a non-container component")
+	}
+	memLimitStr := component.Container.MemoryLimit
+	if memLimitStr == "" {
+		memLimitStr = "0Mi"
+	}
+	memRequestStr := component.Container.MemoryRequest
+	if memRequestStr == "" {
+		memRequestStr = "0Mi"
+	}
+	cpuLimitStr := component.Container.CpuLimit
+	if cpuLimitStr == "" {
+		cpuLimitStr = "0m"
+	}
+	cpuRequestStr := component.Container.CpuRequest
+	if cpuRequestStr == "" {
+		cpuRequestStr = "0m"
+	}
+
+	memoryLimit, err := resource.ParseQuantity(memLimitStr)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse memory limit for container component %s: %w", component.Name, err)
+	}
+	memoryRequest, err := resource.ParseQuantity(memRequestStr)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse memory request for container component %s: %w", component.Name, err)
+	}
+	cpuLimit, err := resource.ParseQuantity(cpuLimitStr)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse CPU limit for container component %s: %w", component.Name, err)
+	}
+	cpuRequest, err := resource.ParseQuantity(cpuRequestStr)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse CPU request for container component %s: %w", component.Name, err)
+	}
+
+	return &corev1.ResourceRequirements{
+		Limits: corev1.ResourceList{
+			corev1.ResourceMemory: memoryLimit,
+			corev1.ResourceCPU:    cpuLimit,
+		},
+		Requests: corev1.ResourceList{
+			corev1.ResourceMemory: memoryRequest,
+			corev1.ResourceCPU:    cpuRequest,
+		},
+	}, nil
+}
+
+func addResourceRequirements(resources *corev1.ResourceRequirements, toAdd *dw.Component) error {
+	componentResources, err := parseResourcesFromComponent(toAdd)
+	if err != nil {
+		return err
+	}
+
+	memoryLimit := resources.Limits[corev1.ResourceMemory]
+	memoryLimit.Add(componentResources.Limits[corev1.ResourceMemory])
+	resources.Limits[corev1.ResourceMemory] = memoryLimit
+
+	cpuLimit := resources.Limits[corev1.ResourceCPU]
+	cpuLimit.Add(componentResources.Limits[corev1.ResourceCPU])
+	resources.Limits[corev1.ResourceCPU] = cpuLimit
+
+	memoryRequest := resources.Requests[corev1.ResourceMemory]
+	memoryRequest.Add(componentResources.Requests[corev1.ResourceMemory])
+	resources.Requests[corev1.ResourceMemory] = memoryRequest
+
+	cpuRequest := resources.Requests[corev1.ResourceCPU]
+	cpuRequest.Add(componentResources.Requests[corev1.ResourceCPU])
+	resources.Requests[corev1.ResourceCPU] = cpuRequest
+
+	return nil
+}
+
+func applyResourceRequirementsToComponent(container *dw.ContainerComponent, resources *corev1.ResourceRequirements) {
+	memLimit := resources.Limits[corev1.ResourceMemory]
+	if !memLimit.IsZero() {
+		container.MemoryLimit = memLimit.String()
+	}
+
+	cpuLimit := resources.Limits[corev1.ResourceCPU]
+	if !cpuLimit.IsZero() {
+		container.CpuLimit = cpuLimit.String()
+	}
+
+	memRequest := resources.Requests[corev1.ResourceMemory]
+	if !memRequest.IsZero() {
+		container.MemoryRequest = memRequest.String()
+	}
+
+	cpuRequest := resources.Requests[corev1.ResourceCPU]
+	if !cpuRequest.IsZero() {
+		container.CpuRequest = cpuRequest.String()
+	}
+}

pkg/library/flatten/internal/testutil/common.go

@@ -37,6 +37,12 @@ var WorkspaceTemplateDiffOpts = cmp.Options{
 	cmpopts.SortSlices(func(a, b dw.EnvVar) bool {
 		return strings.Compare(a.Name, b.Name) > 0
 	}),
+	cmpopts.SortSlices(func(a, b dw.Endpoint) bool {
+		return strings.Compare(a.Name, b.Name) > 0
+	}),
+	cmpopts.SortSlices(func(a, b dw.VolumeMount) 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"),
 }

pkg/library/flatten/merge.go

@@ -16,10 +16,14 @@
 package flatten
 
 import (
+	"encoding/json"
 	"fmt"
+	"strings"
 
 	dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
+	"github.com/devfile/api/v2/pkg/attributes"
 	"github.com/devfile/api/v2/pkg/utils/overriding"
+	"github.com/devfile/devworkspace-operator/pkg/constants"
 	"k8s.io/apimachinery/pkg/api/resource"
 )
 
@@ -118,3 +122,147 @@ func mergeVolume(into, from *dw.VolumeComponent) error {
 	}
 	return nil
 }
+
+// needsContainerContributionMerge returns whether merging container contributions is necessary for this workspace. Merging
+// is necessary if at least one component has the merge-contribution: true attribute and at least one component has the
+// container-contribution: true attribute. If either attribute is present but cannot be parsed as a bool, an error is returned.
+func needsContainerContributionMerge(flattenedSpec *dw.DevWorkspaceTemplateSpec) (bool, error) {
+	hasContribution, hasTarget := false, false
+	var errHolder error
+	for _, component := range flattenedSpec.Components {
+		if component.Container == nil {
+			// Ignore attribute on non-container components as it's not clear what this would mean
+			continue
+		}
+		// Need to check existence before value to avoid potential KeyNotFoundError
+		if component.Attributes.Exists(constants.ContainerContributionAttribute) {
+			if component.Attributes.GetBoolean(constants.ContainerContributionAttribute, &errHolder) {
+				hasContribution = true
+			}
+			if errHolder != nil {
+				// Don't include error in message as it will be propagated to user and is not very clear (references Go unmarshalling)
+				return false, fmt.Errorf("failed to parse %s attribute on component %s as true or false", constants.ContainerContributionAttribute, component.Name)
+			}
+		}
+		if component.Attributes.Exists(constants.MergeContributionAttribute) {
+			if component.Attributes.GetBoolean(constants.MergeContributionAttribute, &errHolder) {
+				hasTarget = true
+			}
+			if errHolder != nil {
+				return false, fmt.Errorf("failed to parse %s attribute on component %s as true or false", constants.MergeContributionAttribute, component.Name)
+			}
+		}
+	}
+	return hasContribution && hasTarget, nil
+}
+
+func mergeContainerContributions(flattenedSpec *dw.DevWorkspaceTemplateSpec) error {
+	var contributions []dw.Component
+	for _, component := range flattenedSpec.Components {
+		if component.Container != nil && component.Attributes.GetBoolean(constants.ContainerContributionAttribute, nil) {
+			contributions = append(contributions, component)
+		}
+	}
+
+	var newComponents []dw.Component
+	mergeDone := false
+	for _, component := range flattenedSpec.Components {
+		if component.Container == nil {
+			newComponents = append(newComponents, component)
+			continue
+		}
+		if component.Attributes.GetBoolean(constants.ContainerContributionAttribute, nil) {
+			// drop contributions from updated list as they will be merged
+			continue
+		} else if component.Attributes.GetBoolean(constants.MergeContributionAttribute, nil) && !mergeDone {
+			mergedComponent, err := mergeContributionsInto(&component, contributions)
+			if err != nil {
+				return fmt.Errorf("failed to merge container contributions: %w", err)
+			}
+			newComponents = append(newComponents, *mergedComponent)
+			mergeDone = true
+		} else {
+			newComponents = append(newComponents, component)
+		}
+	}
+
+	if mergeDone {
+		flattenedSpec.Components = newComponents
+	}
+
+	return nil
+}
+
+func mergeContributionsInto(mergeInto *dw.Component, contributions []dw.Component) (*dw.Component, error) {
+	if mergeInto == nil || mergeInto.Container == nil {
+		return nil, fmt.Errorf("attempting to merge container contributions into a non-container component")
+	}
+	totalResources, err := parseResourcesFromComponent(mergeInto)
+	if err != nil {
+		return nil, err
+	}
+
+	// We don't want to reimplement the complexity of a strategic merge here, so we set up a fake plugin override
+	// and use devfile/api overriding functionality. For specific fields that have to be handled specially (memory
+	// and cpu limits, we compute the value separately and set it at the end
+	var toMerge []dw.ComponentPluginOverride
+	// Store names of original plugins to allow us to generate the merged-contributions attribute
+	var mergedComponentNames []string
+	for _, component := range contributions {
+		if component.Container == nil {
+			return nil, fmt.Errorf("attempting to merge container contribution from a non-container component")
+		}
+		// Set name to match target component so that devfile/api override functionality will apply it correctly
+		component.Name = mergeInto.Name
+		// Unset image to avoid overriding the default image
+		component.Container.Image = ""
+		// Store original source attribute's value and remove from component
+		if component.Attributes.Exists(constants.PluginSourceAttribute) {
+			mergedComponentNames = append(mergedComponentNames, component.Attributes.GetString(constants.PluginSourceAttribute, nil))
+			delete(component.Attributes, constants.PluginSourceAttribute)
+		}
+		if err := addResourceRequirements(totalResources, &component); err != nil {
+			return nil, err
+		}
+		component.Container.MemoryLimit = ""
+		component.Container.MemoryRequest = ""
+		component.Container.CpuLimit = ""
+		component.Container.CpuRequest = ""
+		// Workaround to convert dw.Component into dw.ComponentPluginOverride: marshal to json, and unmarshal to a different type
+		// This works since plugin overrides are generated from components, with the difference being that all fields are optional
+		componentPluginOverride := dw.ComponentPluginOverride{}
+		tempJSONBytes, err := json.Marshal(component)
+		if err != nil {
+			return nil, err
+		}
+		if err := json.Unmarshal(tempJSONBytes, &componentPluginOverride); err != nil {
+			return nil, err
+		}
+		toMerge = append(toMerge, componentPluginOverride)
+	}
+
+	tempSpecContent := &dw.DevWorkspaceTemplateSpecContent{
+		Components: []dw.Component{
+			*mergeInto,
+		},
+	}
+
+	mergedSpecContent, err := overriding.OverrideDevWorkspaceTemplateSpec(tempSpecContent, dw.PluginOverrides{
+		Components: toMerge,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	mergedComponent := mergedSpecContent.Components[0]
+	applyResourceRequirementsToComponent(mergedComponent.Container, totalResources)
+
+	if mergedComponent.Attributes == nil {
+		mergedComponent.Attributes = attributes.Attributes{}
+	}
+	mergedComponent.Attributes.PutString(constants.MergedContributionsAttribute, strings.Join(mergedComponentNames, ","))
+	delete(mergedComponent.Attributes, constants.MergeContributionAttribute)
+	delete(mergedComponent.Attributes, constants.ContainerContributionAttribute)
+
+	return &mergedComponent, nil
+}

pkg/library/flatten/testdata/container-contributions/adds-resources.yaml

@@ -0,0 +1,46 @@
+name: "Adds attributes from contribution"
+
+input:
+  devworkspace:
+    components:
+      - name: test-component
+        attributes:
+          controller.devfile.io/merge-contribution: true
+        container:
+          image: test-image
+          memoryLimit: 1Gi
+          memoryRequest: 1000Mi
+          cpuLimit: 1500m
+          cpuRequest: "1"
+      - name: test-contribution
+        plugin:
+          uri: test-contribution.yaml
+
+  devfileResources:
+    test-contribution.yaml:
+      schemaVersion: 2.1.0
+      metadata:
+        name: test-contribution
+      components:
+      - name: test-contribution
+        attributes:
+          controller.devfile.io/container-contribution: true
+        container:
+          image: contribution-image
+          memoryLimit: 512Mi
+          memoryRequest: 1.5G
+          cpuLimit: "0.5"
+          cpuRequest: 500m
+
+output:
+  devworkspace:
+    components:
+      - name: test-component
+        attributes:
+          controller.devfile.io/merged-contributions: "test-contribution"
+        container:
+          image: test-image
+          memoryLimit: 1536Mi
+          memoryRequest: "2548576000" # 1.5G + 1000Mi = 1.5*1000^3 + 1000*1024^2
+          cpuLimit: "2"
+          cpuRequest: 1500m