Improve ClientIntents' status field accuracy for AWS intents, and add…

… more Kubernetes Events to indicate success and failure cases (#329)

.golangci.yaml

@@ -0,0 +1,11 @@
+# See https://golangci-lint.run/usage/linters/ for linters explanations
+linters:
+  disable-all: true
+  enable:
+    # Enabled by default
+    - errcheck
+    - govet
+    - ineffassign
+    - staticcheck
+    - typecheck
+    - unused

src/operator/api/v1alpha3/clientintents_types.go

@@ -196,7 +196,11 @@ type KafkaTopic struct {
 type IntentsStatus struct {
 	// upToDate field reflects whether the client intents have successfully been applied
 	// to the cluster to the state specified
-	UpToDate bool `json:"upToDate,omitempty"`
+	// +optional
+	UpToDate bool `json:"upToDate"`
+	// The last generation of the intents that was successfully reconciled.
+	// +optional
+	ObservedGeneration int64 `json:"observedGeneration"`
 }
 
 //+kubebuilder:object:root=true

src/operator/config/crd/k8s.otterize.com_clientintents.yaml

@@ -278,6 +278,11 @@ spec:
           status:
             description: IntentsStatus defines the observed state of ClientIntents
             properties:
+              observedGeneration:
+                description: The last generation of the intents that was successfully
+                  reconciled.
+                format: int64
+                type: integer
               upToDate:
                 description: upToDate field reflects whether the client intents have
                   successfully been applied to the cluster to the state specified

src/operator/controllers/aws_pod_reconciler/pods.go

@@ -48,7 +48,7 @@ func (p *AWSPodReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	pod := v1.Pod{}
 	err := p.Get(ctx, req.NamespacedName, &pod)
 
-	if k8serrors.IsNotFound(err) {
+	if k8serrors.IsNotFound(err) || pod.DeletionTimestamp != nil {
 		logger.Infoln("Pod was deleted")
 		return ctrl.Result{}, nil
 	}

src/operator/controllers/intents_controller.go

@@ -171,15 +171,31 @@ func (r *IntentsReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
+	if intents.Status.UpToDate != false && intents.Status.ObservedGeneration != intents.Generation {
+		intentsCopy := intents.DeepCopy()
+		intentsCopy.Status.UpToDate = false
+		if err := r.client.Status().Patch(ctx, intentsCopy, client.MergeFrom(intents)); err != nil {
+			return ctrl.Result{}, err
+		}
+		// we have to finish this reconcile loop here so that the group has a fresh copy of the intents
+		// and that we don't trigger an infinite loop
+		return ctrl.Result{}, nil
+	}
+
 	result, err := r.group.Reconcile(ctx, req)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	intents.Status.UpToDate = true
-	if err := r.client.Status().Update(ctx, intents); err != nil {
-		return ctrl.Result{}, err
+	if intents.DeletionTimestamp == nil {
+		intentsCopy := intents.DeepCopy()
+		intentsCopy.Status.UpToDate = true
+		intentsCopy.Status.ObservedGeneration = intentsCopy.Generation
+		if err := r.client.Status().Patch(ctx, intentsCopy, client.MergeFrom(intents)); err != nil {
+			return ctrl.Result{}, err
+		}
 	}
+
 	return result, nil
 }
 

src/operator/controllers/intents_reconcilers/aws_reconciler.go

@@ -129,7 +129,13 @@ func (r *AWSIntentsReconciler) Reconcile(ctx context.Context, req reconcile.Requ
 	}
 
 	err = r.awsAgent.AddRolePolicy(ctx, req.Namespace, serviceAccountName, intents.Spec.Service.Name, policy.Statement)
-	return ctrl.Result{}, err
+	if err != nil {
+		r.RecordWarningEventf(&intents, consts.ReasonReconcilingAWSPoliciesFailed, "Failed to reconcile AWS policies due to error: %s", err.Error())
+		return ctrl.Result{}, err
+	}
+
+	r.RecordNormalEventf(&intents, consts.ReasonReconciledAWSPolicies, "Successfully reconciled AWS policies")
+	return ctrl.Result{}, nil
 }
 
 func (r *AWSIntentsReconciler) hasMultipleClientsForServiceAccount(ctx context.Context, serviceAccountName string, namespace string) (bool, error) {

src/operator/controllers/intents_reconcilers/consts/consts.go

@@ -13,6 +13,8 @@ const (
 	ReasonRemovingIstioPolicyFailed                     = "RemovingIstioPolicyFailed"
 	ReasonPodsNotFound                                  = "PodsNotFound"
 	ReasonAWSIntentsFoundButNoServiceAccount            = "ReasonAWSIntentsFoundButNoServiceAccount"
+	ReasonReconciledAWSPolicies                         = "ReasonReconciledAWSPolicies"
+	ReasonReconcilingAWSPoliciesFailed                  = "ReasonReconcilingAWSPoliciesFailed"
 	ReasonAWSIntentsServiceAccountUsedByMultipleClients = "ReasonAWSIntentsServiceAccountUsedByMultipleClients"
 	ReasonKubernetesServiceNotFound                     = "KubernetesServiceNotFound"
 	ReasonPortRestrictionUnsupportedForStrings          = "TypeStringPortNotSupported"

src/operator/otterizecrds/clientintents-customresourcedefinition.yaml

@@ -1,3 +1,4 @@
+---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
@@ -30,10 +31,14 @@ spec:
           description: ClientIntents is the Schema for the intents API
           properties:
             apiVersion:
-              description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+              description: 'APIVersion defines the versioned schema of this representation
+              of an object. Servers should convert recognized schemas to the latest
+              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
               type: string
             kind:
-              description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+              description: 'Kind is a string value representing the REST resource this
+              object represents. Servers may infer this from the endpoint the client
+              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
               type: string
             metadata:
               type: object
@@ -140,7 +145,8 @@ spec:
               description: IntentsStatus defines the observed state of ClientIntents
               properties:
                 upToDate:
-                  description: upToDate field reflects whether the client intents have successfully been applied to the cluster to the state specified
+                  description: upToDate field reflects whether the client intents have
+                    successfully been applied to the cluster to the state specified
                   type: boolean
               type: object
           type: object
@@ -154,10 +160,14 @@ spec:
           description: ClientIntents is the Schema for the intents API
           properties:
             apiVersion:
-              description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+              description: 'APIVersion defines the versioned schema of this representation
+              of an object. Servers should convert recognized schemas to the latest
+              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
               type: string
             kind:
-              description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+              description: 'Kind is a string value representing the REST resource this
+              object represents. Servers may infer this from the endpoint the client
+              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
               type: string
             metadata:
               type: object
@@ -278,8 +288,14 @@ spec:
             status:
               description: IntentsStatus defines the observed state of ClientIntents
               properties:
+                observedGeneration:
+                  description: The last generation of the intents that was successfully
+                    reconciled.
+                  format: int64
+                  type: integer
                 upToDate:
-                  description: upToDate field reflects whether the client intents have successfully been applied to the cluster to the state specified
+                  description: upToDate field reflects whether the client intents have
+                    successfully been applied to the cluster to the state specified
                   type: boolean
               type: object
           type: object

src/shared/awsagent/types.go

@@ -11,7 +11,7 @@ const policyNameTagKey = "otterize/policyName"
 const policyNamespaceTagKey = "otterize/policyNamespace"
 const policyHashTagKey = "otterize/policyHash"
 
-const iamRoleDescription = "This IAM role was created by Otterize AWS Integration. For more details go to https://otterize.com"
+const iamRoleDescription = "This IAM role was created by the Otterize intents-operator's AWS integration. For more details, go to https://otterize.com"
 
 // PolicyDocument is our definition of our policies to be uploaded to IAM.
 type PolicyDocument struct {

src/shared/reconcilergroup/reconcilergroup.go

@@ -68,7 +68,7 @@ func (g *Group) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, e
 		return ctrl.Result{}, nil
 	}
 
-	err = g.assureFinalizer(ctx, resourceObject)
+	err = g.ensureFinalizer(ctx, resourceObject)
 	if err != nil {
 		if k8serrors.IsConflict(err) {
 			return ctrl.Result{Requeue: true}, nil
@@ -119,7 +119,7 @@ func (g *Group) removeLegacyFinalizers(ctx context.Context, resource client.Obje
 	return nil
 }
 
-func (g *Group) assureFinalizer(ctx context.Context, resource client.Object) error {
+func (g *Group) ensureFinalizer(ctx context.Context, resource client.Object) error {
 	if !controllerutil.ContainsFinalizer(resource, g.finalizer) {
 		controllerutil.AddFinalizer(resource, g.finalizer)
 		err := g.client.Update(ctx, resource)
@@ -149,7 +149,6 @@ func (g *Group) runGroup(ctx context.Context, req ctrl.Request, finalErr error,
 			if finalErr == nil {
 				finalErr = err
 			}
-			logrus.Errorf("Error in reconciler %T: %s", reconciler, err)
 		}
 		if !res.IsZero() {
 			finalRes = shortestRequeue(res, finalRes)

src/shared/serviceidresolver/serviceidresolver.go

@@ -88,15 +88,22 @@ func (r *Resolver) GetOwnerObject(ctx context.Context, pod *corev1.Pod) (client.
 		ownerObj.SetAPIVersion(owner.APIVersion)
 		ownerObj.SetKind(owner.Kind)
 		err := r.client.Get(ctx, types.NamespacedName{Name: owner.Name, Namespace: obj.GetNamespace()}, ownerObj)
-		if err != nil && k8serrors.IsForbidden(err) {
-			// We don't have permissions for further resolving of the owner object,
-			// and so we treat it as the identity.
-			log.WithError(err).WithFields(logrus.Fields{"owner": owner.Name, "ownerKind": obj.GetObjectKind().GroupVersionKind()}).Warning(
-				"permission error resolving owner, will use owner object as service identifier",
-			)
-			ownerObj.SetName(owner.Name)
-			return ownerObj, nil
-		} else if err != nil {
+		if err != nil {
+			if k8serrors.IsForbidden(err) {
+				// We don't have permissions for further resolving of the owner object,
+				// and so we treat it as the identity.
+				log.WithError(err).WithFields(logrus.Fields{"owner": owner.Name, "ownerKind": obj.GetObjectKind().GroupVersionKind()}).Warning(
+					"permission error resolving owner, will use owner object as service identifier",
+				)
+				ownerObj.SetName(owner.Name)
+				return ownerObj, nil
+			} else if k8serrors.IsNotFound(err) {
+				log.WithError(err).WithFields(logrus.Fields{"owner": owner.Name, "ownerKind": obj.GetObjectKind().GroupVersionKind()}).Warning(
+					"resolving owner failed due to owner not found (this is fine if the owner is also being terminated), will use current owner name as service identifier",
+				)
+				ownerObj.SetName(owner.Name)
+				return ownerObj, nil
+			}
 			return nil, fmt.Errorf("error querying owner reference: %w", err)
 		}