Extract validator creation. If the operator is not a valid semantic v…

…ersion, skip validating compatibility.

cbcontainers/remote_configuration/configurator.go

@@ -29,25 +29,33 @@ type AccessTokenProvider interface {
 	GetCBAccessToken(ctx context.Context, cbContainersCluster *cbcontainersv1.CBContainersAgent, deployedNamespace string) (string, error)
 }
 
+type ChangeValidator interface {
+	ValidateChange(change models.ConfigurationChange, cr *cbcontainersv1.CBContainersAgent) error
+}
+
+type ValidatorCreator func(gateway ApiGateway) (ChangeValidator, error)
+
 type ApiCreator func(cbContainersCluster *cbcontainersv1.CBContainersAgent, accessToken string) (ApiGateway, error)
 
 type Configurator struct {
 	k8sClient           client.Client
 	logger              logr.Logger
 	accessTokenProvider AccessTokenProvider
 	apiCreator          ApiCreator
-	operatorVersion     string
 	deployedNamespace   string
 	clusterIdentifier   string
-	mux                 sync.Mutex
+
+	validatorCreator ValidatorCreator
+
+	mux sync.Mutex
 }
 
 func NewConfigurator(
 	k8sClient client.Client,
 	gatewayCreator ApiCreator,
 	logger logr.Logger,
 	accessTokenProvider AccessTokenProvider,
-	operatorVersion string,
+	validatorCreator ValidatorCreator,
 	deployedNamespace string,
 	clusterIdentifier string,
 ) *Configurator {
@@ -56,7 +64,7 @@ func NewConfigurator(
 		logger:              logger,
 		apiCreator:          gatewayCreator,
 		accessTokenProvider: accessTokenProvider,
-		operatorVersion:     operatorVersion,
+		validatorCreator:    validatorCreator,
 		deployedNamespace:   deployedNamespace,
 		clusterIdentifier:   clusterIdentifier,
 		mux:                 sync.Mutex{},
@@ -161,12 +169,12 @@ func (configurator *Configurator) getPendingChange(ctx context.Context, apiGatew
 }
 
 func (configurator *Configurator) applyChangeToCR(ctx context.Context, apiGateway ApiGateway, change models.ConfigurationChange, cr *cbcontainersv1.CBContainersAgent) error {
-	validator, err := NewConfigurationChangeValidator(configurator.operatorVersion, apiGateway)
+	validator, err := configurator.validatorCreator(apiGateway)
 	if err != nil {
 		return fmt.Errorf("failed to create configuration change validator; %w", err)
 	}
 	if err := validator.ValidateChange(change, cr); err != nil {
-		return err
+		return invalidChangeError{msg: err.Error()}
 	}
 	ApplyConfigChangeToCR(change, cr)
 	return configurator.k8sClient.Update(ctx, cr)

cbcontainers/remote_configuration/configurator_test.go

@@ -21,29 +21,32 @@ type configuratorMocks struct {
 	k8sClient           *k8sMocks.MockClient
 	apiGateway          *mocks.MockApiGateway
 	accessTokenProvider *mocks.MockAccessTokenProvider
+	validator           *mocks.MockChangeValidator
 
-	stubAccessToken     string
-	stubOperatorVersion string
-	stubNamespace       string
-	stubClusterID       string
+	stubAccessToken string
+	stubNamespace   string
+	stubClusterID   string
 }
 
 // setupConfigurator sets up mocks and creates a Configurator instance with those mocks and some dummy data
 func setupConfigurator(ctrl *gomock.Controller) (*remote_configuration.Configurator, configuratorMocks) {
 	k8sClient := k8sMocks.NewMockClient(ctrl)
 	apiGateway := mocks.NewMockApiGateway(ctrl)
 	accessTokenProvider := mocks.NewMockAccessTokenProvider(ctrl)
+	validator := mocks.NewMockChangeValidator(ctrl)
 
 	var mockAPIProvider remote_configuration.ApiCreator = func(
 		cbContainersCluster *cbcontainersv1.CBContainersAgent,
 		accessToken string,
 	) (remote_configuration.ApiGateway, error) {
 		return apiGateway, nil
 	}
+	var mockValidatorProvider remote_configuration.ValidatorCreator = func(gateway remote_configuration.ApiGateway) (remote_configuration.ChangeValidator, error) {
+		return validator, nil
+	}
 
 	namespace := "namespace-name"
 	accessToken := "access-token"
-	operatorVersion := "1.2.3"
 	clusterID := "1234567"
 	accessTokenProvider.EXPECT().GetCBAccessToken(gomock.Any(), gomock.Any(), namespace).Return(accessToken, nil).AnyTimes()
 
@@ -52,7 +55,7 @@ func setupConfigurator(ctrl *gomock.Controller) (*remote_configuration.Configura
 		mockAPIProvider,
 		logr.Discard(),
 		accessTokenProvider,
-		operatorVersion,
+		mockValidatorProvider,
 		namespace,
 		clusterID,
 	)
@@ -61,8 +64,8 @@ func setupConfigurator(ctrl *gomock.Controller) (*remote_configuration.Configura
 		k8sClient:           k8sClient,
 		apiGateway:          apiGateway,
 		accessTokenProvider: accessTokenProvider,
+		validator:           validator,
 		stubAccessToken:     accessToken,
-		stubOperatorVersion: operatorVersion,
 		stubNamespace:       namespace,
 		stubClusterID:       clusterID,
 	}
@@ -84,7 +87,7 @@ func TestConfigChangeIsAppliedAndAcknowledgedCorrectly(t *testing.T) {
 	configChange.AgentVersion = &expectedAgentVersion
 
 	setupCRInK8S(mocks.k8sClient, cr)
-	setupValidCompatibilityData(mocks.apiGateway, mocks.stubOperatorVersion)
+	mocks.validator.EXPECT().ValidateChange(configChange, cr).Return(nil)
 	mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)
 
 	// Setup mock assertions
@@ -117,28 +120,22 @@ func TestWhenChangeIsNotApplicableShouldReturnError(t *testing.T) {
 	configurator, mocks := setupConfigurator(ctrl)
 
 	cr := &cbcontainersv1.CBContainersAgent{}
-	maxAgentVersionForOperator := "4.0.0"
 	agentVersion := "5.0.0"
 	configChange := randomPendingConfigChange()
 	configChange.AgentVersion = &agentVersion
 
 	setupCRInK8S(mocks.k8sClient, cr)
 	mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)
 
-	// Setup invalid compatibility; no need to do full verification here - this is what the validator tests are for
-	// We just want to check that _some_ validation happens
-	mocks.apiGateway.EXPECT().GetCompatibilityMatrixEntryFor(mocks.stubOperatorVersion).Return(&models.OperatorCompatibility{
-		MinAgent: models.AgentMinVersionNone,
-		MaxAgent: models.AgentVersion(maxAgentVersionForOperator),
-	}, nil)
+	mocks.validator.EXPECT().ValidateChange(configChange, cr).Return(errors.New("some error"))
 
 	// Setup mock assertions
 	mocks.apiGateway.EXPECT().UpdateConfigurationChangeStatus(gomock.Any(), gomock.Any()).
 		DoAndReturn(func(_ context.Context, update models.ConfigurationChangeStatusUpdate) error {
 			assert.Equal(t, configChange.ID, update.ID)
 			assert.Equal(t, models.ChangeStatusFailed, update.Status)
 			assert.NotEmpty(t, update.Error)
-			assert.NotEmpty(t, update.ErrorReason)
+			assert.NotEmpty(t, update.ErrorReason) // Should be populated for validation errors
 			assert.Equal(t, int64(0), update.AppliedGeneration)
 			assert.Empty(t, update.AppliedTimestamp)
 			assert.Equal(t, mocks.stubClusterID, update.ClusterIdentifier)
@@ -204,8 +201,8 @@ func TestWhenThereAreMultiplePendingChangesTheOldestIsSelected(t *testing.T) {
 	newerChange.Timestamp = time.Now().UTC().Add(-1 * time.Hour).Format(time.RFC3339)
 
 	setupCRInK8S(mocks.k8sClient, nil)
+	setupValidatorAcceptAll(mocks.validator)
 	mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{newerChange, olderChange}, nil)
-	setupValidCompatibilityData(mocks.apiGateway, mocks.stubOperatorVersion)
 
 	setupUpdateCRMock(t, mocks.k8sClient, func(agent *cbcontainersv1.CBContainersAgent) {
 		assert.Equal(t, expectedVersion, agent.Spec.Version)
@@ -260,9 +257,9 @@ func TestWhenUpdatingCRFailsChangeIsUpdatedAsFailed(t *testing.T) {
 
 	configChange := randomPendingConfigChange()
 
-	mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)
 	setupCRInK8S(mocks.k8sClient, nil)
-	setupValidCompatibilityData(mocks.apiGateway, mocks.stubOperatorVersion)
+	setupValidatorAcceptAll(mocks.validator)
+	mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)
 
 	errFromService := errors.New("some error")
 	mocks.k8sClient.EXPECT().Update(gomock.Any(), gomock.Any()).Return(errFromService)
@@ -294,7 +291,7 @@ func TestWhenUpdatingStatusToBackendFailsShouldReturnError(t *testing.T) {
 	configChange := randomPendingConfigChange()
 
 	setupCRInK8S(mocks.k8sClient, nil)
-	setupValidCompatibilityData(mocks.apiGateway, mocks.stubOperatorVersion)
+	setupValidatorAcceptAll(mocks.validator)
 	mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)
 	mocks.k8sClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 
@@ -373,9 +370,6 @@ func setupUpdateCRMock(t *testing.T, mock *k8sMocks.MockClient, assert func(*cbc
 		})
 }
 
-func setupValidCompatibilityData(mockGateway *mocks.MockApiGateway, operatorVersion string) {
-	mockGateway.EXPECT().GetCompatibilityMatrixEntryFor(operatorVersion).Return(&models.OperatorCompatibility{
-		MinAgent: models.AgentMinVersionNone,
-		MaxAgent: models.AgentMaxVersionLatest,
-	}, nil)
+func setupValidatorAcceptAll(validator *mocks.MockChangeValidator) {
+	validator.EXPECT().ValidateChange(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 }

cbcontainers/remote_configuration/mocks/generated.go

@@ -1,4 +1,5 @@
 package mocks
 
-//go:generate mockgen -destination mock_api_gateway.go -package mocks github.com/vmware/cbcontainers-operator/remote_configuration ApiGateway
-//go:generate mockgen -destination mock_access_token_provider.go -package mocks github.com/vmware/cbcontainers-operator/remote_configuration AccessTokenProvider
+//go:generate mockgen -destination mock_api_gateway.go -package mocks github.com/vmware/cbcontainers-operator/cbcontainers/remote_configuration ApiGateway
+//go:generate mockgen -destination mock_access_token_provider.go -package mocks github.com/vmware/cbcontainers-operator/cbcontainers/remote_configuration AccessTokenProvider
+//go:generate mockgen -destination mock_validator.go -package mocks github.com/vmware/cbcontainers-operator/cbcontainers/remote_configuration ChangeValidator

cbcontainers/remote_configuration/validation.go

@@ -14,6 +14,18 @@ func (i invalidChangeError) Error() string {
 	return i.msg
 }
 
+// EmptyConfigurationChangeValidator can be used when no validation should occur
+type EmptyConfigurationChangeValidator struct {
+}
+
+func (emptyValidator *EmptyConfigurationChangeValidator) ValidateChange(change models.ConfigurationChange, cr *cbcontainersv1.CBContainersAgent) error {
+	return nil
+}
+
+type ConfigurationChangeValidator struct {
+	OperatorCompatibilityData models.OperatorCompatibility
+}
+
 func NewConfigurationChangeValidator(operatorVersion string, api ApiGateway) (*ConfigurationChangeValidator, error) {
 	compatibilityMatrix, err := api.GetCompatibilityMatrixEntryFor(operatorVersion)
 	if err != nil {
@@ -28,10 +40,6 @@ func NewConfigurationChangeValidator(operatorVersion string, api ApiGateway) (*C
 	}, nil
 }
 
-type ConfigurationChangeValidator struct {
-	OperatorCompatibilityData models.OperatorCompatibility
-}
-
 func (validator *ConfigurationChangeValidator) ValidateChange(change models.ConfigurationChange, cr *cbcontainersv1.CBContainersAgent) error {
 	var versionToValidate string
 
@@ -43,12 +51,5 @@ func (validator *ConfigurationChangeValidator) ValidateChange(change models.Conf
 		versionToValidate = cr.Spec.Version
 	}
 
-	return validator.validateOperatorAndSensorVersionCompatibility(versionToValidate)
-}
-
-func (validator *ConfigurationChangeValidator) validateOperatorAndSensorVersionCompatibility(sensorVersion string) error {
-	if err := validator.OperatorCompatibilityData.CheckCompatibility(sensorVersion); err != nil {
-		return invalidChangeError{msg: err.Error()}
-	}
-	return nil
+	return validator.OperatorCompatibilityData.CheckCompatibility(versionToValidate)
 }

main.go

@@ -183,6 +183,19 @@ func main() {
 		setupLog.Error(err, "unable to read the running operator's version from environment variable")
 		os.Exit(1)
 	}
+
+	var validatorCreator remote_configuration.ValidatorCreator
+	if err != nil && errors.Is(err, operator.ErrNotSemVer) {
+		setupLog.Info(fmt.Sprintf("Detected operator version (%s) is not a semantic version. Compatibility checks for remote configuration will be disabled", operatorVersion))
+		validatorCreator = func(_ remote_configuration.ApiGateway) (remote_configuration.ChangeValidator, error) {
+			return &remote_configuration.EmptyConfigurationChangeValidator{}, nil
+		}
+	} else {
+		validatorCreator = func(gateway remote_configuration.ApiGateway) (remote_configuration.ChangeValidator, error) {
+			return remote_configuration.NewConfigurationChangeValidator(operatorVersion, gateway)
+		}
+	}
+
 	var configuratorGatewayCreator remote_configuration.ApiCreator = func(cbContainersCluster *operatorcontainerscarbonblackiov1.CBContainersAgent, accessToken string) (remote_configuration.ApiGateway, error) {
 		return gateway.NewDefaultGatewayCreator().CreateGateway(cbContainersCluster, accessToken)
 	}
@@ -192,7 +205,7 @@ func main() {
 		configuratorGatewayCreator,
 		log,
 		operator.NewSecretAccessTokenProvider(k8sClient),
-		operatorVersion,
+		validatorCreator,
 		operatorNamespace,
 		clusterIdentifier,
 	)