Merge pull request #470 from Danil-Grigorev/always-patch-status-capi-…

…provider

🌱 Ensure controller always patches CAPIProvider status

internal/controllers/capiprovider_controller.go

@@ -74,12 +74,18 @@ func (r *CAPIProviderReconciler) sync(ctx context.Context, capiProvider *turtles
 		sync.NewSecretMapperSync(ctx, r.Client, capiProvider),
 	)
 
+	defer r.patchStatus(ctx, capiProvider, &err)
+
 	if err := s.Sync(ctx); client.IgnoreNotFound(err) != nil {
 		return ctrl.Result{}, err
 	}
 	defer s.Apply(ctx, &err)
 
-	return ctrl.Result{}, sync.PatchStatus(ctx, r.Client, capiProvider)
+	return ctrl.Result{}, nil
+}
+
+func (r *CAPIProviderReconciler) patchStatus(ctx context.Context, capiProvider *turtlesv1.CAPIProvider, err *error) {
+	*err = sync.PatchStatus(ctx, r.Client, capiProvider)
 }
 
 // SetupWithManager sets up the controller with the Manager.

internal/controllers/capiprovider_controller_test.go

@@ -25,6 +25,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	. "sigs.k8s.io/controller-runtime/pkg/envtest/komega"
 )
@@ -123,7 +124,7 @@ var _ = Describe("Reconcile CAPIProvider", func() {
 			},
 		})).To(Succeed())
 
-		Expect(cl.Create(ctx, &corev1.Secret{
+		secret := &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:         "cc",
 				GenerateName: "cc-",
@@ -136,7 +137,8 @@ var _ = Describe("Reconcile CAPIProvider", func() {
 			StringData: map[string]string{
 				"digitaloceancredentialConfig-accessToken": "token",
 			},
-		})).To(Succeed())
+		}
+		Expect(cl.Create(ctx, secret)).To(Succeed())
 
 		Eventually(Update(provider, func() {
 			provider.Spec.Features = &turtlesv1.Features{MachinePool: true}
@@ -153,5 +155,36 @@ var _ = Describe("Reconcile CAPIProvider", func() {
 			"DO_B64ENCODED_CREDENTIALS": []byte("dG9rZW4="),
 		})))
 
+		Eventually(func(g Gomega) {
+			g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(provider), provider)).ToNot(HaveOccurred())
+			g.Expect(conditions.IsTrue(provider, turtlesv1.RancherCredentialsSecretCondition))
+		}).Should(Succeed())
+	})
+
+	It("Should reflect missing infrastructure digitalocean provider credential secret in the status", func() {
+		provider := &turtlesv1.CAPIProvider{ObjectMeta: metav1.ObjectMeta{
+			Name:      "digitalocean",
+			Namespace: ns.Name,
+		}, Spec: turtlesv1.CAPIProviderSpec{
+			Type: turtlesv1.Infrastructure,
+		}}
+		Expect(cl.Create(ctx, provider)).ToNot(HaveOccurred())
+
+		doSecret := objectFromKey(client.ObjectKeyFromObject(provider), &corev1.Secret{})
+		Eventually(testEnv.GetAs(provider, &operatorv1.InfrastructureProvider{})).ShouldNot(BeNil())
+		Eventually(testEnv.GetAs(provider, doSecret)).ShouldNot(BeNil())
+
+		Eventually(Update(provider, func() {
+			provider.Spec.Features = &turtlesv1.Features{MachinePool: true}
+			provider.Spec.Credentials = &turtlesv1.Credentials{
+				RancherCloudCredential: "some-missing",
+			}
+		})).Should(Succeed())
+
+		Eventually(func(g Gomega) {
+			g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(provider), provider)).ToNot(HaveOccurred())
+			g.Expect(conditions.IsFalse(provider, turtlesv1.RancherCredentialsSecretCondition))
+			g.Expect(conditions.GetMessage(provider, turtlesv1.RancherCredentialsSecretCondition)).To(Equal("Credential keys missing: key not found: digitaloceancredentialConfig-accessToken"))
+		}).Should(Succeed())
 	})
 })

internal/controllers/import_controller_test.go

@@ -138,7 +138,7 @@ var _ = Describe("reconcile CAPI Cluster", func() {
 			})
 			g.Expect(err).ToNot(HaveOccurred())
 			g.Expect(res.RequeueAfter).To(Equal(defaultRequeueDuration))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should reconcile a CAPI cluster when rancher cluster doesn't exist", func() {

internal/sync/provider_sync_test.go

