Merge pull request #756 from dmvolod/issue-755

k8s-ci-robot · web-flow · commit 214e27113bec · 2025-03-24T08:06:33.000-07:00
🐛 InfrastructureProvider waiting for core provider even though CoreProvider is ready
diff --git a/Makefile b/Makefile
@@ -280,6 +280,7 @@ test-verbose: ## Run tests with verbose settings.
 
 .PHONY: test-junit
 test-junit: $(SETUP_ENVTEST) $(GOTESTSUM) ## Run tests with verbose setting and generate a junit report
+	mkdir -p $(ARTIFACTS)
 	set +o errexit; (KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test -json ./... $(TEST_ARGS); echo $$? > $(ARTIFACTS)/junit.exitcode) | tee $(ARTIFACTS)/junit.stdout
 	$(GOTESTSUM) --junitfile $(ARTIFACTS)/junit.xml --raw-command cat $(ARTIFACTS)/junit.stdout
 	exit $$(cat $(ARTIFACTS)/junit.exitcode)
diff --git a/cmd/plugin/cmd/suite_test.go b/cmd/plugin/cmd/suite_test.go
@@ -30,7 +30,7 @@ import (
 )
 
 const (
-	waitShort = time.Second * 5
+	waitShort = time.Second * 10
 	waitLong  = time.Second * 20
 )
 
diff --git a/internal/controller/genericprovider_controller.go b/internal/controller/genericprovider_controller.go
@@ -206,7 +206,7 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene
 
 		if !res.IsZero() || err != nil {
 			// the steps are sequential, so we must be complete before progressing.
-			return res, err
+			return res, ignoreCoreProviderWaitError(err)
 		}
 	}
 
diff --git a/internal/controller/genericprovider_controller_test.go b/internal/controller/genericprovider_controller_test.go
@@ -156,7 +156,7 @@ func testDeploymentLabelValueGetter(deploymentNS, deploymentName string) func()
 	}
 }
 
-func TestConfigSecretChangesAreAppliedTotheDeployment(t *testing.T) {
+func TestConfigSecretChangesAreAppliedToTheDeployment(t *testing.T) {
 	g := NewWithT(t)
 	objs := []client.Object{}
 
@@ -203,7 +203,7 @@ func TestConfigSecretChangesAreAppliedTotheDeployment(t *testing.T) {
 		30*time.Second,
 	).Should(BeEquivalentTo("initial-value"))
 
-	t.Log("Provider deploymnet deployed")
+	t.Log("Provider deployment deployed")
 
 	configSecret.Data["CONFIGURED_VALUE"] = []byte("updated-value")
 
@@ -531,6 +531,64 @@ releaseSeries:
 	}
 }
 
+func TestProviderShouldNotBeInstalledWhenCoreProviderNotReady(t *testing.T) {
+	g := NewGomegaWithT(t)
+	coreProvider := &operatorv1.CoreProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster-api",
+		},
+		Spec: operatorv1.CoreProviderSpec{
+			ProviderSpec: operatorv1.ProviderSpec{
+				Version: "v0.0.1-incorrect",
+			},
+		},
+	}
+	controlPlaneProvider := &operatorv1.ControlPlaneProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "kubeadm",
+		},
+		Spec: operatorv1.ControlPlaneProviderSpec{
+			ProviderSpec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+			},
+		},
+	}
+
+	ns, err := env.CreateNamespace(ctx, "core-provider-not-ready")
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(ns.Name).NotTo(BeEmpty())
+
+	coreProvider.Namespace = ns.Name
+	g.Expect(env.CreateAndWait(ctx, coreProvider)).To(Succeed())
+
+	controlPlaneProvider.Namespace = ns.Name
+	g.Expect(env.CreateAndWait(ctx, controlPlaneProvider)).To(Succeed())
+
+	defer func() {
+		g.Expect(env.CleanupAndWait(ctx, []client.Object{coreProvider, controlPlaneProvider}...)).To(Succeed())
+	}()
+
+	g.Eventually(func() bool {
+		if err = env.Get(ctx, client.ObjectKeyFromObject(coreProvider), coreProvider); err != nil {
+			return false
+		}
+
+		providerInstalled := conditions.Get(coreProvider, operatorv1.ProviderInstalledCondition)
+
+		return providerInstalled != nil && providerInstalled.Status == corev1.ConditionFalse
+	}, timeout).Should(BeEquivalentTo(true))
+
+	g.Consistently(func() bool {
+		if err = env.Get(ctx, client.ObjectKeyFromObject(controlPlaneProvider), controlPlaneProvider); err != nil {
+			return false
+		}
+
+		return !conditions.IsTrue(controlPlaneProvider, operatorv1.PreflightCheckCondition) &&
+			!conditions.IsTrue(controlPlaneProvider, operatorv1.ProviderInstalledCondition) &&
+			!conditions.IsTrue(controlPlaneProvider, clusterv1.ReadyCondition)
+	}, timeout/3).Should(BeEquivalentTo(true))
+}
+
 func TestProviderConfigSecretChanges(t *testing.T) {
 	testCases := []struct {
 		name           string
@@ -846,15 +904,9 @@ func generateExpectedResultChecker(provider genericprovider.GenericProvider, spe
 			return false
 		}
 
-		for _, cond := range provider.GetStatus().Conditions {
-			if cond.Type == operatorv1.ProviderInstalledCondition {
-				if cond.Status == condStatus {
-					return true
-				}
-			}
-		}
+		condition := conditions.Get(provider, operatorv1.ProviderInstalledCondition)
 
-		return false
+		return condition != nil && condition.Status == condStatus
 	}
 }
 
