Remove overly verbose event ReasonEnforcementGloballyDisabled in allowexternaltraffic.Off mode & improve event messages (#574)

This PR removes an overly verbose event with reason ReasonEnforcementGloballyDisabled, reported when external traffic policy support is disabled.
The PR also fixes the formatting of the events (to start with capital case, to meed conventions).

Co-authored-by: zohar7ch <zohar.s@otterize.com>

Author: orishoshan

src/operator/controllers/external_traffic/network_policy.go

@@ -27,15 +27,13 @@ import (
 )
 
 const (
-	ReasonEnforcementGloballyDisabled         = "EnforcementGloballyDisabled"
 	ReasonCreatingExternalTrafficPolicyFailed = "CreatingExternalTrafficPolicyFailed"
 	ReasonCreatedExternalTrafficPolicy        = "CreatedExternalTrafficPolicy"
 	ReasonGettingExternalTrafficPolicyFailed  = "GettingExternalTrafficPolicyFailed"
 	ReasonRemovingExternalTrafficPolicy       = "RemovingExternalTrafficPolicy"
 	ReasonRemovingExternalTrafficPolicyFailed = "RemovingExternalTrafficPolicyFailed"
 	ReasonRemovedExternalTrafficPolicy        = "RemovedExternalTrafficPolicy"
 	OtterizeExternalNetworkPolicyNameTemplate = "external-access-to-%s"
-	successMsgNetpolCreate                    = "created external traffic network policy. service '%s' refers to pods protected by network policy '%s'"
 )
 
 type NetworkPolicyHandler struct {
@@ -76,20 +74,20 @@ func (r *NetworkPolicyHandler) createOrUpdateNetworkPolicy(
 		return errors.Wrap(err)
 	}
 	existingPolicy := &v1.NetworkPolicy{}
-	errGetExistingPolicy := r.client.Get(ctx, types.NamespacedName{Name: policyName, Namespace: endpoints.GetNamespace()}, existingPolicy)
+	err = r.client.Get(ctx, types.NamespacedName{Name: policyName, Namespace: endpoints.GetNamespace()}, existingPolicy)
 
 	// No matching network policy found, create one
-	if k8serrors.IsNotFound(errGetExistingPolicy) {
+	if k8serrors.IsNotFound(err) {
 		logrus.Debugf("Creating network policy to allow external traffic to %s (ns %s)", endpoints.GetName(), endpoints.GetNamespace())
 		err := r.client.Create(ctx, newPolicy)
 		if err != nil {
 			return r.handleCreationErrors(ctx, err, newPolicy, owner)
 		}
 		r.RecordNormalEvent(owner, ReasonCreatedExternalTrafficPolicy, successMsg)
 		return nil
-	} else if errGetExistingPolicy != nil {
+	} else if err != nil {
 		r.RecordWarningEventf(owner, ReasonGettingExternalTrafficPolicyFailed, "failed to get external traffic network policy: %s", err.Error())
-		return errGetExistingPolicy
+		return errors.Wrap(err)
 	}
 
 	// Found matching policy, is an update needed?
@@ -507,19 +505,19 @@ func (r *NetworkPolicyHandler) handlePolicyDelete(ctx context.Context, policyNam
 		ownerObj.SetKind(ownerRef.Kind)
 		err := r.client.Get(ctx, types.NamespacedName{Name: ownerRef.Name, Namespace: policyNamespace}, ownerObj)
 		if err != nil {
-			logrus.Debugf("can't get the owner of %s. So no events will be recorded for the deletion", policyName)
+			logrus.Debugf("Can't get the owner of %s. So no events will be recorded for the deletion", policyName)
 		}
 		owner = ownerObj
 	}
 
-	r.RecordNormalEvent(owner, ReasonRemovingExternalTrafficPolicy, "removing external traffic network policy, reconciler was disabled")
+	r.RecordNormalEvent(owner, ReasonRemovingExternalTrafficPolicy, "Removing external traffic network policy")
 
 	err = r.client.Delete(ctx, policy)
 	if err != nil && !k8serrors.IsNotFound(err) {
-		r.RecordWarningEventf(owner, ReasonRemovingExternalTrafficPolicyFailed, "failed removing external traffic network policy: %s", err.Error())
+		r.RecordWarningEventf(owner, ReasonRemovingExternalTrafficPolicyFailed, "Failed removing external traffic network policy: %s", err.Error())
 		return errors.Wrap(err)
 	}
-	r.RecordNormalEvent(owner, ReasonRemovedExternalTrafficPolicy, "removed external traffic network policy, success")
+	r.RecordNormalEvent(owner, ReasonRemovedExternalTrafficPolicy, "Removed external traffic network policy")
 
 	return nil
 }
@@ -533,7 +531,6 @@ func (r *NetworkPolicyHandler) handleNetpolsForOtterizeService(ctx context.Conte
 
 	// delete policy if disabled
 	if r.allowExternalTraffic == allowexternaltraffic.Off {
-		r.RecordNormalEventf(svc, ReasonEnforcementGloballyDisabled, "Skipping created external traffic network policy for service '%s' because enforcement is globally disabled", endpoints.GetName())
 		err = r.handlePolicyDelete(ctx, r.formatPolicyName(endpoints.Name), endpoints.Namespace)
 		if err != nil {
 			return errors.Wrap(err)
@@ -542,7 +539,8 @@ func (r *NetworkPolicyHandler) handleNetpolsForOtterizeService(ctx context.Conte
 	}
 
 	for _, netpol := range netpolList {
-		successMsg := fmt.Sprintf(successMsgNetpolCreate, endpoints.GetName(), netpol.GetName())
+		successMsg := fmt.Sprintf("Created external traffic network policy. service '%s' refers to pods protected by network policy '%s'",
+			endpoints.GetName(), netpol.GetName())
 		err = r.createOrUpdateNetworkPolicy(ctx, endpoints, svc, otterizeServiceName, netpol.Spec.PodSelector, ingressList, successMsg)
 
 		if err != nil {

src/operator/controllers/intents_reconcilers/external_traffic_network_policy/external_traffic_network_policy_test.go

@@ -839,12 +839,7 @@ func (s *ExternalNetworkPolicyReconcilerTestSuite) TestEndpointsReconcilerNetwor
 		err = s.Mgr.GetClient().Get(context.Background(), types.NamespacedName{Namespace: s.TestNamespace, Name: externalNetworkPolicyName}, np)
 		assert.True(errors.IsNotFound(err))
 	})
-	select {
-	case event := <-recorder.Events:
-		s.Require().Contains(event, external_traffic.ReasonEnforcementGloballyDisabled)
-	default:
-		s.Fail("event not raised")
-	}
+	s.ExpectNoEvent(recorder)
 }
 
 func TestExternalNetworkPolicyReconcilerTestSuite(t *testing.T) {

src/operator/controllers/intents_reconcilers/external_traffic_network_policy/external_traffic_network_policy_with_ingress_controllers_configured_test.go

@@ -924,12 +924,7 @@ func (s *ExternalNetworkPolicyReconcilerWithIngressControllersConfiguredTestSuit
 		err = s.Mgr.GetClient().Get(context.Background(), types.NamespacedName{Namespace: s.TestNamespace, Name: externalNetworkPolicyName}, np)
 		assert.True(errors.IsNotFound(err))
 	})
-	select {
-	case event := <-recorder.Events:
-		s.Require().Contains(event, external_traffic.ReasonEnforcementGloballyDisabled)
-	default:
-		s.Fail("event not raised")
-	}
+	s.ExpectNoEvent(recorder)
 }
 
 func (s *ExternalNetworkPolicyReconcilerWithIngressControllersConfiguredTestSuite) TestNetworkPolicyForAWSALBExemption_enabled() {

src/shared/testbase/mocks_tests_suite_base.go

@@ -2,7 +2,6 @@ package testbase
 
 import (
 	mocks "github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/mocks"
-	"github.com/stretchr/testify/suite"
 	"go.uber.org/mock/gomock"
 	"k8s.io/client-go/tools/record"
 	"strings"
@@ -14,7 +13,7 @@ const (
 )
 
 type MocksSuiteBase struct {
-	suite.Suite
+	TestSuiteBase
 	Controller *gomock.Controller
 	Recorder   *record.FakeRecorder
 	Client     *mocks.MockClient
@@ -63,12 +62,3 @@ func (s *MocksSuiteBase) ExpectEventsOrderAndCountDontMatter(expectedEventReason
 		}
 	}
 }
-
-func (s *MocksSuiteBase) ExpectNoEvent(eventRecorder *record.FakeRecorder) {
-	select {
-	case event := <-eventRecorder.Events:
-		s.Fail("Unexpected event found", event)
-	default:
-		// Amazing, no events left behind!
-	}
-}

src/shared/testbase/testsuitebase.go

@@ -22,6 +22,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/record"
 	_ "os"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -39,8 +40,21 @@ const waitForCreationInterval = 20 * time.Millisecond
 const waitForCreationTimeout = 10 * time.Second
 const waitForDeletionTSTimeout = 3 * time.Second
 
-type ControllerManagerTestSuiteBase struct {
+type TestSuiteBase struct {
 	suite.Suite
+}
+
+func (s *TestSuiteBase) ExpectNoEvent(eventRecorder *record.FakeRecorder) {
+	select {
+	case event := <-eventRecorder.Events:
+		s.Fail("Unexpected event found", event)
+	default:
+		// Amazing, no events left behind!
+	}
+}
+
+type ControllerManagerTestSuiteBase struct {
+	TestSuiteBase
 	TestEnv          *envtest.Environment
 	RestConfig       *rest.Config
 	TestNamespace    string