Skip to content

Commit b3f81bc

Browse files
dmvoloddmvolod
committed
🐛 InfrastructureProvider waiting for core provider even though CoreProvider is ready
1 parent e2a065e commit b3f81bc

File tree

6 files changed

+175
-56
lines changed

6 files changed

+175
-56
lines changed

‎internal/controller/genericprovider_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene
206206

207207
if !res.IsZero() || err != nil {
208208
// the steps are sequential, so we must be complete before progressing.
209-
return res, err
209+
return res, ignoreCoreProviderWaitError(err)
210210
}
211211
}
212212

‎internal/controller/genericprovider_controller_test.go‎

Lines changed: 62 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func testDeploymentLabelValueGetter(deploymentNS, deploymentName string) func()
156156
}
157157
}
158158

159-
func TestConfigSecretChangesAreAppliedTotheDeployment(t *testing.T) {
159+
func TestConfigSecretChangesAreAppliedToTheDeployment(t *testing.T) {
160160
g := NewWithT(t)
161161
objs := []client.Object{}
162162

@@ -203,7 +203,7 @@ func TestConfigSecretChangesAreAppliedTotheDeployment(t *testing.T) {
203203
30*time.Second,
204204
).Should(BeEquivalentTo("initial-value"))
205205

206-
t.Log("Provider deploymnet deployed")
206+
t.Log("Provider deployment deployed")
207207

208208
configSecret.Data["CONFIGURED_VALUE"] = []byte("updated-value")
209209

@@ -531,6 +531,64 @@ releaseSeries:
531531
}
532532
}
533533

534+
func TestProviderShouldNotBeInstalledWhenCoreProviderNotReady(t *testing.T) {
535+
g := NewGomegaWithT(t)
536+
coreProvider := &operatorv1.CoreProvider{
537+
ObjectMeta: metav1.ObjectMeta{
538+
Name: "cluster-api",
539+
},
540+
Spec: operatorv1.CoreProviderSpec{
541+
ProviderSpec: operatorv1.ProviderSpec{
542+
Version: "v0.0.1-incorrect",
543+
},
544+
},
545+
}
546+
controlPlaneProvider := &operatorv1.ControlPlaneProvider{
547+
ObjectMeta: metav1.ObjectMeta{
548+
Name: "kubeadm",
549+
},
550+
Spec: operatorv1.ControlPlaneProviderSpec{
551+
ProviderSpec: operatorv1.ProviderSpec{
552+
Version: testCurrentVersion,
553+
},
554+
},
555+
}
556+
557+
ns, err := env.CreateNamespace(ctx, "core-provider-not-ready")
558+
g.Expect(err).ToNot(HaveOccurred())
559+
g.Expect(ns.Name).NotTo(BeEmpty())
560+
561+
coreProvider.Namespace = ns.Name
562+
g.Expect(env.CreateAndWait(ctx, coreProvider)).To(Succeed())
563+
564+
controlPlaneProvider.Namespace = ns.Name
565+
g.Expect(env.CreateAndWait(ctx, controlPlaneProvider)).To(Succeed())
566+
567+
defer func() {
568+
g.Expect(env.CleanupAndWait(ctx, []client.Object{coreProvider, controlPlaneProvider}...)).To(Succeed())
569+
}()
570+
571+
g.Eventually(func() bool {
572+
if err = env.Get(ctx, client.ObjectKeyFromObject(coreProvider), coreProvider); err != nil {
573+
return false
574+
}
575+
576+
providerInstalled := conditions.Get(coreProvider, operatorv1.ProviderInstalledCondition)
577+
578+
return providerInstalled != nil && providerInstalled.Status == corev1.ConditionFalse
579+
}, timeout).Should(BeEquivalentTo(true))
580+
581+
g.Consistently(func() bool {
582+
if err = env.Get(ctx, client.ObjectKeyFromObject(controlPlaneProvider), controlPlaneProvider); err != nil {
583+
return false
584+
}
585+
586+
return !conditions.IsTrue(controlPlaneProvider, operatorv1.PreflightCheckCondition) &&
587+
!conditions.IsTrue(controlPlaneProvider, operatorv1.ProviderInstalledCondition) &&
588+
!conditions.IsTrue(controlPlaneProvider, clusterv1.ReadyCondition)
589+
}, timeout/3).Should(BeEquivalentTo(true))
590+
}
591+
534592
func TestProviderConfigSecretChanges(t *testing.T) {
535593
testCases := []struct {
536594
name string
@@ -846,15 +904,9 @@ func generateExpectedResultChecker(provider genericprovider.GenericProvider, spe
846904
return false
847905
}
848906

849-
for _, cond := range provider.GetStatus().Conditions {
850-
if cond.Type == operatorv1.ProviderInstalledCondition {
851-
if cond.Status == condStatus {
852-
return true
853-
}
854-
}
855-
}
907+
condition := conditions.Get(provider, operatorv1.ProviderInstalledCondition)
856908

857-
return false
909+
return condition != nil && condition.Status == condStatus
858910
}
859911
}
860912

