Add conditions.Factory (#58)

The conditions.NewCondition function reads cluster state to determine
the namespace the operator is running in so that it can manipulate the
correct OperatorConditions object in a cluster.

However this function's dependency on a specific file existing in a
specific location makes it difficult to test code that uses it.

The Factory interface is introduced to enable users to provide their own
implementations for building a Condition object to manage OperatorCondition
resources. Users can now build a factory and inject it into code that builds
new conditions. This enables code under test to use the Factory abstraction
and enables operator authors to inject custom functionality needed when
writing tests.

Signed-off-by: Joe Lanford <[email protected]>

Author: joelanford

conditions/conditions.go

@@ -17,26 +17,17 @@ package conditions
 import (
 	"context"
 	"fmt"
-	"os"
 
 	apiv2 "github.com/operator-framework/api/pkg/operators/v2"
-	"github.com/operator-framework/operator-lib/internal/utils"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var (
-	// readNamespace gets the namespacedName of the operator.
-	readNamespace = utils.GetOperatorNamespace
-)
-
-const (
-	// operatorCondEnvVar is the env variable which
-	// contains the name of the Condition CR associated to the operator,
-	// set by OLM.
-	operatorCondEnvVar = "OPERATOR_CONDITION_NAME"
+	// ErrNoOperatorCondition indicates that the operator condition CRD is nil
+	ErrNoOperatorCondition = fmt.Errorf("operator Condition CRD is nil")
 )
 
 // condition is a Condition that gets and sets a specific
@@ -49,21 +40,6 @@ type condition struct {
 
 var _ Condition = &condition{}
 
-// NewCondition returns a new Condition interface using the provided client
-// for the specified conditionType. The condition will internally fetch the namespacedName
-// of the operatorConditionCRD.
-func NewCondition(cl client.Client, condType apiv2.ConditionType) (Condition, error) {
-	objKey, err := GetNamespacedName()
-	if err != nil {
-		return nil, err
-	}
-	return &condition{
-		namespacedName: *objKey,
-		condType:       condType,
-		client:         cl,
-	}, nil
-}
-
 // Get implements conditions.Get
 func (c *condition) Get(ctx context.Context) (*metav1.Condition, error) {
 	operatorCond := &apiv2.OperatorCondition{}
@@ -92,33 +68,9 @@ func (c *condition) Set(ctx context.Context, status metav1.ConditionStatus, opti
 		Status: status,
 	}
 
-	if len(option) != 0 {
-		for _, opt := range option {
-			opt(newCond)
-		}
+	for _, opt := range option {
+		opt(newCond)
 	}
 	meta.SetStatusCondition(&operatorCond.Spec.Conditions, *newCond)
-	err = c.client.Update(ctx, operatorCond)
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-// GetNamespacedName returns the NamespacedName of the CR. It returns an error
-// when the name of the CR cannot be found from the environment variable set by
-// OLM. Hence, GetNamespacedName() can provide the NamespacedName when the operator
-// is running on cluster and is being managed by OLM. If running locally, operator
-// writers are encouraged to skip this method or gracefully handle the errors by logging
-// a message.
-func GetNamespacedName() (*types.NamespacedName, error) {
-	conditionName := os.Getenv(operatorCondEnvVar)
-	if conditionName == "" {
-		return nil, fmt.Errorf("could not determine operator condition name: environment variable %s not set", operatorCondEnvVar)
-	}
-	operatorNs, err := readNamespace()
-	if err != nil {
-		return nil, fmt.Errorf("could not determine operator namespace: %v", err)
-	}
-	return &types.NamespacedName{Name: conditionName, Namespace: operatorNs}, nil
+	return c.client.Update(ctx, operatorCond)
 }

conditions/conditions_test.go

@@ -32,48 +32,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
-const (
-	conditionFoo apiv2.ConditionType = "conditionFoo"
-	conditionBar apiv2.ConditionType = "conditionBar"
-)
-
 var _ = Describe("Condition", func() {
 	var ns = "default"
 	ctx := context.TODO()
 	var clock kubeclock.Clock = &kubeclock.RealClock{}
-	var transitionTime metav1.Time = metav1.Time{Time: clock.Now()}
+	var transitionTime = metav1.Time{Time: clock.Now()}
 	var cl client.Client
-	var err error
-
-	BeforeEach(func() {
-		sch := runtime.NewScheme()
-		err = apiv2.AddToScheme(sch)
-		Expect(err).NotTo(HaveOccurred())
-		cl = fake.NewClientBuilder().WithScheme(sch).Build()
-	})
-
-	Describe("NewCondition", func() {
-		It("should create a new condition", func() {
-			err := os.Setenv(operatorCondEnvVar, "test-operator-condition")
-			Expect(err).NotTo(HaveOccurred())
-			readNamespace = func() (string, error) {
-				return ns, nil
-			}
-
-			c, err := NewCondition(cl, conditionFoo)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(c).NotTo(BeNil())
-		})
-
-		It("should error when namespacedName cannot be found", func() {
-			err := os.Unsetenv(operatorCondEnvVar)
-			Expect(err).NotTo(HaveOccurred())
-
-			c, err := NewCondition(cl, conditionFoo)
-			Expect(err).To(HaveOccurred())
-			Expect(c).To(BeNil())
-		})
-	})
 
 	Describe("Get/Set", func() {
 		var operatorCond *apiv2.OperatorCondition
@@ -160,7 +124,6 @@ var _ = Describe("Condition", func() {
 				Expect(apierrors.IsNotFound(err)).To(BeTrue())
 				Expect(con).To(BeNil())
 			})
-
 		})
 
 		Context("Set", func() {
@@ -211,53 +174,6 @@ var _ = Describe("Condition", func() {
 				Expect(err).To(HaveOccurred())
 				Expect(apierrors.IsNotFound(err)).To(BeTrue())
 			})
-
-		})
-
-	})
-
-	Describe("GetNamespacedName", func() {
-		It("should error when name of the operator condition cannot be found", func() {
-			err := os.Unsetenv(operatorCondEnvVar)
-			Expect(err).NotTo(HaveOccurred())
-
-			objKey, err := GetNamespacedName()
-			Expect(err).To(HaveOccurred())
-			Expect(objKey).To(BeNil())
-			Expect(err.Error()).To(ContainSubstring("could not determine operator condition name"))
-		})
-
-		It("should error when object namespace cannot be found", func() {
-			err := os.Setenv(operatorCondEnvVar, "test")
-			Expect(err).NotTo(HaveOccurred())
-
-			readNamespace = func() (string, error) {
-				return "", os.ErrNotExist
-			}
-
-			objKey, err := GetNamespacedName()
-			Expect(err).To(HaveOccurred())
-			Expect(objKey).To(BeNil())
-			Expect(err.Error()).To(ContainSubstring("could not determine operator namespace"))
-		})
-
-		It("should return the right namespaced name from SA namespace file", func() {
-			err := os.Setenv(operatorCondEnvVar, "test")
-			Expect(err).NotTo(HaveOccurred())
-
-			readNamespace = func() (string, error) {
-				return "testns", nil
-			}
-			objKey, err := GetNamespacedName()
-			Expect(err).NotTo(HaveOccurred())
-			Expect(objKey).NotTo(BeNil())
-			Expect(objKey.Name).To(BeEquivalentTo("test"))
-			Expect(objKey.Namespace).To(BeEquivalentTo("testns"))
 		})
 	})
 })
-
-func deleteCondition(ctx context.Context, client client.Client, obj client.Object) {
-	err := client.Delete(ctx, obj)
-	Expect(err).NotTo(HaveOccurred())
-}

conditions/factory.go

@@ -0,0 +1,119 @@
+// Copyright 2021 The Operator-SDK Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package conditions
+
+import (
+	"fmt"
+	"os"
+
+	apiv2 "github.com/operator-framework/api/pkg/operators/v2"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/operator-framework/operator-lib/internal/utils"
+)
+
+// Factory define the interface for building Conditions.
+type Factory interface {
+	NewCondition(apiv2.ConditionType) (Condition, error)
+	GetNamespacedName() (*types.NamespacedName, error)
+}
+
+// InClusterFactory is a conditions factory that can build conditions and get
+// the namespaced name of the operator's condition based on an in-cluster
+// configuration.
+type InClusterFactory struct {
+	Client client.Client
+}
+
+// NewCondition creates a new Condition using the provided client and condition
+// type. The condition's name and namespace are determined by the Factory's GetName
+// and GetNamespace functions.
+func (f InClusterFactory) NewCondition(condType apiv2.ConditionType) (Condition, error) {
+	objKey, err := f.GetNamespacedName()
+	if err != nil {
+		return nil, err
+	}
+	return &condition{
+		namespacedName: *objKey,
+		condType:       condType,
+		client:         f.Client,
+	}, nil
+}
+
+// GetNamespacedName returns the NamespacedName of the CR. It returns an error
+// when the name of the CR cannot be found from the environment variable set by
+// OLM. Hence, GetNamespacedName() can provide the NamespacedName when the operator
+// is running on cluster and is being managed by OLM.
+func (f InClusterFactory) GetNamespacedName() (*types.NamespacedName, error) {
+	conditionName, err := f.getConditionName()
+	if err != nil {
+		return nil, fmt.Errorf("get operator condition name: %v", err)
+	}
+	conditionNamespace, err := f.getConditionNamespace()
+	if err != nil {
+		return nil, fmt.Errorf("get operator condition namespace: %v", err)
+	}
+
+	return &types.NamespacedName{Name: conditionName, Namespace: conditionNamespace}, nil
+}
+
+const (
+	// operatorCondEnvVar is the env variable which
+	// contains the name of the Condition CR associated to the operator,
+	// set by OLM.
+	operatorCondEnvVar = "OPERATOR_CONDITION_NAME"
+)
+
+// getConditionName reads and returns the OPERATOR_CONDITION_NAME environment
+// variable. If the variable is unset or empty, it returns an error.
+func (f InClusterFactory) getConditionName() (string, error) {
+	name := os.Getenv(operatorCondEnvVar)
+	if name == "" {
+		return "", fmt.Errorf("could not determine operator condition name: environment variable %s not set", operatorCondEnvVar)
+	}
+	return name, nil
+}
+
+// readNamespace gets the namespacedName of the operator.
+var readNamespace = utils.GetOperatorNamespace
+
+// getConditionNamespace reads the namespace file mounted into a pod in a
+// cluster via its service account volume. If the file is not found or cannot be
+// read, this function returns an error.
+func (f InClusterFactory) getConditionNamespace() (string, error) {
+	return readNamespace()
+}
+
+// NewCondition returns a new Condition interface using the provided client
+// for the specified conditionType. The condition will internally fetch the namespacedName
+// of the operatorConditionCRD.
+//
+// Deprecated: Use InClusterFactory{cl}.NewCondition() instead.
+func NewCondition(cl client.Client, condType apiv2.ConditionType) (Condition, error) {
+	return InClusterFactory{cl}.NewCondition(condType)
+}
+
+// GetNamespacedName returns the NamespacedName of the CR. It returns an error
+// when the name of the CR cannot be found from the environment variable set by
+// OLM. Hence, GetNamespacedName() can provide the NamespacedName when the operator
+// is running on cluster and is being managed by OLM. If running locally, operator
+// writers are encouraged to skip this method or gracefully handle the errors by logging
+// a message.
+//
+// Deprecated: InClusterFactory{}.GetNamespacedName().
+func GetNamespacedName() (*types.NamespacedName, error) {
+	return InClusterFactory{}.GetNamespacedName()
+}