@@ -138,7 +138,7 @@ var _ = Describe("Provider sync", func() {
 			g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(capiProvider), capiProvider)).To(Succeed())
 			g.Expect(conditions.Get(capiProvider, turtlesv1.LastAppliedConfigurationTime)).ToNot(BeNil())
 			g.Expect(conditions.Get(capiProvider, turtlesv1.LastAppliedConfigurationTime).LastTransitionTime.Second()).To(Equal(appliedCondition.LastTransitionTime.Second()))
-		})
+		}).Should(Succeed())
 
 		s := sync.NewProviderSync(testEnv, capiProvider)
 
@@ -158,6 +158,7 @@ var _ = Describe("Provider sync", func() {
 				appliedCondition.LastTransitionTime.Time)).To(BeTrue())
 		}).Should(Succeed())
 
+		Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(capiProvider), capiProvider)).To(Succeed())
 		condition := conditions.Get(capiProvider, turtlesv1.LastAppliedConfigurationTime)
 
 		Consistently(func(g Gomega) {
@@ -167,7 +168,7 @@ var _ = Describe("Provider sync", func() {
 			s.Apply(ctx, &err)
 			g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(capiProvider), capiProvider)).To(Succeed())
 			g.Expect(conditions.Get(capiProvider, turtlesv1.LastAppliedConfigurationTime)).To(Equal(condition))
-		}, 5*time.Second)
+		}, 5*time.Second).Should(Succeed())
 	})
 
 	It("Should individually sync every provider", func() {
@@ -208,7 +209,7 @@ var _ = Describe("Provider sync", func() {
 			g.Expect(capiProviderDuplicate.Status.Conditions).To(HaveLen(1))
 			g.Expect(conditions.IsTrue(capiProviderDuplicate, turtlesv1.LastAppliedConfigurationTime)).To(BeTrue())
 			g.Expect(conditions.Get(capiProviderDuplicate, turtlesv1.LastAppliedConfigurationTime).LastTransitionTime.Second()).To(Equal(time.Now().UTC().Second()))
-		})
+		}).Should(Succeed())
 
 		// Provider manifest should be created and phase set to provisioning
 		Eventually(func(g Gomega) {

internal/sync/secret_mapper_sync.go

@@ -98,7 +98,7 @@ var (
 )
 
 var (
-	missingKey    = "Credential keys missing: %w"
+	missingKey    = "Credential keys missing: %s"
 	missingSource = "Rancher Credentials secret named %s was not located"
 )
 

internal/sync/secret_mapper_sync_test.go

@@ -146,7 +146,7 @@ var _ = Describe("SecretMapperSync get", func() {
 			g.Expect(syncer.Get(context.Background())).ToNot(HaveOccurred())
 			g.Expect(syncer.RancherSecret.Annotations).To(Equal(rancherSecret.Annotations))
 			g.Expect(syncer.RancherSecret.Name).To(Equal(rancherSecret.Name))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should ignore similarly named secret for a different driver", func() {
@@ -165,12 +165,13 @@ var _ = Describe("SecretMapperSync get", func() {
 		}
 
 		Eventually(func(g Gomega) {
-			g.Expect(syncer.Get(context.Background())).To(HaveOccurred())
+			err = syncer.Get(context.Background())
+			g.Expect(err).To(HaveOccurred())
 			g.Expect(err.Error()).To(ContainSubstring("unable to locate rancher secret with name"))
 			g.Expect(conditions.Get(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).ToNot(BeNil())
 			g.Expect(conditions.IsFalse(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(BeTrue())
 			g.Expect(conditions.GetMessage(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(Equal("Rancher Credentials secret named test-rancher-secret was not located"))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should get the source Rancher secret pointed by ref", func() {
@@ -190,7 +191,7 @@ var _ = Describe("SecretMapperSync get", func() {
 			g.Expect(syncer.Get(context.Background())).NotTo(HaveOccurred())
 			g.Expect(syncer.RancherSecret.Name).To(Equal(customRancherSecret.Name))
 			g.Expect(syncer.RancherSecret.Namespace).To(Equal(customRancherSecret.Namespace))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should handle unexisting secret pointer by ref", func() {
@@ -200,18 +201,19 @@ var _ = Describe("SecretMapperSync get", func() {
 		}}
 		Expect(testEnv.Client.Create(ctx, secret)).To(Succeed())
 
-		syncer := sync.SecretMapperSync{
+		syncer := &sync.SecretMapperSync{
 			SecretSync:    sync.NewSecretSync(testEnv.Client, capiProviderWithRancherRef).(*sync.SecretSync),
 			RancherSecret: sync.SecretMapperSync{}.GetSecret(capiProviderWithRancherRef),
 		}
 
 		Eventually(func(g Gomega) {
-			g.Expect(syncer.Get(context.Background())).NotTo(HaveOccurred())
+			err = syncer.Get(context.Background())
+			g.Expect(err).To(HaveOccurred())
 			g.Expect(err.Error()).To(ContainSubstring("unable to locate rancher secret with name"))
 			g.Expect(conditions.Get(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).ToNot(BeNil())
 			g.Expect(conditions.IsFalse(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(BeTrue())
 			g.Expect(conditions.GetMessage(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(Equal(fmt.Sprintf("Rancher Credentials secret named %s:secret-name was not located", ns.Name)))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should handle when the source Rancher secret is not found", func() {
@@ -227,12 +229,13 @@ var _ = Describe("SecretMapperSync get", func() {
 		}
 
 		Eventually(func(g Gomega) {
-			g.Expect(syncer.Get(context.Background())).To(HaveOccurred())
+			err = syncer.Get(context.Background())
+			g.Expect(err).To(HaveOccurred())
 			g.Expect(err.Error()).To(ContainSubstring("unable to locate rancher secret with name"))
 			g.Expect(conditions.Get(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).ToNot(BeNil())
 			g.Expect(conditions.IsFalse(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(BeTrue())
 			g.Expect(conditions.GetMessage(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(Equal("Rancher Credentials secret named test-rancher-secret was not located"))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should not error when the destination secret is not created yet", func() {
@@ -247,7 +250,7 @@ var _ = Describe("SecretMapperSync get", func() {
 			g.Expect(syncer.Get(context.Background())).ToNot(HaveOccurred())
 			g.Expect(syncer.RancherSecret.Annotations).To(Equal(rancherSecret.Annotations))
 			g.Expect(syncer.RancherSecret.Name).To(Equal(rancherSecret.Name))
-		})
+		}).Should(Succeed())
 	})
 
 	It("should point to the right initial secret", func() {
@@ -275,7 +278,7 @@ var _ = Describe("SecretMapperSync get", func() {
 		Eventually(func(g Gomega) {
 			g.Expect(syncer.Sync(context.Background())).To(BeNil())
 			g.Expect(syncer.SecretSync.Secret.StringData).To(HaveLen(0))
-		})
+		}).Should(Succeed())
 	})
 
 	It("provider requirements azure", func() {
@@ -301,7 +304,7 @@ var _ = Describe("SecretMapperSync get", func() {
 				"AZURE_TENANT_ID":           "",
 				"AZURE_SUBSCRIPTION_ID_B64": "",
 			}))
-		})
+		}).Should(Succeed())
 	})
 
 	It("provider requirements aws", func() {
@@ -323,7 +326,7 @@ var _ = Describe("SecretMapperSync get", func() {
 				"AWS_ACCESS_KEY_ID":          "",
 				"AWS_SECRET_ACCESS_KEY":      "",
 			}))
-		})
+		}).Should(Succeed())
 	})
 
 	It("provider requirements gcp", func() {
@@ -342,7 +345,7 @@ var _ = Describe("SecretMapperSync get", func() {
 			g.Expect(syncer.Secret.StringData).To(Equal(map[string]string{
 				"GCP_B64ENCODED_CREDENTIALS": "",
 			}))
-		})
+		}).Should(Succeed())
 	})
 	It("provider requirements digitalocean", func() {
 		capiProvider.Spec.Name = "digitalocean"
@@ -361,7 +364,7 @@ var _ = Describe("SecretMapperSync get", func() {
 				"DO_B64ENCODED_CREDENTIALS": "",
 				"DIGITALOCEAN_ACCESS_TOKEN": "",
 			}))
-		})
+		}).Should(Succeed())
 	})
 	It("provider requirements vsphere", func() {
 		capiProvider.Spec.Name = "vsphere"
@@ -379,7 +382,7 @@ var _ = Describe("SecretMapperSync get", func() {
 				"VSPHERE_PASSWORD": "",
 				"VSPHERE_USERNAME": "",
 			}))
-		})
+		}).Should(Succeed())
 	})
 
 	It("prepare aws secret", func() {
@@ -407,6 +410,6 @@ var _ = Describe("SecretMapperSync get", func() {
 
 			g.Expect(conditions.Get(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).ToNot(BeNil())
 			g.Expect(conditions.IsTrue(syncer.Source, turtlesv1.RancherCredentialsSecretCondition)).To(BeTrue())
-		})
+		}).Should(Succeed())
 	})
 })