‎internal/controller/phases.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func (p *phaseReconciler) load(ctx context.Context) (reconcile.Result, error) {
194194
labelSelector = p.provider.GetSpec().FetchConfig.Selector
195195
}
196196

197-
additionalManifests, err := p.fetchAddionalManifests(ctx)
197+
additionalManifests, err := p.fetchAdditionalManifests(ctx)
198198
if err != nil {
199199
return reconcile.Result{}, wrapPhaseError(err, "failed to load additional manifests", operatorv1.ProviderInstalledCondition)
200200
}
@@ -356,7 +356,7 @@ func (p *phaseReconciler) configmapRepository(ctx context.Context, labelSelector
356356
return mr, nil
357357
}
358358

359-
func (p *phaseReconciler) fetchAddionalManifests(ctx context.Context) (string, error) {
359+
func (p *phaseReconciler) fetchAdditionalManifests(ctx context.Context) (string, error) {
360360
cm := &corev1.ConfigMap{}
361361

362362
if p.provider.GetSpec().AdditionalManifestsRef != nil {
@@ -548,7 +548,7 @@ func (p *phaseReconciler) install(ctx context.Context) (reconcile.Result, error)
548548
return reconcile.Result{}, nil
549549
}
550550

551-
func (p *phaseReconciler) reportStatus(ctx context.Context) (reconcile.Result, error) {
551+
func (p *phaseReconciler) reportStatus(_ context.Context) (reconcile.Result, error) {
552552
status := p.provider.GetStatus()
553553
status.Contract = &p.contract
554554
installedVersion := p.components.Version()

‎internal/controller/preflight_checks.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package controller
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
2223
"os"
2324

@@ -46,9 +47,11 @@ var (
4647
moreThanOneProviderInstanceExistsMessage = "There is already a %s with name %s in the cluster. Only one is allowed."
4748
capiVersionIncompatibilityMessage = "CAPI operator is only compatible with %s providers, detected %s for provider %s."
4849
invalidGithubTokenMessage = "Invalid github token, please check your github token value and its permissions" //nolint:gosec
49-
waitingForCoreProviderReadyMessage = "Waiting for the core provider to be installed."
50+
waitingForCoreProviderReadyMessage = "Waiting for the CoreProvider to be installed."
5051
incorrectCoreProviderNameMessage = "Incorrect CoreProvider name: %s. It should be %s"
5152
unsupportedProviderDowngradeMessage = "Downgrade is not supported for provider %s"
53+
54+
errCoreProviderWait = errors.New(waitingForCoreProviderReadyMessage)
5255
)
5356

5457
// preflightChecks performs preflight checks before installing provider.
@@ -121,10 +124,10 @@ func preflightChecks(ctx context.Context, c client.Client, provider genericprovi
121124
}
122125

123126
if token, ok := secret.Data[configclient.GitHubTokenVariable]; ok {
124-
client := github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource(
127+
githubClient := github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource(
125128
&oauth2.Token{AccessToken: string(token)},
126129
)))
127-
if _, _, err := client.Organizations.List(ctx, "kubernetes-sigs", nil); err != nil {
130+
if _, _, err := githubClient.Organizations.List(ctx, "kubernetes-sigs", nil); err != nil {
128131
conditions.Set(provider, conditions.FalseCondition(
129132
operatorv1.PreflightCheckCondition,
130133
operatorv1.InvalidGithubTokenReason,
@@ -190,7 +193,7 @@ func preflightChecks(ctx context.Context, c client.Client, provider genericprovi
190193
"%s", waitingForCoreProviderReadyMessage,
191194
))
192195

193-
return nil
196+
return errCoreProviderWait
194197
}
195198
}
196199

@@ -258,6 +261,15 @@ func coreProviderIsReady(ctx context.Context, c client.Client) (bool, error) {
258261
return false, nil
259262
}
260263

264+
// ignoreCoreProviderWaitError ignores errCoreProviderWait error.
265+
func ignoreCoreProviderWaitError(err error) error {
266+
if errors.Is(err, errCoreProviderWait) {
267+
return nil
268+
}
269+
270+
return err
271+
}
272+
261273
// isPredefinedProvider checks if a given provider is known for Cluster API.
262274
// The list of known providers can be found here:
263275
// https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/client/config/providers_client.go

‎internal/controller/preflight_checks_test.go‎

Lines changed: 62 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,61 @@ func TestPreflightChecks(t *testing.T) {
241241
},
242242
providerList: &operatorv1.InfrastructureProviderList{},
243243
},
244+
{
245+
name: "only one infra provider exists but core provider is not ready, preflight check failed",
246+
expectedError: true,
247+
providers: []operatorv1.GenericProvider{
248+
&operatorv1.InfrastructureProvider{
249+
ObjectMeta: metav1.ObjectMeta{
250+
Name: "aws",
251+
Namespace: namespaceName1,
252+
},
253+
TypeMeta: metav1.TypeMeta{
254+
Kind: "InfrastructureProvider",
255+
APIVersion: "operator.cluster.x-k8s.io/v1alpha1",
256+
},
257+
Spec: operatorv1.InfrastructureProviderSpec{
258+
ProviderSpec: operatorv1.ProviderSpec{
259+
Version: "v1.0.0",
260+
},
261+
},
262+
},
263+
&operatorv1.CoreProvider{
264+
ObjectMeta: metav1.ObjectMeta{
265+
Name: "cluster-api",
266+
Namespace: namespaceName2,
267+
},
268+
TypeMeta: metav1.TypeMeta{
269+
Kind: "CoreProvider",
270+
APIVersion: "operator.cluster.x-k8s.io/v1alpha4",
271+
},
272+
Spec: operatorv1.CoreProviderSpec{
273+
ProviderSpec: operatorv1.ProviderSpec{
274+
Version: "v1.0.0",
275+
},
276+
},
277+
Status: operatorv1.CoreProviderStatus{
278+
ProviderStatus: operatorv1.ProviderStatus{
279+
Conditions: []clusterv1.Condition{
280+
{
281+
Type: clusterv1.ReadyCondition,
282+
Status: corev1.ConditionFalse,
283+
LastTransitionTime: metav1.Now(),
284+
},
285+
},
286+
},
287+
},
288+
},
289+
},
290+
expectedCondition: clusterv1.Condition{
291+
Type: operatorv1.PreflightCheckCondition,
292+
Status: corev1.ConditionFalse,
293+
Reason: operatorv1.WaitingForCoreProviderReadyReason,
294+
Severity: clusterv1.ConditionSeverityInfo,
295+
Message: "Waiting for the CoreProvider to be installed.",
296+
},
297+
providerList: &operatorv1.InfrastructureProviderList{},
298+
},
244299
{
245300
name: "two different infra providers exist in same namespaces, preflight check passed",
246301
providers: []operatorv1.GenericProvider{
@@ -509,7 +564,7 @@ func TestPreflightChecks(t *testing.T) {
509564
providerList: &operatorv1.InfrastructureProviderList{},
510565
},
511566
{
512-
name: "predefined Core Provider without fetch config, preflight check passed",
567+
name: "predefined core provider without fetch config, preflight check passed",
513568
providers: []operatorv1.GenericProvider{
514569
&operatorv1.CoreProvider{
515570
ObjectMeta: metav1.ObjectMeta{
@@ -601,13 +656,13 @@ func TestPreflightChecks(t *testing.T) {
601656
t.Run(tc.name, func(t *testing.T) {
602657
gs := NewWithT(t)
603658

604-
fakeclient := fake.NewClientBuilder().WithObjects().Build()
659+
fakeClient := fake.NewClientBuilder().WithObjects().Build()
605660

606661
for _, c := range tc.providers {
607-
gs.Expect(fakeclient.Create(ctx, c)).To(Succeed())
662+
gs.Expect(fakeClient.Create(ctx, c)).To(Succeed())
608663
}
609664

610-
err := preflightChecks(context.Background(), fakeclient, tc.providers[0], tc.providerList)
665+
err := preflightChecks(context.Background(), fakeClient, tc.providers[0], tc.providerList)
611666
if tc.expectedError {
612667
gs.Expect(err).To(HaveOccurred())
613668
} else {
@@ -700,11 +755,11 @@ func TestPreflightChecksUpgradesDowngrades(t *testing.T) {
700755
},
701756
}
702757

703-
fakeclient := fake.NewClientBuilder().WithObjects().Build()
758+
fakeClient := fake.NewClientBuilder().WithObjects().Build()
704759

705-
gs.Expect(fakeclient.Create(ctx, provider)).To(Succeed())
760+
gs.Expect(fakeClient.Create(ctx, provider)).To(Succeed())
706761

707-
err := preflightChecks(context.Background(), fakeclient, provider, &operatorv1.CoreProviderList{})
762+
err := preflightChecks(context.Background(), fakeClient, provider, &operatorv1.CoreProviderList{})
708763
if tc.expectedError {
709764
gs.Expect(err).To(HaveOccurred())
710765
} else {

0 commit comments

Comments
 (0)