Merge pull request #958 from tkashem/follow-up

Bug 1732613: Follow up for pod configuration

pkg/controller/install/deployment.go

@@ -80,7 +80,9 @@ func (c DeploymentInitializerFuncChain) Apply(deployment *appsv1.Deployment) (er
 	return
 }
 
-type DeploymentInitializerFuncBuilder func(owner ownerutil.Owner) DeploymentInitializerFunc
+// DeploymentInitializerBuilderFunc returns a DeploymentInitializerFunc based on
+// the given context.
+type DeploymentInitializerBuilderFunc func(owner ownerutil.Owner) DeploymentInitializerFunc
 
 func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeploymentInterface, templateAnnotations map[string]string, owner ownerutil.Owner, previousStrategy Strategy, initializers DeploymentInitializerFuncChain) StrategyInstaller {
 	return &StrategyDeploymentInstaller{

pkg/controller/install/resolver.go

@@ -29,7 +29,7 @@ type StrategyResolverInterface interface {
 }
 
 type StrategyResolver struct {
-	ProxyInjectorBuilder DeploymentInitializerFuncBuilder
+	ProxyInjectorBuilderFunc DeploymentInitializerBuilderFunc
 }
 
 func (r *StrategyResolver) UnmarshalStrategy(s v1alpha1.NamedInstallStrategy) (strategy Strategy, err error) {
@@ -51,8 +51,8 @@ func (r *StrategyResolver) InstallerForStrategy(strategyName string, opClient op
 		strategyClient := wrappers.NewInstallStrategyDeploymentClient(opClient, opLister, owner.GetNamespace())
 
 		initializers := []DeploymentInitializerFunc{}
-		if r.ProxyInjectorBuilder != nil {
-			initializers = append(initializers, r.ProxyInjectorBuilder(owner))
+		if r.ProxyInjectorBuilderFunc != nil {
+			initializers = append(initializers, r.ProxyInjectorBuilderFunc(owner))
 		}
 
 		return NewStrategyDeploymentInstaller(strategyClient, annotations, owner, previousStrategy, initializers)

pkg/controller/operators/olm/envvar/initializer.go

@@ -56,12 +56,14 @@ func (d *DeploymentInitializer) initialize(ownerCSV ownerutil.Owner, deployment
 			err = fmt.Errorf("failed to query cluster proxy configuration - %v", err)
 			return err
 		}
+
+		proxyEnvVar = dropEmptyProxyEnv(proxyEnvVar)
 	}
 
 	merged = append(podConfigEnvVar, proxyEnvVar...)
 
 	if len(merged) == 0 {
-		d.logger.Debugf("no env var to inject into csv=%s", ownerCSV.GetName())
+		d.logger.WithField("csv", ownerCSV.GetName()).Debug("no env var to inject into csv")
 	}
 
 	podSpec := deployment.Spec.Template.Spec
@@ -71,3 +73,16 @@ func (d *DeploymentInitializer) initialize(ownerCSV ownerutil.Owner, deployment
 
 	return nil
 }
+
+func dropEmptyProxyEnv(in []corev1.EnvVar) (out []corev1.EnvVar) {
+	out = make([]corev1.EnvVar, 0)
+	for i := range in {
+		if in[i].Value == "" {
+			continue
+		}
+
+		out = append(out, in[i])
+	}
+
+	return
+}

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

@@ -29,18 +29,15 @@ func merge(containerEnvVars []corev1.EnvVar, newEnvVars []corev1.EnvVar) (merged
 
 	for _, newEnvVar := range newEnvVars {
 		existing, found := find(containerEnvVars, newEnvVar.Name)
-		if !found {
-			if newEnvVar.Value != "" {
-				merged = append(merged, corev1.EnvVar{
-					Name:  newEnvVar.Name,
-					Value: newEnvVar.Value,
-				})
-			}
-
+		if found {
+			existing.Value = newEnvVar.Value
 			continue
 		}
 
-		existing.Value = newEnvVar.Value
+		merged = append(merged, corev1.EnvVar{
+			Name:  newEnvVar.Name,
+			Value: newEnvVar.Value,
+		})
 	}
 
 	return

pkg/controller/operators/olm/operator.go

@@ -407,7 +407,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 		return nil, err
 	}
 
-
 	// setup proxy env var injection policies
 	discovery := config.operatorClient.KubernetesInterface().Discovery()
 	proxyAPIExists, err := proxy.IsAPIAvailable(discovery)
@@ -416,7 +415,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 		return nil, err
 	}
 
-	proxyQuerierInUse := proxy.DefaultQuerier()
+	proxyQuerierInUse := proxy.NoopQuerier()
 	if proxyAPIExists {
 		op.logger.Info("OpenShift Proxy API  available - setting up watch for Proxy type")
 
@@ -443,7 +442,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 
 	proxyEnvInjector := envvar.NewDeploymentInitializer(op.logger, proxyQuerierInUse, op.lister)
 	op.resolver = &install.StrategyResolver{
-		ProxyInjectorBuilder: proxyEnvInjector.GetDeploymentInitializer,
+		ProxyInjectorBuilderFunc: proxyEnvInjector.GetDeploymentInitializer,
 	}
 
 	return op, nil
@@ -454,12 +453,21 @@ func (a *Operator) now() metav1.Time {
 }
 
 func (a *Operator) syncSubscription(obj interface{}) error {
-	_, ok := obj.(*v1alpha1.Subscription)
+	sub, ok := obj.(*v1alpha1.Subscription)
 	if !ok {
 		a.logger.Debugf("wrong type: %#v\n", obj)
 		return fmt.Errorf("casting Subscription failed")
 	}
 
+	installedCSV := sub.Status.InstalledCSV
+	if installedCSV != "" {
+		a.logger.WithField("csv", installedCSV).Debug("subscription has changed, requeuing installed csv")
+		if err := a.csvQueueSet.Requeue(sub.GetNamespace(), installedCSV); err != nil {
+			a.logger.WithField("csv", installedCSV).Debug("failed to requeue installed csv")
+			return err
+		}
+	}
+
 	return nil
 }
 

pkg/lib/proxy/querier.go

@@ -4,9 +4,10 @@ import (
 	corev1 "k8s.io/api/core/v1"
 )
 
-// DefaultQuerier does...
-func DefaultQuerier() Querier {
-	return &defaultQuerier{}
+// NoopQuerier returns an instance of noopQuerier. It's used for upstream where
+// we don't have any cluster proxy configuration.
+func NoopQuerier() Querier {
+	return &noopQuerier{}
 }
 
 // Querier is an interface that wraps the QueryProxyConfig method.
@@ -16,11 +17,11 @@ type Querier interface {
 	QueryProxyConfig() (proxy []corev1.EnvVar, err error)
 }
 
-type defaultQuerier struct {
+type noopQuerier struct {
 }
 
 // QueryProxyConfig returns no env variable(s), err is set to nil to indicate
 // that the cluster has no global proxy configuration.
-func (*defaultQuerier) QueryProxyConfig() (proxy []corev1.EnvVar, err error) {
+func (*noopQuerier) QueryProxyConfig() (proxy []corev1.EnvVar, err error) {
 	return
 }

test/e2e/subscription_e2e_test.go

@@ -407,7 +407,7 @@ func createSubscription(t *testing.T, crc versioned.Interface, namespace, name,
 	return buildSubscriptionCleanupFunc(t, crc, subscription)
 }
 
-func createSubscriptionWithPodConfig(t *testing.T, crc versioned.Interface, namespace, name, packageName, channel string, approval v1alpha1.Approval, config v1alpha1.SubscriptionConfig) cleanupFunc {
+func createSubscriptionForCatalog(t *testing.T, crc versioned.Interface, namespace, name, catalog, packageName, channel, startingCSV string, approval v1alpha1.Approval) cleanupFunc {
 	subscription := &v1alpha1.Subscription{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       v1alpha1.SubscriptionKind,
@@ -418,12 +418,12 @@ func createSubscriptionWithPodConfig(t *testing.T, crc versioned.Interface, name
 			Name:      name,
 		},
 		Spec: &v1alpha1.SubscriptionSpec{
-			CatalogSource:          catalogSourceName,
+			CatalogSource:          catalog,
 			CatalogSourceNamespace: namespace,
 			Package:                packageName,
 			Channel:                channel,
+			StartingCSV:            startingCSV,
 			InstallPlanApproval:    approval,
-			Config:                 config,
 		},
 	}
 
@@ -432,7 +432,7 @@ func createSubscriptionWithPodConfig(t *testing.T, crc versioned.Interface, name
 	return buildSubscriptionCleanupFunc(t, crc, subscription)
 }
 
-func createSubscriptionForCatalog(t *testing.T, crc versioned.Interface, namespace, name, catalog, packageName, channel, startingCSV string, approval v1alpha1.Approval) cleanupFunc {
+func createSubscriptionForCatalogWithSpec(t *testing.T, crc versioned.Interface, namespace, name string, spec *v1alpha1.SubscriptionSpec) cleanupFunc {
 	subscription := &v1alpha1.Subscription{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       v1alpha1.SubscriptionKind,
@@ -442,14 +442,7 @@ func createSubscriptionForCatalog(t *testing.T, crc versioned.Interface, namespa
 			Namespace: namespace,
 			Name:      name,
 		},
-		Spec: &v1alpha1.SubscriptionSpec{
-			CatalogSource:          catalog,
-			CatalogSourceNamespace: namespace,
-			Package:                packageName,
-			Channel:                channel,
-			StartingCSV:            startingCSV,
-			InstallPlanApproval:    approval,
-		},
+		Spec: spec,
 	}
 
 	subscription, err := crc.OperatorsV1alpha1().Subscriptions(namespace).Create(subscription)
@@ -1120,11 +1113,9 @@ func TestCreateNewSubscriptionWithPodConfig(t *testing.T) {
 
 	newConfigClient := func(t *testing.T) configv1client.ConfigV1Interface {
 		config, err := clientcmd.BuildConfigFromFlags("", *kubeConfigPath)
-		// require.NoErrorf(t, err, "could not create rest config: %s", err.Error())
 		require.NoError(t, err,)
 
 		client, err := configv1client.NewForConfig(config)
-		// require.NoErrorf(t, err, "error creating OpenShift Config client: %s", err.Error())
 		require.NoError(t, err)
 
 		return client
@@ -1141,33 +1132,36 @@ func TestCreateNewSubscriptionWithPodConfig(t *testing.T) {
 		}
 
 		require.NotNil(t, proxy)
-		proxyEnv := []corev1.EnvVar{
-			corev1.EnvVar{
+		proxyEnv := []corev1.EnvVar{}
+
+		if proxy.Status.HTTPProxy !="" {
+			proxyEnv = append(proxyEnv, corev1.EnvVar{
 				Name:  "HTTP_PROXY",
 				Value: proxy.Status.HTTPProxy,
-			},
-			corev1.EnvVar{
+			})
+		}
+
+		if proxy.Status.HTTPSProxy !="" {
+			proxyEnv = append(proxyEnv, corev1.EnvVar{
 				Name:  "HTTPS_PROXY",
 				Value: proxy.Status.HTTPSProxy,
-			},
-			corev1.EnvVar{
+			})
+		}
+
+		if proxy.Status.NoProxy !="" {
+			proxyEnv = append(proxyEnv, corev1.EnvVar{
 				Name:  "NO_PROXY",
 				Value: proxy.Status.NoProxy,
-			},
+			})
 		}
 
 		return proxyEnv
 	}
 
-	c := newKubeClient(t)
-	crc := newCRClient(t)
+	kubeClient := newKubeClient(t)
+	crClient := newCRClient(t)
 	config := newConfigClient(t)
 
-	defer func() {
-		require.NoError(t, crc.OperatorsV1alpha1().Subscriptions(testNamespace).DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{}))
-	}()
-	require.NoError(t, initCatalog(t, c, crc))
-
 	podEnv := []corev1.EnvVar{
 		corev1.EnvVar{
 			Name:  "MY_ENV_VARIABLE1",
@@ -1182,22 +1176,31 @@ func TestCreateNewSubscriptionWithPodConfig(t *testing.T) {
 		Env: podEnv,
 	}
 
-	subscriptionName := "mysub-podconfig"
-	cleanup := createSubscriptionWithPodConfig(t, crc, testNamespace, subscriptionName, testPackageName, betaChannel, v1alpha1.ApprovalAutomatic, podConfig)
-	defer cleanup()
+	permissions := deploymentPermissions(t)
+	catsrc, subSpec, catsrcCleanup := newCatalogSource(t, kubeClient, crClient, "podconfig", testNamespace, permissions)
+	defer catsrcCleanup()
+
+	// Ensure that the catalog source is resolved before we create a subscription.
+	_, err := fetchCatalogSource(t, crClient, catsrc.GetName(), testNamespace, catalogSourceRegistryPodSynced)
+	require.NoError(t, err)
+
+	subscriptionName := genName("podconfig-sub-")
+	subSpec.Config = podConfig
+	cleanupSubscription := createSubscriptionForCatalogWithSpec(t, crClient, testNamespace, subscriptionName, subSpec)	
+	defer cleanupSubscription()
 
-	subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
+	subscription, err := fetchSubscription(t, crClient, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
 	require.NoError(t, err)
 	require.NotNil(t, subscription)
 
-	csv, err := fetchCSV(t, crc, subscription.Status.CurrentCSV, testNamespace, buildCSVConditionChecker(v1alpha1.CSVPhaseSucceeded))
+	csv, err := fetchCSV(t, crClient, subscription.Status.CurrentCSV, testNamespace, buildCSVConditionChecker(v1alpha1.CSVPhaseSucceeded))
 	require.NoError(t, err)
 
 	proxyEnv := proxyEnvVarFunc(t, config)
 	expected := podEnv
 	expected = append(expected, proxyEnv...)
 
-	checkDeploymentWithPodConfiguration(t, c, csv, podConfig.Env)
+	checkDeploymentWithPodConfiguration(t, kubeClient, csv, podConfig.Env)
 }
 
 func checkDeploymentWithPodConfiguration(t *testing.T, client operatorclient.ClientInterface, csv *v1alpha1.ClusterServiceVersion, envVar []corev1.EnvVar) {

test/e2e/user_defined_sa_test.go

@@ -40,7 +40,8 @@ func TestUserDefinedServiceAccountWithNoPermission(t *testing.T) {
 	_, cleanupOG := newOperatorGroupWithServiceAccount(t, crclient, namespace, ogName, saName)
 	defer cleanupOG()
 
-	catsrc, subSpec, catsrcCleanup := newCatalogSource(t, kubeclient, crclient, namespace)
+	permissions := deploymentPermissions(t)
+	catsrc, subSpec, catsrcCleanup := newCatalogSource(t, kubeclient, crclient, "scoped", namespace, permissions)
 	defer catsrcCleanup()
 
 	// Ensure that the catalog source is resolved before we create a subscription.
@@ -96,7 +97,8 @@ func TestUserDefinedServiceAccountWithPermission(t *testing.T) {
 	_, cleanupOG := newOperatorGroupWithServiceAccount(t, crclient, namespace, ogName, saName)
 	defer cleanupOG()
 
-	catsrc, subSpec, catsrcCleanup := newCatalogSource(t, kubeclient, crclient, namespace)
+	permissions := deploymentPermissions(t)
+	catsrc, subSpec, catsrcCleanup := newCatalogSource(t, kubeclient, crclient, "scoped", namespace, permissions)
 	defer catsrcCleanup()
 
 	// Ensure that the catalog source is resolved before we create a subscription.
@@ -194,7 +196,7 @@ func newOperatorGroupWithServiceAccount(t *testing.T, client versioned.Interface
 	return
 }
 
-func newCatalogSource(t *testing.T, kubeclient operatorclient.ClientInterface, crclient versioned.Interface, namespace string) (catsrc *v1alpha1.CatalogSource, subscriptionSpec *v1alpha1.SubscriptionSpec, cleanup cleanupFunc) {
+func newCatalogSource(t *testing.T, kubeclient operatorclient.ClientInterface, crclient versioned.Interface, prefix, namespace string, permissions []install.StrategyDeploymentPermissions) (catsrc *v1alpha1.CatalogSource, subscriptionSpec *v1alpha1.SubscriptionSpec, cleanup cleanupFunc) {
 	crdPlural := genName("ins")
 	crdName := crdPlural + ".cluster.com"
 
@@ -215,12 +217,15 @@ func newCatalogSource(t *testing.T, kubeclient operatorclient.ClientInterface, c
 		},
 	}
 
+	prefixFunc := func(s string) string {
+		return fmt.Sprintf("%s-%s-", prefix, s)
+	}
+
 	// Create CSV
-	packageName := genName("nginx-")
+	packageName := genName(prefixFunc("package"))
 	stableChannel := "stable"
 
-	permissions := deploymentPermissions(t)
-	namedStrategy := newNginxInstallStrategy(genName("dep-"), permissions, nil)
+	namedStrategy := newNginxInstallStrategy(genName(prefixFunc("dep")), permissions, nil)
 	csvA := newCSV("nginx-a", namespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, namedStrategy)
 	csvB := newCSV("nginx-b", namespace, "nginx-a", semver.MustParse("0.2.0"), []apiextensions.CustomResourceDefinition{crd}, nil, namedStrategy)
 
@@ -235,7 +240,7 @@ func newCatalogSource(t *testing.T, kubeclient operatorclient.ClientInterface, c
 		},
 	}
 
-	catalogSourceName := genName("mock-nginx-")
+	catalogSourceName := genName(prefixFunc("catsrc"))
 	catsrc, cleanup = createInternalCatalogSource(t, kubeclient, crclient, catalogSourceName, namespace, manifests, []apiextensions.CustomResourceDefinition{crd}, []v1alpha1.ClusterServiceVersion{csvA, csvB})
 	require.NotNil(t, catsrc)
 	require.NotNil(t, cleanup)