fix: Malformatted EventMesh Secret causes warning status (#520)

* Refactor test

* Wrap error for malformatted secrets

* Add integration test

* add test case 'missing namespace'

* change linter

* add another test case

s

* remove var

* Clean up

* Remove testcase variable

.github/workflows/lint-go.yml

@@ -18,4 +18,4 @@ permissions:
 jobs:
   unit-test:
     name: "Run golangci-lint"
-    uses: kyma-project/eventing-tools/.github/workflows/lint-reusable.yml@main
+    uses: kyma-project/eventing-tools/.github/workflows/lint-go-reusable.yml@main

internal/controller/operator/eventing/controller.go

@@ -669,6 +669,12 @@ func (r *Reconciler) reconcileEventMeshBackend(ctx context.Context, eventing *op
 	// Start the EventMesh subscription controller
 	err = r.reconcileEventMeshSubManager(ctx, eventing, eventMeshSecret)
 	if err != nil {
+		// In case the the error is caused by a malformatted secret, we want to set the status to warning,
+		// to indicate the requirement of user interaction.
+		if IsMalformattedSecretErr(err) {
+			return kctrl.Result{}, r.syncSubManagerStatusWithNATSState(ctx, operatorv1alpha1.StateWarning, eventing,
+				ErrEventMeshSecretMalformatted, log)
+		}
 		return kctrl.Result{}, r.syncStatusWithSubscriptionManagerErr(ctx, eventing, err, log)
 	}
 	eventing.Status.SetSubscriptionManagerReadyConditionToTrue()

internal/controller/operator/eventing/controller_test.go

@@ -432,11 +432,10 @@ func Test_stopNatsCRWatch(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		testcase := tc
-		t.Run(testcase.name, func(t *testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			// given
 			testEnv := NewMockedUnitTestEnvironment(t)
-			testEnv.Reconciler.natsCRWatchStarted = testcase.natsCRWatchStarted
+			testEnv.Reconciler.natsCRWatchStarted = tc.natsCRWatchStarted
 
 			// Create a fake Watcher
 			natsWatcher := new(watchermocks.Watcher)
@@ -452,8 +451,8 @@ func Test_stopNatsCRWatch(t *testing.T) {
 			testEnv.Reconciler.stopNATSCRWatch(eventing)
 
 			// Check the results
-			require.Equal(t, nil, testcase.watchNatsWatcher)
-			require.False(t, testcase.natsCRWatchStarted)
+			require.Nil(t, tc.watchNatsWatcher)
+			require.False(t, tc.natsCRWatchStarted)
 		})
 	}
 }

internal/controller/operator/eventing/eventmesh.go

@@ -28,13 +28,17 @@ const (
 	secretKeyTokenURL     = "token_url"
 	secretKeyCertsURL     = "certs_url"
 
+	EMSecretMessagingMissing      = "messaging is missing from EM secret"
+	EMSecretNamespaceMissing      = "namespace is missing from EM secret"
 	EventMeshSecretMissingMessage = "The specified EventMesh secret is not found. Please provide an existing secret."
+	EventMeshSecretMalformatted   = "The EventMesh secret data is not formatted properly."
 )
 
 var (
-	ErrEMSecretMessagingMissing = errors.New("messaging is missing from EM secret")
-	ErrEMSecretNamespaceMissing = errors.New("namespace is missing from EM secret")
-	ErrEventMeshSecretMissing   = errors.New(EventMeshSecretMissingMessage)
+	ErrEMSecretMessagingMissing    = errors.New(EMSecretMessagingMissing)
+	ErrEMSecretNamespaceMissing    = errors.New(EMSecretNamespaceMissing)
+	ErrEventMeshSecretMissing      = errors.New(EventMeshSecretMissingMessage)
+	ErrEventMeshSecretMalformatted = errors.New(EventMeshSecretMalformatted)
 )
 
 type oauth2Credentials struct {
@@ -44,6 +48,16 @@ type oauth2Credentials struct {
 	certsURL     []byte
 }
 
+func newMalformattedSecretErr(e string) error {
+	// The new error is handled as a string (%s), the malformation error is wrapped as an Err.
+	return fmt.Errorf("%s: %w", e, ErrEventMeshSecretMalformatted)
+}
+
+// IsMalformattedSecretErr checks if the error is of type ErrEventMeshSecretMalformatted.
+func IsMalformattedSecretErr(err error) bool {
+	return errors.Is(err, ErrEventMeshSecretMalformatted)
+}
+
 func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing *v1alpha1.Eventing, eventMeshSecret *kcorev1.Secret) error {
 	// gets oauth2ClientID and secret and stops the EventMesh subscription manager if changed
 	err := r.syncOauth2ClientIDAndSecret(ctx, eventing)
@@ -314,38 +328,38 @@ func getSecretForPublisher(eventMeshSecret *kcorev1.Secret) (*kcorev1.Secret, er
 		label.KeyName: label.ValueEventingPublisherProxy,
 	}
 
-	if _, ok := eventMeshSecret.Data["messaging"]; !ok {
-		return nil, ErrEMSecretMessagingMissing
-	}
-	messagingBytes := eventMeshSecret.Data["messaging"]
-
 	if _, ok := eventMeshSecret.Data["namespace"]; !ok {
-		return nil, ErrEMSecretNamespaceMissing
+		return nil, newMalformattedSecretErr(EMSecretNamespaceMissing)
 	}
 	namespaceBytes := eventMeshSecret.Data["namespace"]
 
+	if _, ok := eventMeshSecret.Data["messaging"]; !ok {
+		return nil, newMalformattedSecretErr(EMSecretMessagingMissing)
+	}
+	messagingBytes := eventMeshSecret.Data["messaging"]
+
 	var messages []Message
 	err := json.Unmarshal(messagingBytes, &messages)
 	if err != nil {
-		return nil, err
+		return nil, newMalformattedSecretErr(err.Error())
 	}
 
 	for _, msg := range messages {
 		if msg.Broker.BrokerType == "saprestmgw" {
 			if len(msg.OA2.ClientID) == 0 {
-				return nil, errors.New("client ID is missing")
+				return nil, newMalformattedSecretErr("client ID is missing")
 			}
 			if len(msg.OA2.ClientSecret) == 0 {
-				return nil, errors.New("client secret is missing")
+				return nil, newMalformattedSecretErr("client secret is missing")
 			}
 			if len(msg.OA2.TokenEndpoint) == 0 {
-				return nil, errors.New("tokenendpoint is missing")
+				return nil, newMalformattedSecretErr("tokenendpoint is missing")
 			}
 			if len(msg.OA2.GrantType) == 0 {
-				return nil, errors.New("granttype is missing")
+				return nil, newMalformattedSecretErr("granttype is missing")
 			}
 			if len(msg.URI) == 0 {
-				return nil, errors.New("publish URL is missing")
+				return nil, newMalformattedSecretErr("publish URL is missing")
 			}
 
 			secret.StringData = getSecretStringData(msg.OA2.ClientID, msg.OA2.ClientSecret, msg.OA2.TokenEndpoint, msg.OA2.GrantType, msg.URI, string(namespaceBytes))

internal/controller/operator/eventing/eventmesh_test.go

@@ -695,18 +695,18 @@ func Test_GetSecretForPublisher(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		testcase := tc
-		t.Run(testcase.name, func(t *testing.T) {
-			publisherSecret := secretFor(testcase.messagingData, testcase.namespaceData)
+		t.Run(tc.name, func(t *testing.T) {
+			publisherSecret := secretFor(tc.messagingData, tc.namespaceData)
 
 			gotPublisherSecret, err := getSecretForPublisher(publisherSecret)
-			if testcase.expectedError != nil {
+			if tc.expectedError != nil {
 				require.Error(t, err)
-				require.ErrorContains(t, err, testcase.expectedError.Error())
+				require.ErrorIs(t, err, ErrEventMeshSecretMalformatted)
+				require.ErrorContains(t, err, tc.expectedError.Error())
 				return
 			}
 			require.NoError(t, err)
-			require.Equal(t, testcase.expectedSecret, gotPublisherSecret, "invalid publisher secret")
+			require.Equal(t, tc.expectedSecret, gotPublisherSecret, "invalid publisher secret")
 		})
 	}
 }
@@ -1117,3 +1117,37 @@ func Test_syncOauth2ClientIDAndSecret(t *testing.T) {
 		})
 	}
 }
+
+// Test_IsMalfromattedSecret verifies that the function IsMalformattedSecretErr asses correctly
+// if a given error is a malformatted secret error or not.
+func Test_IsMalfromattedSecret(t *testing.T) {
+	testCases := []struct {
+		name       string
+		givenErr   error
+		wantResult bool
+	}{
+		{
+			name:       "should return true when an error is an ErrMalformedSecret",
+			givenErr:   ErrEventMeshSecretMalformatted,
+			wantResult: true,
+		}, {
+			name:       "should return true when an error is a wrapped ErrMalformedSecret",
+			givenErr:   newMalformattedSecretErr("this error will wrap ErrMalformedSecret"),
+			wantResult: true,
+		}, {
+			name:       "should return false when an error is not an ErrMalformedSecret",
+			givenErr:   fmt.Errorf("this is not a malformed secret error"),
+			wantResult: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// When:
+			result := IsMalformattedSecretErr(tc.givenErr)
+
+			// Then:
+			require.Equal(t, tc.wantResult, result)
+		})
+	}
+}