diff --git a/internal/controller/phases.go b/internal/controller/phases.go
@@ -194,7 +194,7 @@ func (p *phaseReconciler) load(ctx context.Context) (reconcile.Result, error) {
 		labelSelector = p.provider.GetSpec().FetchConfig.Selector
 	}
 
-	additionalManifests, err := p.fetchAddionalManifests(ctx)
+	additionalManifests, err := p.fetchAdditionalManifests(ctx)
 	if err != nil {
 		return reconcile.Result{}, wrapPhaseError(err, "failed to load additional manifests", operatorv1.ProviderInstalledCondition)
 	}
@@ -356,7 +356,7 @@ func (p *phaseReconciler) configmapRepository(ctx context.Context, labelSelector
 	return mr, nil
 }
 
-func (p *phaseReconciler) fetchAddionalManifests(ctx context.Context) (string, error) {
+func (p *phaseReconciler) fetchAdditionalManifests(ctx context.Context) (string, error) {
 	cm := &corev1.ConfigMap{}
 
 	if p.provider.GetSpec().AdditionalManifestsRef != nil {
@@ -548,7 +548,7 @@ func (p *phaseReconciler) install(ctx context.Context) (reconcile.Result, error)
 	return reconcile.Result{}, nil
 }
 
-func (p *phaseReconciler) reportStatus(ctx context.Context) (reconcile.Result, error) {
+func (p *phaseReconciler) reportStatus(_ context.Context) (reconcile.Result, error) {
 	status := p.provider.GetStatus()
 	status.Contract = &p.contract
 	installedVersion := p.components.Version()
diff --git a/internal/controller/preflight_checks.go b/internal/controller/preflight_checks.go
@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 
@@ -46,9 +47,11 @@ var (
 	moreThanOneProviderInstanceExistsMessage     = "There is already a %s with name %s in the cluster. Only one is allowed."
 	capiVersionIncompatibilityMessage            = "CAPI operator is only compatible with %s providers, detected %s for provider %s."
 	invalidGithubTokenMessage                    = "Invalid github token, please check your github token value and its permissions" //nolint:gosec
-	waitingForCoreProviderReadyMessage           = "Waiting for the core provider to be installed."
+	waitingForCoreProviderReadyMessage           = "Waiting for the CoreProvider to be installed."
 	incorrectCoreProviderNameMessage             = "Incorrect CoreProvider name: %s. It should be %s"
 	unsupportedProviderDowngradeMessage          = "Downgrade is not supported for provider %s"
+
+	errCoreProviderWait = errors.New(waitingForCoreProviderReadyMessage)
 )
 
 // preflightChecks performs preflight checks before installing provider.
@@ -121,10 +124,10 @@ func preflightChecks(ctx context.Context, c client.Client, provider genericprovi
 		}
 
 		if token, ok := secret.Data[configclient.GitHubTokenVariable]; ok {
-			client := github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource(
+			githubClient := github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource(
 				&oauth2.Token{AccessToken: string(token)},
 			)))
-			if _, _, err := client.Organizations.List(ctx, "kubernetes-sigs", nil); err != nil {
+			if _, _, err := githubClient.Organizations.List(ctx, "kubernetes-sigs", nil); err != nil {
 				conditions.Set(provider, conditions.FalseCondition(
 					operatorv1.PreflightCheckCondition,
 					operatorv1.InvalidGithubTokenReason,
@@ -190,7 +193,7 @@ func preflightChecks(ctx context.Context, c client.Client, provider genericprovi
 				"%s", waitingForCoreProviderReadyMessage,
 			))
 
-			return nil
+			return errCoreProviderWait
 		}
 	}
 
@@ -258,6 +261,15 @@ func coreProviderIsReady(ctx context.Context, c client.Client) (bool, error) {
 	return false, nil
 }
 
+// ignoreCoreProviderWaitError ignores errCoreProviderWait error.
+func ignoreCoreProviderWaitError(err error) error {
+	if errors.Is(err, errCoreProviderWait) {
+		return nil
+	}
+
+	return err
+}
+
 // isPredefinedProvider checks if a given provider is known for Cluster API.
 // The list of known providers can be found here:
 // https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/client/config/providers_client.go
diff --git a/internal/controller/preflight_checks_test.go b/internal/controller/preflight_checks_test.go
@@ -241,6 +241,61 @@ func TestPreflightChecks(t *testing.T) {
 			},
 			providerList: &operatorv1.InfrastructureProviderList{},
 		},
+		{
+			name:          "only one infra provider exists but core provider is not ready, preflight check failed",
+			expectedError: true,
+			providers: []operatorv1.GenericProvider{
+				&operatorv1.InfrastructureProvider{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "aws",
+						Namespace: namespaceName1,
+					},
+					TypeMeta: metav1.TypeMeta{
+						Kind:       "InfrastructureProvider",
+						APIVersion: "operator.cluster.x-k8s.io/v1alpha1",
+					},
+					Spec: operatorv1.InfrastructureProviderSpec{
+						ProviderSpec: operatorv1.ProviderSpec{
+							Version: "v1.0.0",
+						},
+					},
+				},
+				&operatorv1.CoreProvider{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "cluster-api",
+						Namespace: namespaceName2,
+					},
+					TypeMeta: metav1.TypeMeta{
+						Kind:       "CoreProvider",
+						APIVersion: "operator.cluster.x-k8s.io/v1alpha4",
+					},
+					Spec: operatorv1.CoreProviderSpec{
+						ProviderSpec: operatorv1.ProviderSpec{
+							Version: "v1.0.0",
+						},
+					},
+					Status: operatorv1.CoreProviderStatus{
+						ProviderStatus: operatorv1.ProviderStatus{
+							Conditions: []clusterv1.Condition{
+								{
+									Type:               clusterv1.ReadyCondition,
+									Status:             corev1.ConditionFalse,
+									LastTransitionTime: metav1.Now(),
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedCondition: clusterv1.Condition{
+				Type:     operatorv1.PreflightCheckCondition,
+				Status:   corev1.ConditionFalse,
+				Reason:   operatorv1.WaitingForCoreProviderReadyReason,
+				Severity: clusterv1.ConditionSeverityInfo,
+				Message:  "Waiting for the CoreProvider to be installed.",
+			},
+			providerList: &operatorv1.InfrastructureProviderList{},
+		},
 		{
 			name: "two different infra providers exist in same namespaces, preflight check passed",
 			providers: []operatorv1.GenericProvider{
@@ -509,7 +564,7 @@ func TestPreflightChecks(t *testing.T) {
 			providerList: &operatorv1.InfrastructureProviderList{},
 		},
 		{
-			name: "predefined Core Provider without fetch config, preflight check passed",
+			name: "predefined core provider without fetch config, preflight check passed",
 			providers: []operatorv1.GenericProvider{
 				&operatorv1.CoreProvider{
 					ObjectMeta: metav1.ObjectMeta{
@@ -601,13 +656,13 @@ func TestPreflightChecks(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			gs := NewWithT(t)
 
-			fakeclient := fake.NewClientBuilder().WithObjects().Build()
+			fakeClient := fake.NewClientBuilder().WithObjects().Build()
 
 			for _, c := range tc.providers {
-				gs.Expect(fakeclient.Create(ctx, c)).To(Succeed())
+				gs.Expect(fakeClient.Create(ctx, c)).To(Succeed())
 			}
 
-			err := preflightChecks(context.Background(), fakeclient, tc.providers[0], tc.providerList)
+			err := preflightChecks(context.Background(), fakeClient, tc.providers[0], tc.providerList)
 			if tc.expectedError {
 				gs.Expect(err).To(HaveOccurred())
 			} else {
@@ -700,11 +755,11 @@ func TestPreflightChecksUpgradesDowngrades(t *testing.T) {
 				},
 			}
 
-			fakeclient := fake.NewClientBuilder().WithObjects().Build()
+			fakeClient := fake.NewClientBuilder().WithObjects().Build()
 
-			gs.Expect(fakeclient.Create(ctx, provider)).To(Succeed())
+			gs.Expect(fakeClient.Create(ctx, provider)).To(Succeed())
 
-			err := preflightChecks(context.Background(), fakeclient, provider, &operatorv1.CoreProviderList{})
+			err := preflightChecks(context.Background(), fakeClient, provider, &operatorv1.CoreProviderList{})
 			if tc.expectedError {
 				gs.Expect(err).To(HaveOccurred())
 			} else {
diff --git a/test/e2e/air_gapped_test.go b/test/e2e/air_gapped_test.go

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ import (`
`30`	`30`	`)`
`31`	`31`
`32`	`32`	`const (`
`33`		`- waitShort = time.Second * 5`
	`33`	`+ waitShort = time.Second * 10`
`34`	`34`	`waitLong = time.Second * 20`
`35`	`35`	`)`
`36`	`36`
Original file line number	Diff line number	Diff line change
`@@ -206,7 +206,7 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene`
`206`	`206`
`207`	`207`	`if !res.IsZero() \|\| err != nil {`
`208`	`208`	`// the steps are sequential, so we must be complete before progressing.`
`209`		`- return res, err`
	`209`	`+ return res, ignoreCoreProviderWaitError(err)`
`210`	`210`	`}`
`211`	`211`	`}`
`212`	`212`
Original file line number	Diff line number	Diff line change
`@@ -194,7 +194,7 @@ func (p *phaseReconciler) load(ctx context.Context) (reconcile.Result, error) {`
`194`	`194`	`labelSelector = p.provider.GetSpec().FetchConfig.Selector`
`195`	`195`	`}`
`196`	`196`
`197`		`- additionalManifests, err := p.fetchAddionalManifests(ctx)`
	`197`	`+ additionalManifests, err := p.fetchAdditionalManifests(ctx)`
`198`	`198`	`if err != nil {`
`199`	`199`	`return reconcile.Result{}, wrapPhaseError(err, "failed to load additional manifests", operatorv1.ProviderInstalledCondition)`
`200`	`200`	`}`
`@@ -356,7 +356,7 @@ func (p *phaseReconciler) configmapRepository(ctx context.Context, labelSelector`
`356`	`356`	`return mr, nil`
`357`	`357`	`}`
`358`	`358`
`359`		`-func (p *phaseReconciler) fetchAddionalManifests(ctx context.Context) (string, error) {`
	`359`	`+func (p *phaseReconciler) fetchAdditionalManifests(ctx context.Context) (string, error) {`
`360`	`360`	`cm := &corev1.ConfigMap{}`
`361`	`361`
`362`	`362`	`if p.provider.GetSpec().AdditionalManifestsRef != nil {`
`@@ -548,7 +548,7 @@ func (p *phaseReconciler) install(ctx context.Context) (reconcile.Result, error)`
`548`	`548`	`return reconcile.Result{}, nil`
`549`	`549`	`}`
`550`	`550`
`551`		`-func (p *phaseReconciler) reportStatus(ctx context.Context) (reconcile.Result, error) {`
	`551`	`+func (p *phaseReconciler) reportStatus(_ context.Context) (reconcile.Result, error) {`
`552`	`552`	`status := p.provider.GetStatus()`
`553`	`553`	`status.Contract = &p.contract`
`554`	`554`	`installedVersion := p.components.Version()`