Merge pull request #2010 from joelanford/fix/dep-resources

Bug 1926893: only override deployment resources when explicitly defined in subscription config

Author: openshift-merge-robot

Makefile

@@ -266,7 +266,7 @@ run-console-local:
 uninstall:
 	@echo Uninstalling OLM:
 	- kubectl delete -f deploy/upstream/quickstart/crds.yaml
-	- kubectl delete -f deploy/upstream/quickstart/olm.yam
+	- kubectl delete -f deploy/upstream/quickstart/olm.yaml
 	- kubectl delete catalogsources.operators.coreos.com
 	- kubectl delete clusterserviceversions.operators.coreos.com
 	- kubectl delete installplans.operators.coreos.com

go.mod

@@ -26,7 +26,7 @@ require (
 	github.com/onsi/gomega v1.10.2
 	github.com/openshift/api v0.0.0-20200331152225-585af27e34fd
 	github.com/openshift/client-go v0.0.0-20200326155132-2a6cd50aedd0
-	github.com/operator-framework/api v0.5.1
+	github.com/operator-framework/api v0.6.0
 	github.com/operator-framework/operator-registry v1.13.6
 	github.com/otiai10/copy v1.2.0
 	github.com/pkg/errors v0.9.1
@@ -41,18 +41,18 @@ require (
 	google.golang.org/grpc v1.30.0
 	gopkg.in/yaml.v2 v2.3.0
 	helm.sh/helm/v3 v3.1.0-rc.1.0.20201215141456-e71d38b414eb
-	k8s.io/api v0.20.0
-	k8s.io/apiextensions-apiserver v0.20.0
-	k8s.io/apimachinery v0.20.0
-	k8s.io/apiserver v0.20.0
-	k8s.io/client-go v0.20.0
-	k8s.io/code-generator v0.20.0
-	k8s.io/component-base v0.20.0
+	k8s.io/api v0.20.1
+	k8s.io/apiextensions-apiserver v0.20.1
+	k8s.io/apimachinery v0.20.1
+	k8s.io/apiserver v0.20.1
+	k8s.io/client-go v0.20.1
+	k8s.io/code-generator v0.20.1
+	k8s.io/component-base v0.20.1
 	k8s.io/klog v1.0.0
 	k8s.io/kube-aggregator v0.20.0
 	k8s.io/kube-openapi v0.0.0-20201113171705-d219536bb9fd
 	rsc.io/letsencrypt v0.0.3 // indirect
-	sigs.k8s.io/controller-runtime v0.7.0
+	sigs.k8s.io/controller-runtime v0.8.0
 	sigs.k8s.io/controller-tools v0.4.1
 	sigs.k8s.io/kind v0.7.0
 )

go.sum

@@ -16,7 +16,7 @@ type operatorConfig struct {
 	logger *logrus.Logger
 }
 
-func (o *operatorConfig) GetConfigOverrides(ownerCSV ownerutil.Owner) (envVarOverrides []corev1.EnvVar, volumeOverrides []corev1.Volume, volumeMountOverrides []corev1.VolumeMount, tolerationOverrides []corev1.Toleration, resourcesOverride corev1.ResourceRequirements, nodeSelectorOverride map[string]string, err error) {
+func (o *operatorConfig) GetConfigOverrides(ownerCSV ownerutil.Owner) (envVarOverrides []corev1.EnvVar, volumeOverrides []corev1.Volume, volumeMountOverrides []corev1.VolumeMount, tolerationOverrides []corev1.Toleration, resourcesOverride *corev1.ResourceRequirements, nodeSelectorOverride map[string]string, err error) {
 	list, listErr := o.lister.OperatorsV1alpha1().SubscriptionLister().Subscriptions(ownerCSV.GetNamespace()).List(labels.Everything())
 	if listErr != nil {
 		err = fmt.Errorf("failed to list subscription namespace=%s - %v", ownerCSV.GetNamespace(), listErr)
@@ -29,6 +29,11 @@ func (o *operatorConfig) GetConfigOverrides(ownerCSV ownerutil.Owner) (envVarOve
 		return
 	}
 
+	if owner.Spec.Config == nil {
+		// No overrides
+		return
+	}
+
 	envVarOverrides = owner.Spec.Config.Env
 	volumeOverrides = owner.Spec.Config.Volumes
 	volumeMountOverrides = owner.Spec.Config.VolumeMounts

pkg/controller/operators/olm/overrides/config.go

@@ -194,14 +194,18 @@ func findToleration(tolerations []corev1.Toleration, toleration corev1.Toleratio
 // into given podSpec
 //
 // If podSpec already defines Resources, it will be overwritten
-func InjectResourcesIntoDeployment(podSpec *corev1.PodSpec, resources corev1.ResourceRequirements) error {
+func InjectResourcesIntoDeployment(podSpec *corev1.PodSpec, resources *corev1.ResourceRequirements) error {
 	if podSpec == nil {
 		return errors.New("no pod spec provided")
 	}
 
+	if resources == nil {
+		return nil
+	}
+
 	for i := range podSpec.Containers {
 		container := &podSpec.Containers[i]
-		container.Resources = resources
+		container.Resources = *resources
 	}
 
 	return nil

pkg/controller/operators/olm/overrides/inject/inject.go

@@ -631,25 +631,25 @@ func TestInjectResourcesIntoDeployment(t *testing.T) {
 	tests := []struct {
 		name      string
 		podSpec   *corev1.PodSpec
-		resources corev1.ResourceRequirements
+		resources *corev1.ResourceRequirements
 		expected  *corev1.PodSpec
 	}{
 		{
-			// PodSpec has one container and empty resources
-			// Expected: Resources will remain empty
-			name: "WithEmptyResources",
+			// PodSpec has one container and existing resources
+			// Expected: PodSpec resources will remain untouched
+			name: "WithNilResources",
 			podSpec: &corev1.PodSpec{
 				Containers: []corev1.Container{
 					corev1.Container{
-						Resources: corev1.ResourceRequirements{},
+						Resources: defaultResources,
 					},
 				},
 			},
-			resources: corev1.ResourceRequirements{},
+			resources: nil,
 			expected: &corev1.PodSpec{
 				Containers: []corev1.Container{
 					corev1.Container{
-						Resources: corev1.ResourceRequirements{},
+						Resources: defaultResources,
 					},
 				},
 			},
@@ -665,7 +665,7 @@ func TestInjectResourcesIntoDeployment(t *testing.T) {
 					},
 				},
 			},
-			resources: defaultResources,
+			resources: &defaultResources,
 			expected: &corev1.PodSpec{
 				Containers: []corev1.Container{
 					corev1.Container{
@@ -686,7 +686,7 @@ func TestInjectResourcesIntoDeployment(t *testing.T) {
 					},
 				},
 			},
-			resources: corev1.ResourceRequirements{},
+			resources: &corev1.ResourceRequirements{},
 			expected: &corev1.PodSpec{
 				Containers: []corev1.Container{
 					corev1.Container{

pkg/controller/operators/olm/overrides/inject/inject_test.go

@@ -1190,7 +1190,7 @@ var _ = Describe("Subscription", func() {
 				Operator: corev1.TolerationOpEqual,
 			},
 		}
-		podResources := corev1.ResourceRequirements{
+		podResources := &corev1.ResourceRequirements{
 			Limits: corev1.ResourceList{
 				corev1.ResourceCPU: resource.MustParse("100m"),
 			},
@@ -1200,7 +1200,7 @@ var _ = Describe("Subscription", func() {
 			},
 		}
 
-		podConfig := v1alpha1.SubscriptionConfig{
+		podConfig := &v1alpha1.SubscriptionConfig{
 			Env:          podEnv,
 			Volumes:      podVolumes,
 			VolumeMounts: podVolumeMounts,
@@ -1259,7 +1259,7 @@ var _ = Describe("Subscription", func() {
 			"foo": "bar",
 		}
 
-		podConfig := v1alpha1.SubscriptionConfig{
+		podConfig := &v1alpha1.SubscriptionConfig{
 			NodeSelector: podNodeSelector,
 		}
 
@@ -2510,7 +2510,7 @@ func checkDeploymentHasPodConfigNodeSelector(t GinkgoTInterface, client operator
 	return nil
 }
 
-func checkDeploymentWithPodConfiguration(t GinkgoTInterface, client operatorclient.ClientInterface, csv *v1alpha1.ClusterServiceVersion, envVar []corev1.EnvVar, volumes []corev1.Volume, volumeMounts []corev1.VolumeMount, tolerations []corev1.Toleration, resources corev1.ResourceRequirements) {
+func checkDeploymentWithPodConfiguration(t GinkgoTInterface, client operatorclient.ClientInterface, csv *v1alpha1.ClusterServiceVersion, envVar []corev1.EnvVar, volumes []corev1.Volume, volumeMounts []corev1.VolumeMount, tolerations []corev1.Toleration, resources *corev1.ResourceRequirements) {
 	resolver := install.StrategyResolver{}
 
 	strategy, err := resolver.UnmarshalStrategy(csv.Spec.InstallStrategy)
@@ -2571,10 +2571,10 @@ func checkDeploymentWithPodConfiguration(t GinkgoTInterface, client operatorclie
 		return
 	}
 
-	findResources := func(existingResource corev1.ResourceRequirements, podResource corev1.ResourceRequirements) (foundResource *corev1.ResourceRequirements, found bool) {
+	findResources := func(existingResource *corev1.ResourceRequirements, podResource *corev1.ResourceRequirements) (foundResource *corev1.ResourceRequirements, found bool) {
 		if reflect.DeepEqual(existingResource, podResource) {
 			found = true
-			foundResource = &podResource
+			foundResource = podResource
 		}
 
 		return
@@ -2595,10 +2595,10 @@ func checkDeploymentWithPodConfiguration(t GinkgoTInterface, client operatorclie
 			require.Equalf(t, v.MountPath, existing.MountPath, "VolumeMount MountPath does not match %s=%s", v.Name, v.MountPath)
 		}
 
-		existing, found := findResources(container.Resources, resources)
+		existing, found := findResources(&container.Resources, resources)
 		require.Truef(t, found, "Resources not injected. Resource=%v", resources)
 		require.NotNil(t, existing)
-		require.Equalf(t, *existing, resources, "Resource=%v does not match expected Resource=%v", existing, resources)
+		require.Equalf(t, existing, resources, "Resource=%v does not match expected Resource=%v", existing, resources)
 	}
 
 	for _, deploymentSpec := range strategyDetailsDeployment.DeploymentSpecs {