From 9c71c67182c8189314156d7ef034f3b49ad5617f Mon Sep 17 00:00:00 2001 From: Danil Grigorev Date: Wed, 6 Mar 2024 23:33:18 +0100 Subject: [PATCH 1/2] Collect correct manifest config map per provider while performing upgrade Clusterctl upgrade logic validates other installed providers, while perfoming upgrade for version compatibility. Operator stores this data in a ConfigMap to allow air-gapped support. This ensures we fetch correct configMap first, to validate version against correct metadata.yaml. Signed-off-by: Danil Grigorev --- internal/controller/manifests_downloader.go | 34 +++++++++++++----- internal/controller/phases.go | 31 ++++++++++++++-- util/util.go | 40 +++++++++++++++++++++ 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/internal/controller/manifests_downloader.go b/internal/controller/manifests_downloader.go index 5174ed875..dc29d190e 100644 --- a/internal/controller/manifests_downloader.go +++ b/internal/controller/manifests_downloader.go @@ -66,7 +66,7 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu MatchLabels: p.prepareConfigMapLabels(), } - exists, err := p.checkConfigMapExists(ctx, labelSelector) + exists, err := p.checkConfigMapExists(ctx, labelSelector, p.provider.GetNamespace()) if err != nil { return reconcile.Result{}, wrapPhaseError(err, "failed to check that config map with manifests exists", operatorv1.ProviderInstalledCondition) } @@ -123,11 +123,11 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu } // checkConfigMapExists checks if a config map exists in Kubernetes with the given LabelSelector. -func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelector metav1.LabelSelector) (bool, error) { +func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelector metav1.LabelSelector, namespace string) (bool, error) { labelSet := labels.Set(labelSelector.MatchLabels) listOpts := []client.ListOption{ client.MatchingLabelsSelector{Selector: labels.SelectorFromSet(labelSet)}, - client.InNamespace(p.provider.GetNamespace()), + client.InNamespace(namespace), } var configMapList corev1.ConfigMapList @@ -145,12 +145,7 @@ func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelecto // prepareConfigMapLabels returns labels that identify a config map with downloaded manifests. func (p *phaseReconciler) prepareConfigMapLabels() map[string]string { - return map[string]string{ - configMapVersionLabel: p.provider.GetSpec().Version, - configMapTypeLabel: p.provider.GetType(), - configMapNameLabel: p.provider.GetName(), - operatorManagedLabel: "true", - } + return providerLabels(p.provider) } // createManifestsConfigMap creates a config map with downloaded manifests. @@ -210,6 +205,27 @@ func (p *phaseReconciler) createManifestsConfigMap(ctx context.Context, metadata return nil } +func providerLabelSelector(provider operatorv1.GenericProvider) *metav1.LabelSelector { + // Replace label selector if user wants to use custom config map + if provider.GetSpec().FetchConfig != nil && provider.GetSpec().FetchConfig.Selector != nil { + return provider.GetSpec().FetchConfig.Selector + } + + return &metav1.LabelSelector{ + MatchLabels: providerLabels(provider), + } +} + +// prepareConfigMapLabels returns default set of labels that identify a config map with downloaded manifests. +func providerLabels(provider operatorv1.GenericProvider) map[string]string { + return map[string]string{ + configMapVersionLabel: provider.GetSpec().Version, + configMapTypeLabel: provider.GetType(), + configMapNameLabel: provider.GetName(), + operatorManagedLabel: "true", + } +} + // needToCompress checks whether the input data exceeds the maximum configmap // size limit and returns whether it should be compressed. func needToCompress(bs ...[]byte) bool { diff --git a/internal/controller/phases.go b/internal/controller/phases.go index 30d68c5d4..e2b2147cf 100644 --- a/internal/controller/phases.go +++ b/internal/controller/phases.go @@ -48,7 +48,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -const metadataFile = "metadata.yaml" +const ( + metadataFile = "metadata.yaml" +) // phaseReconciler holds all required information for interacting with clusterctl code and // helps to iterate through provider reconciliation phases. @@ -574,7 +576,32 @@ func clusterctlProviderName(provider operatorv1.GenericProvider) client.ObjectKe } func (p *phaseReconciler) repositoryProxy(ctx context.Context, provider configclient.Provider, configClient configclient.Client, options ...repository.Option) (repository.Client, error) { - cl, err := repository.New(ctx, provider, configClient, append([]repository.Option{repository.InjectRepository(p.repo)}, options...)...) + injectRepo := p.repo + + if !provider.SameAs(p.providerConfig) { + genericProvider, err := util.GetGenericProvider(ctx, p.ctrlClient, provider) + if err != nil { + return nil, wrapPhaseError(err, "unable to find generic provider for configclient "+string(provider.Type())+": "+provider.Name(), operatorv1.ProviderUpgradedCondition) + } + + if exists, err := p.checkConfigMapExists(ctx, *providerLabelSelector(genericProvider), genericProvider.GetNamespace()); err != nil { + provider := client.ObjectKeyFromObject(genericProvider) + return nil, wrapPhaseError(err, "failed to check the config map repository existence for provider "+provider.String(), operatorv1.ProviderUpgradedCondition) + } else if !exists { + provider := client.ObjectKeyFromObject(genericProvider) + return nil, wrapPhaseError(fmt.Errorf("config map not found"), "config map repository required for validation does not exist yet for provider "+provider.String(), operatorv1.ProviderUpgradedCondition) + } + + repo, err := p.configmapRepository(ctx, providerLabelSelector(genericProvider), genericProvider.GetNamespace(), "") + if err != nil { + provider := client.ObjectKeyFromObject(genericProvider) + return nil, wrapPhaseError(err, "failed to load the repository for provider "+provider.String(), operatorv1.ProviderUpgradedCondition) + } + + injectRepo = repo + } + + cl, err := repository.New(ctx, provider, configClient, append([]repository.Option{repository.InjectRepository(injectRepo)}, options...)...) if err != nil { return nil, err } diff --git a/util/util.go b/util/util.go index 0c27f9c81..949882eaf 100644 --- a/util/util.go +++ b/util/util.go @@ -27,6 +27,7 @@ import ( clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" configclient "sigs.k8s.io/cluster-api/cmd/clusterctl/client/config" "sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -36,6 +37,11 @@ const ( gitlabPackagesAPIPrefix = "/api/v4/projects/" ) +type genericProviderList interface { + ctrlclient.ObjectList + operatorv1.GenericProviderList +} + func IsCoreProvider(p genericprovider.GenericProvider) bool { _, ok := p.(*operatorv1.CoreProvider) return ok @@ -61,6 +67,40 @@ func ClusterctlProviderType(genericProvider operatorv1.GenericProvider) clusterc return clusterctlv1.ProviderTypeUnknown } +// GetGenericProvider returns the first of generic providers matching the type and the name from the configclient.Provider. +func GetGenericProvider(ctx context.Context, cl ctrlclient.Client, provider configclient.Provider) (operatorv1.GenericProvider, error) { + var list genericProviderList + + switch provider.Type() { + case clusterctlv1.CoreProviderType: + list = &operatorv1.CoreProviderList{} + case clusterctlv1.ControlPlaneProviderType: + list = &operatorv1.ControlPlaneProviderList{} + case clusterctlv1.InfrastructureProviderType: + list = &operatorv1.InfrastructureProviderList{} + case clusterctlv1.BootstrapProviderType: + list = &operatorv1.BootstrapProviderList{} + case clusterctlv1.AddonProviderType: + list = &operatorv1.AddonProviderList{} + case clusterctlv1.IPAMProviderType: + list = &operatorv1.IPAMProviderList{} + case clusterctlv1.RuntimeExtensionProviderType, clusterctlv1.ProviderTypeUnknown: + return nil, fmt.Errorf("provider %s type is not supported %s", provider.Name(), provider.Type()) + } + + if err := cl.List(ctx, list); err != nil { + return nil, err + } + + for _, p := range list.GetItems() { + if p.GetName() == provider.Name() { + return p, nil + } + } + + return nil, fmt.Errorf("unable to find provider manifest with name %s", provider.Name()) +} + // RepositoryFactory returns the repository implementation corresponding to the provider URL. // inspired by https://github.com/kubernetes-sigs/cluster-api/blob/124d9be7035e492f027cdc7a701b6b179451190a/cmd/clusterctl/client/repository/client.go#L170 func RepositoryFactory(ctx context.Context, providerConfig configclient.Provider, configVariablesClient configclient.VariablesClient) (repository.Repository, error) { From 99db14b3979fc1eda8a2b3d4145f71d5870d96e7 Mon Sep 17 00:00:00 2001 From: Danil Grigorev Date: Thu, 7 Mar 2024 15:38:41 +0100 Subject: [PATCH 2/2] Add tests for other provider metadata check during upgrade Signed-off-by: Danil Grigorev --- .../controller/manifests_downloader_test.go | 2 +- internal/controller/phases_test.go | 210 ++++++++++++++++++ 2 files changed, 211 insertions(+), 1 deletion(-) diff --git a/internal/controller/manifests_downloader_test.go b/internal/controller/manifests_downloader_test.go index 8e1cbc29f..f564100b9 100644 --- a/internal/controller/manifests_downloader_test.go +++ b/internal/controller/manifests_downloader_test.go @@ -62,7 +62,7 @@ func TestManifestsDownloader(t *testing.T) { MatchLabels: p.prepareConfigMapLabels(), } - exists, err := p.checkConfigMapExists(ctx, labelSelector) + exists, err := p.checkConfigMapExists(ctx, labelSelector, p.provider.GetNamespace()) g.Expect(err).ToNot(HaveOccurred()) g.Expect(exists).To(BeTrue()) diff --git a/internal/controller/phases_test.go b/internal/controller/phases_test.go index 112cd3c85..d4308d603 100644 --- a/internal/controller/phases_test.go +++ b/internal/controller/phases_test.go @@ -18,6 +18,7 @@ package controller import ( "context" + "fmt" "testing" . "github.com/onsi/gomega" @@ -26,6 +27,7 @@ import ( clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" configclient "sigs.k8s.io/cluster-api/cmd/clusterctl/client/config" "sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2" @@ -412,6 +414,214 @@ metadata: } } +func TestRepositoryProxy(t *testing.T) { + coreProvider := configclient.NewProvider("cluster-api", "https://github.com/kubernetes-sigs/cluster-api/releases/latest/core-components.yaml", clusterctlv1.CoreProviderType) + awsProvider := configclient.NewProvider("aws", "https://github.com/kubernetes-sigs/cluster-api-provider-aws/releases/v1.4.1/infrastructure-components.yaml", clusterctlv1.InfrastructureProviderType) + + provider := &operatorv1.InfrastructureProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aws", + Namespace: "ns1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "InfrastructureProvider", + APIVersion: "operator.cluster.x-k8s.io/v1alpha2", + }, + Spec: operatorv1.InfrastructureProviderSpec{ + ProviderSpec: operatorv1.ProviderSpec{ + Version: "v2.3.5", + FetchConfig: &operatorv1.FetchConfiguration{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"provider-components": "aws"}, + }, + }, + }, + }, + } + + core := &operatorv1.CoreProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-api", + Namespace: "default", + }, + Spec: operatorv1.CoreProviderSpec{ + ProviderSpec: operatorv1.ProviderSpec{ + Version: testCurrentVersion, + }, + }, + } + + awsMetadata := ` +apiVersion: clusterctl.cluster.x-k8s.io/v1alpha3 +releaseSeries: + - major: 2 + minor: 4 + contract: v1beta1 + - major: 2 + minor: 3 + contract: v1beta1` + + awsMetaReleaseSeries := []clusterctlv1.ReleaseSeries{ + { + Major: 2, + Minor: 4, + Contract: "v1beta1", + }, { + Major: 2, + Minor: 3, + Contract: "v1beta1", + }, + } + + metadata := ` +apiVersion: clusterctl.cluster.x-k8s.io/v1alpha3 +releaseSeries: + - major: 0 + minor: 4 + contract: v1alpha4 + - major: 0 + minor: 3 + contract: v1alpha3` + + metaReleaseSeries := []clusterctlv1.ReleaseSeries{{ + Major: 0, + Minor: 4, + Contract: "v1alpha4", + }, { + Major: 0, + Minor: 3, + Contract: "v1alpha3", + }} + + tests := []struct { + name string + configMaps []corev1.ConfigMap + genericProviders []client.Object + provider configclient.Provider + defaultRepository bool + wantMetadataSeries []clusterctlv1.ReleaseSeries + wantErr string + metadataErr string + wantDefaultVersion string + }{ + { + name: "missing configmaps", + provider: coreProvider, + wantDefaultVersion: testCurrentVersion, + genericProviders: []client.Object{core, provider}, + metadataErr: "failed to read \"metadata.yaml\" from the repository for provider \"cluster-api\": unable to get files for version v0.4.2", + }, + { + name: "correct configmap with data", + genericProviders: []client.Object{core, provider}, + provider: coreProvider, + defaultRepository: true, + wantDefaultVersion: testCurrentVersion, + wantMetadataSeries: metaReleaseSeries, + configMaps: []corev1.ConfigMap{ + { + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: testCurrentVersion, + Namespace: "default", + Labels: map[string]string{ + configMapVersionLabel: testCurrentVersion, + configMapTypeLabel: core.GetType(), + configMapNameLabel: core.GetName(), + operatorManagedLabel: "true", + }, + }, + Data: map[string]string{"metadata": metadata, "components": ""}, + }, + }, + }, + { + name: "upgrade required validation of another provider missing config map", + genericProviders: []client.Object{core, provider}, + provider: awsProvider, + wantErr: wrapPhaseError(fmt.Errorf("config map not found"), "config map repository required for validation does not exist yet for provider ns1/aws", operatorv1.ProviderUpgradedCondition).Error(), + }, + { + name: "updgrade requested an unknown provider for the operator", + genericProviders: []client.Object{core}, + provider: awsProvider, + wantErr: wrapPhaseError(fmt.Errorf("unable to find provider manifest with name aws"), "unable to find generic provider for configclient InfrastructureProvider: aws", operatorv1.ProviderUpgradedCondition).Error(), + }, + { + name: "upgrade required validation of another provider metadata succeeds", + genericProviders: []client.Object{core, provider}, + provider: awsProvider, + wantDefaultVersion: "v2.3.5", + wantMetadataSeries: awsMetaReleaseSeries, + configMaps: []corev1.ConfigMap{ + { + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "v2.3.5", + Namespace: provider.Namespace, + Labels: map[string]string{"provider-components": "aws"}, + }, + Data: map[string]string{"metadata": awsMetadata, "components": ""}, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + fakeclient := fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(tt.genericProviders...).Build() + p := &phaseReconciler{ + ctrlClient: fakeclient, + providerConfig: coreProvider, + repo: repository.NewMemoryRepository(), + provider: core, + } + + for i := range tt.configMaps { + g.Expect(fakeclient.Create(ctx, &tt.configMaps[i])).To(Succeed()) + } + if tt.defaultRepository { + var err error + p.repo, err = p.configmapRepository(ctx, &metav1.LabelSelector{ + MatchLabels: map[string]string{ + operatorManagedLabel: "true", + }, + }, "default", "") + g.Expect(err).To(Succeed()) + } + + cl, err := configclient.New(ctx, "") + g.Expect(err).To(Succeed()) + + got, err := p.repositoryProxy(ctx, tt.provider, cl) + if len(tt.wantErr) > 0 { + g.Expect(err).Should(MatchError(tt.wantErr)) + return + } + g.Expect(err).To(Succeed()) + + meta := got.Metadata(tt.wantDefaultVersion) + metadataData, err := meta.Get(ctx) + if len(tt.metadataErr) > 0 { + g.Expect(err).Should(MatchError(tt.metadataErr)) + return + } + g.Expect(err).To(Succeed()) + g.Expect(metadataData.ReleaseSeries).To(Equal(tt.wantMetadataSeries)) + + g.Expect(got.DefaultVersion()).To(Equal(tt.wantDefaultVersion)) + }) + } +} + func TestRepositoryFactory(t *testing.T) { testCases := []struct { name string