chore: improve getEnvironment add tests, fix metrics labels

.kubernetes/manifests.yaml

@@ -295,7 +295,8 @@ spec:
           [{
             "metrics": [
               "controller*",
-              "workqueue*"
+              "workqueue*",
+              "custom*"
             ],
             "namespace": "kcio",
             "prometheus_url": "http://%%host%%:8080/metrics",
@@ -324,7 +325,7 @@ spec:
               key: account
               name: spotinst-credentials
               optional: true
-        image: tfgco/kubernetes-crossplane-infrastructure-operator:v0.8.0-alpha
+        image: tfgco/kubernetes-crossplane-infrastructure-operator:v0.8.4-alpha
         livenessProbe:
           httpGet:
             path: /healthz

config/default/manager_datadog_openmetrics_patch.yaml

@@ -13,7 +13,8 @@ spec:
           [{
             "metrics": [
               "controller*",
-              "workqueue*"
+              "workqueue*",
+              "custom*"
             ],
             "namespace": "kcio",
             "prometheus_url": "http://%%host%%:8080/metrics",

config/manager/kustomization.yaml

@@ -13,4 +13,4 @@ kind: Kustomization
 images:
 - name: controller
   newName: tfgco/kubernetes-crossplane-infrastructure-operator
-  newTag: v0.8.3-alpha
+  newTag: v0.8.4-alpha

internal/controller/ec2.aws/securitygroup_controller.go

@@ -976,34 +976,43 @@ func (r *SecurityGroupReconciliation) addKCPFinalizer(ctx context.Context, kcp k
 }
 
 func (r *SecurityGroupReconciliation) getEnvironment(ctx context.Context) string {
-	ref := r.sg.Spec.InfrastructureRef[0]
 	var clusterObject clusterv1beta1.Cluster
-	switch ref.Kind {
-	case "KopsMachinePool":
-		kmp := kinfrastructurev1alpha1.KopsMachinePool{}
-		key := client.ObjectKey{
-			Name:      ref.Name,
-			Namespace: ref.Namespace,
-		}
-		if err := r.Client.Get(ctx, key, &kmp); err != nil {
-			return ""
-		}
-		clusterName := &kmp.Spec.ClusterName
-		key = client.ObjectKey{
-			Name:      *clusterName,
-			Namespace: kmp.ObjectMeta.Namespace,
-		}
-		if err := r.Client.Get(ctx, key, &clusterObject); err != nil {
-			return ""
-		}
-	case "KopsControlPlane":
-		key := client.ObjectKey{
-			Name:      ref.Name,
-			Namespace: ref.Namespace,
+	var err error
+
+	for _, ref := range r.sg.Spec.InfrastructureRef {
+		err = nil
+		switch ref.Kind {
+		case "KopsMachinePool":
+			kmp := kinfrastructurev1alpha1.KopsMachinePool{}
+			key := client.ObjectKey{
+				Name:      ref.Name,
+				Namespace: ref.Namespace,
+			}
+			if err = r.Client.Get(ctx, key, &kmp); err != nil {
+				continue
+			}
+			clusterName := &kmp.Spec.ClusterName
+			key = client.ObjectKey{
+				Name:      *clusterName,
+				Namespace: kmp.ObjectMeta.Namespace,
+			}
+			err = r.Client.Get(ctx, key, &clusterObject)
+		case "KopsControlPlane":
+			key := client.ObjectKey{
+				Name:      ref.Name,
+				Namespace: ref.Namespace,
+			}
+			err = r.Client.Get(ctx, key, &clusterObject)
 		}
-		if err := r.Client.Get(ctx, key, &clusterObject); err != nil {
-			return ""
+		if err != nil {
+			continue
+		} else {
+			break
 		}
 	}
-	return clusterObject.Labels["environment"]
+	if err == nil {
+		return clusterObject.Labels["environment"]
+	} else {
+		return "environmentNotFound"
+	}
 }

internal/controller/ec2.aws/securitygroup_controller_test.go

@@ -4638,6 +4638,131 @@ func TestCustomMetrics(t *testing.T) {
 	}
 }
 
+func TestGetEnvironment(t *testing.T) {
+	testCases := []struct {
+		description         string
+		k8sObjects          []client.Object
+		sg                  *securitygroupv1alpha2.SecurityGroup
+		expectedEnvironment string
+	}{
+		{
+			description: "should find environment by KMP",
+			k8sObjects: []client.Object{
+				kmp,
+				cluster,
+			},
+			sg:                  sg,
+			expectedEnvironment: "testing",
+		},
+		{
+			description: "should find environment by KCP",
+			k8sObjects: []client.Object{
+				kcp,
+				cluster,
+			},
+			sg:                  sgKCP,
+			expectedEnvironment: "testing",
+		},
+		{
+			description: "should not find environment because cluster not exists",
+			k8sObjects: []client.Object{
+				kmp,
+			},
+			sg:                  sgKCP,
+			expectedEnvironment: "environmentNotFound",
+		},
+		{
+			description: "should not find environment because kmp not exists",
+			k8sObjects: []client.Object{
+				cluster,
+			},
+			sg:                  sg,
+			expectedEnvironment: "environmentNotFound",
+		},
+		{
+			description: "should find environment by second KMP",
+			k8sObjects: []client.Object{
+				kmp,
+				cluster,
+			},
+			sg: &securitygroupv1alpha2.SecurityGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-security-group",
+				},
+				Spec: securitygroupv1alpha2.SecurityGroupSpec{
+					IngressRules: []securitygroupv1alpha2.IngressRule{
+						{
+							IPProtocol: "TCP",
+							FromPort:   40000,
+							ToPort:     60000,
+							AllowedCIDRBlocks: []string{
+								"0.0.0.0/0",
+							},
+						},
+					},
+					InfrastructureRef: []*corev1.ObjectReference{
+						{
+							APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha1",
+							Kind:       "KopsMachinePool",
+							Name:       "test-kops-machine-pool-not-exists",
+							Namespace:  metav1.NamespaceDefault,
+						},
+						{
+							APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha1",
+							Kind:       "KopsMachinePool",
+							Name:       "test-kops-machine-pool",
+							Namespace:  metav1.NamespaceDefault,
+						},
+					},
+				},
+			},
+			expectedEnvironment: "testing",
+		},
+	}
+
+	RegisterFailHandler(Fail)
+	g := NewWithT(t)
+
+	err := clusterv1beta1.AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = crossec2v1beta1.SchemeBuilder.AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = securitygroupv1alpha2.AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = kinfrastructurev1alpha1.AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = kcontrolplanev1alpha1.AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			ctx := context.TODO()
+			fakeClient := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tc.k8sObjects...).Build()
+
+			reconciler := SecurityGroupReconciler{
+				Client:                      fakeClient,
+				NewEC2ClientFactory:         nil,
+				NewAutoScalingClientFactory: nil,
+			}
+
+			reconciliation := &SecurityGroupReconciliation{
+				SecurityGroupReconciler: reconciler,
+				log:                     ctrl.LoggerFrom(ctx),
+				sg:                      tc.sg,
+				ec2Client:               nil,
+				asgClient:               nil,
+			}
+
+			environment := reconciliation.getEnvironment(ctx)
+			g.Expect(environment).To(Equal(tc.expectedEnvironment))
+		})
+	}
+}
+
 func assertConditions(g *WithT, from conditions.Getter, conditions ...*clusterv1beta1.Condition) {
 	for _, condition := range conditions {
 		assertCondition(g, from, condition)

metrics/metrics.go

@@ -8,11 +8,12 @@ import (
 var (
 
 	// TODO: We could add the reason of the error as a label as well
+	// For while we only have support for this metric in the securityGroup controller
 	ReconciliationConsecutiveErrorsTotal = prometheus.NewGaugeVec(
 		prometheus.GaugeOpts{
 			Name: "custom_reconciliation_consecutive_errors_total",
-			Help: "Total number of consecutive reconciliation errors labeled by controller, cluster and environment",
-		}, []string{"controller", "sg_name", "cluster_environment"},
+			Help: "Total number of consecutive reconciliation errors labeled by controller, name of securityGroup/clustermesh and cluster environment",
+		}, []string{"controller", "object_name", "cluster_environment"},
 	)
 )