From a111f7bd0467236f11433f599360da6710630eb0 Mon Sep 17 00:00:00 2001 From: Konstantinos Angelopoulos Date: Mon, 10 Jun 2024 12:12:42 +0300 Subject: [PATCH] save --- .../templates/storageclass.yaml | 8 ++++++++ .../charts/csi-driver-manila/values.yaml | 2 +- .../openstack/validation/infrastructure.go | 2 +- pkg/controller/controlplane/valuesprovider.go | 6 ------ .../controlplane/valuesprovider_test.go | 20 +++++++++---------- .../infrastructure/infraflow/delete.go | 11 +++++++--- .../infrastructure/infraflow/reconcile.go | 3 ++- .../infrastructure/infraflow/utils.go | 12 ++--------- .../infrastructure/terraform_reconciler.go | 7 ++++--- pkg/internal/infrastructure/infrastucture.go | 13 +++++++++--- .../infrastructure/infrastructure_test.go | 20 +++++++++---------- 11 files changed, 56 insertions(+), 48 deletions(-) diff --git a/charts/internal/shoot-system-components/charts/csi-driver-manila/templates/storageclass.yaml b/charts/internal/shoot-system-components/charts/csi-driver-manila/templates/storageclass.yaml index 9e4a4b772..38b8a3f16 100644 --- a/charts/internal/shoot-system-components/charts/csi-driver-manila/templates/storageclass.yaml +++ b/charts/internal/shoot-system-components/charts/csi-driver-manila/templates/storageclass.yaml @@ -15,7 +15,9 @@ mountOptions: parameters: type: default shareNetworkID: {{ $.Values.openstack.shareNetworkID }} + {{- if $.Values.openstack.shareClient}} nfs-shareClient: {{ required "openstack.shareClient needs to be set" $.Values.openstack.shareClient }} + {{- end }} csi.storage.k8s.io/provisioner-secret-name: manila-csi-plugin csi.storage.k8s.io/provisioner-secret-namespace: {{ $.Release.Namespace }} csi.storage.k8s.io/node-stage-secret-name: manila-csi-plugin @@ -43,7 +45,9 @@ parameters: type: default autoTopology: "true" shareNetworkID: {{ $.Values.openstack.shareNetworkID }} + {{- if $.Values.openstack.shareClient}} nfs-shareClient: {{ required "openstack.shareClient needs to be set" $.Values.openstack.shareClient }} + {{- end }} csi.storage.k8s.io/provisioner-secret-name: manila-csi-plugin csi.storage.k8s.io/provisioner-secret-namespace: {{ $.Release.Namespace }} csi.storage.k8s.io/node-stage-secret-name: manila-csi-plugin @@ -72,7 +76,9 @@ parameters: type: default availability: {{ required "openstack.availabilityZones needs to be set" . }} shareNetworkID: {{ $.Values.openstack.shareNetworkID }} + {{- if $.Values.openstack.shareClient}} nfs-shareClient: {{ required "openstack.shareClient needs to be set" $.Values.openstack.shareClient }} + {{- end }} csi.storage.k8s.io/provisioner-secret-name: manila-csi-plugin csi.storage.k8s.io/provisioner-secret-namespace: {{ $.Release.Namespace }} csi.storage.k8s.io/node-stage-secret-name: manila-csi-plugin @@ -107,7 +113,9 @@ parameters: type: default availability: {{ required "openstack.availabilityZones needs to be set" . }} shareNetworkID: {{ $.Values.openstack.shareNetworkID }} + {{- if $.Values.openstack.shareClient}} nfs-shareClient: {{ required "openstack.shareClient needs to be set" $.Values.openstack.shareClient }} + {{- end }} csi.storage.k8s.io/provisioner-secret-name: manila-csi-plugin csi.storage.k8s.io/provisioner-secret-namespace: {{ $.Release.Namespace }} csi.storage.k8s.io/node-stage-secret-name: manila-csi-plugin diff --git a/charts/internal/shoot-system-components/charts/csi-driver-manila/values.yaml b/charts/internal/shoot-system-components/charts/csi-driver-manila/values.yaml index 99e2cd552..0aba7eef0 100644 --- a/charts/internal/shoot-system-components/charts/csi-driver-manila/values.yaml +++ b/charts/internal/shoot-system-components/charts/csi-driver-manila/values.yaml @@ -13,7 +13,7 @@ openstack: - zone1 - zone2 shareNetworkID: shareNetworkIDValue - shareClient: 10.180.0.0/16 + # shareClient: 10.180.0.0/16 shareProtocol: NFS authURL: authURLValue region: regionValue diff --git a/pkg/apis/openstack/validation/infrastructure.go b/pkg/apis/openstack/validation/infrastructure.go index 3ce9e49b6..c59f2bf19 100644 --- a/pkg/apis/openstack/validation/infrastructure.go +++ b/pkg/apis/openstack/validation/infrastructure.go @@ -33,7 +33,7 @@ func ValidateInfrastructureConfig(infra *api.InfrastructureConfig, nodesCIDR *st } networksPath := fldPath.Child("networks") - if len(infra.Networks.Worker) == 0 && len(infra.Networks.Workers) == 0 { + if len(infra.Networks.Worker) == 0 && len(infra.Networks.Workers) == 0 && infra.Networks.SubnetID == nil { allErrs = append(allErrs, field.Required(networksPath.Child("workers"), "must specify the network range for the worker network")) } diff --git a/pkg/controller/controlplane/valuesprovider.go b/pkg/controller/controlplane/valuesprovider.go index 233fc68f5..08c9e25cf 100644 --- a/pkg/controller/controlplane/valuesprovider.go +++ b/pkg/controller/controlplane/valuesprovider.go @@ -44,7 +44,6 @@ import ( "github.com/gardener/gardener-extension-provider-openstack/charts" api "github.com/gardener/gardener-extension-provider-openstack/pkg/apis/openstack" "github.com/gardener/gardener-extension-provider-openstack/pkg/apis/openstack/helper" - "github.com/gardener/gardener-extension-provider-openstack/pkg/internal/infrastructure" "github.com/gardener/gardener-extension-provider-openstack/pkg/openstack" "github.com/gardener/gardener-extension-provider-openstack/pkg/utils" ) @@ -952,10 +951,6 @@ func (vp *valuesProvider) addCSIManilaValues( "clusterID": cp.Namespace, } - infraConfig, err := helper.InfrastructureConfigFromRawExtension(cluster.Shoot.Spec.Provider.InfrastructureConfig) - if err != nil { - return fmt.Errorf("could not decode infrastructure config of controlplane '%s': %w", kutil.ObjectName(cp), err) - } infraStatus, err := vp.getInfrastructureStatus(cp) if err != nil { return err @@ -984,7 +979,6 @@ func (vp *valuesProvider) addCSIManilaValues( values["openstack"] = map[string]interface{}{ "availabilityZones": vp.getAllWorkerPoolsZones(cluster), "shareNetworkID": shareNetworkID, - "shareClient": infrastructure.WorkersCIDR(infraConfig), "authURL": authURL, "region": cp.Spec.Region, "domainName": domainName, diff --git a/pkg/controller/controlplane/valuesprovider_test.go b/pkg/controller/controlplane/valuesprovider_test.go index 7b5d89c50..5c6ade8b5 100644 --- a/pkg/controller/controlplane/valuesprovider_test.go +++ b/pkg/controller/controlplane/valuesprovider_test.go @@ -663,11 +663,11 @@ var _ = Describe("ValuesProvider", func() { "authURL": authURL, "region": "europe", "applicationCredentialSecret": "", - "shareClient": "10.200.0.0/19", - "shareNetworkID": "1111-2222-3333-4444", - "domainName": "domain-name", - "tlsInsecure": "", - "caCert": "", + // "shareClient": "10.200.0.0/19", + "shareNetworkID": "1111-2222-3333-4444", + "domainName": "domain-name", + "tlsInsecure": "", + "caCert": "", }, }), })) @@ -792,11 +792,11 @@ var _ = Describe("ValuesProvider", func() { "authURL": authURL, "region": "europe", "applicationCredentialSecret": "", - "shareClient": "10.200.0.0/19", - "shareNetworkID": "1111-2222-3333-4444", - "domainName": "domain-name", - "tlsInsecure": "", - "caCert": "", + // "shareClient": "10.200.0.0/19", + "shareNetworkID": "1111-2222-3333-4444", + "domainName": "domain-name", + "tlsInsecure": "", + "caCert": "", }, "vpaEnabled": true, }), diff --git a/pkg/controller/infrastructure/infraflow/delete.go b/pkg/controller/infrastructure/infraflow/delete.go index 209cfb849..417c36127 100644 --- a/pkg/controller/infrastructure/infraflow/delete.go +++ b/pkg/controller/infrastructure/infraflow/delete.go @@ -37,7 +37,7 @@ func (fctx *FlowContext) buildDeleteGraph() *flow.Graph { needToDeleteNetwork := fctx.config.Networks.ID == nil needToDeleteRouter := fctx.config.Networks.Router == nil // skip deletion of the subnet if we need to delete the network (it will be deleted anyway), or if the subnet is user provided. - needToDeleteSubnet := !needToDeleteNetwork && fctx.config.Networks.SubnetID == nil + needToDeleteSubnet := fctx.config.Networks.SubnetID == nil _ = fctx.AddTask(g, "delete ssh key pair", fctx.deleteSSHKeyPair, @@ -57,8 +57,13 @@ func (fctx *FlowContext) buildDeleteGraph() *flow.Graph { if routerID == nil { return nil } - return infrastructure.CleanupKubernetesRoutes(ctx, fctx.networking, *routerID, infrastructure.WorkersCIDR(fctx.config)) + subnetID := fctx.state.Get(IdentifierSubnet) + if subnetID == nil { + return nil + } + return infrastructure.CleanupKubernetesRoutes(ctx, fctx.networking, *routerID, *subnetID) }, + shared.DoIf(needToDeleteRouter || needToDeleteSubnet), shared.Timeout(defaultTimeout), ) k8sLoadBalancers := fctx.AddTask(g, "delete kubernetes loadbalancers", @@ -82,7 +87,7 @@ func (fctx *FlowContext) buildDeleteGraph() *flow.Graph { // subnet deletion only needed if network is given by spec _ = fctx.AddTask(g, "delete subnet", fctx.deleteSubnet, - shared.DoIf(needToDeleteSubnet), shared.Timeout(defaultTimeout), shared.Dependencies(deleteRouterInterface, k8sLoadBalancers)) + shared.DoIf(!needToDeleteNetwork && needToDeleteSubnet), shared.Timeout(defaultTimeout), shared.Dependencies(deleteRouterInterface, k8sLoadBalancers)) _ = fctx.AddTask(g, "delete network", fctx.deleteNetwork, shared.DoIf(needToDeleteNetwork), shared.Timeout(defaultTimeout), shared.Dependencies(deleteRouterInterface)) diff --git a/pkg/controller/infrastructure/infraflow/reconcile.go b/pkg/controller/infrastructure/infraflow/reconcile.go index 762b3df62..5f9b06c2f 100644 --- a/pkg/controller/infrastructure/infraflow/reconcile.go +++ b/pkg/controller/infrastructure/infraflow/reconcile.go @@ -64,6 +64,7 @@ func (fctx *FlowContext) buildReconcileGraph() *flow.Graph { _ = fctx.AddTask(g, "ensure router interface", fctx.ensureRouterInterface, + shared.DoIf(fctx.config.Networks.SubnetID == nil || fctx.config.Networks.Router == nil), shared.Timeout(defaultTimeout), shared.Dependencies(ensureRouter, ensureSubnet)) ensureSecGroup := fctx.AddTask(g, "ensure security group", @@ -292,7 +293,7 @@ func (fctx *FlowContext) ensureNewSubnet(ctx context.Context) error { desired := &subnets.Subnet{ Name: fctx.defaultSubnetName(), NetworkID: networkID, - CIDR: fctx.workerCIDR(), + CIDR: infrainternal.WorkersCIDR(fctx.config), IPVersion: 4, DNSNameservers: fctx.cloudProfileConfig.DNSServers, } diff --git a/pkg/controller/infrastructure/infraflow/utils.go b/pkg/controller/infrastructure/infraflow/utils.go index 7e8e258b3..f9fa7e04f 100644 --- a/pkg/controller/infrastructure/infraflow/utils.go +++ b/pkg/controller/infrastructure/infraflow/utils.go @@ -6,6 +6,7 @@ package infraflow import ( "fmt" + "reflect" ) // ErrorMultipleMatches is returned when the findExisting finds multiple resources matching a name. @@ -36,7 +37,7 @@ func findExisting[T any](id *string, name string, // TODO: check if this makes sense if len(found) > 1 { - return nil, ErrorMultipleMatches + return nil, fmt.Errorf("error finding existing %s: %w", reflect.TypeFor[T]().Name(), ErrorMultipleMatches) } if len(selector) > 0 { @@ -92,12 +93,3 @@ func (fctx *FlowContext) defaultSecurityGroupName() string { func (fctx *FlowContext) defaultSharedNetworkName() string { return fctx.infra.Namespace } - -func (fctx *FlowContext) workerCIDR() string { - s := fctx.config.Networks.Worker - if workers := fctx.config.Networks.Workers; workers != "" { - s = workers - } - - return s -} diff --git a/pkg/controller/infrastructure/terraform_reconciler.go b/pkg/controller/infrastructure/terraform_reconciler.go index 803a5f221..ee1a4fe6d 100644 --- a/pkg/controller/infrastructure/terraform_reconciler.go +++ b/pkg/controller/infrastructure/terraform_reconciler.go @@ -186,9 +186,9 @@ func (t *TerraformReconciler) delete(ctx context.Context, infra *extensionsv1alp destroyKubernetesRoutes = g.Add(flow.Task{ Name: "Destroying Kubernetes route entries", Fn: flow.TaskFn(func(ctx context.Context) error { - return t.cleanupKubernetesRoutes(ctx, config, networkingClient, vars[infrastructure.TerraformOutputKeyRouterID]) + return t.cleanupKubernetesRoutes(ctx, config, networkingClient, vars[infrastructure.TerraformOutputKeyRouterID], vars[infrastructure.TerraformOutputKeySubnetID]) }).RetryUntilTimeout(10*time.Second, 5*time.Minute), - SkipIf: !configExists, + SkipIf: !configExists || (config.Networks.SubnetID != nil && config.Networks.Router != nil), }) _ = g.Add(flow.Task{ @@ -231,6 +231,7 @@ func (t *TerraformReconciler) cleanupKubernetesRoutes( config *api.InfrastructureConfig, client openstackclient.Networking, routerID string, + subnetID string, ) error { if routerID == "" { return nil @@ -239,7 +240,7 @@ func (t *TerraformReconciler) cleanupKubernetesRoutes( if workesCIDR == "" { return nil } - return infrastructure.CleanupKubernetesRoutes(ctx, client, routerID, workesCIDR) + return infrastructure.CleanupKubernetesRoutes(ctx, client, routerID, subnetID) } func (t *TerraformReconciler) cleanupKubernetesLoadbalancers( diff --git a/pkg/internal/infrastructure/infrastucture.go b/pkg/internal/infrastructure/infrastucture.go index 7b50a6ce6..0030f4294 100644 --- a/pkg/internal/infrastructure/infrastucture.go +++ b/pkg/internal/infrastructure/infrastucture.go @@ -95,18 +95,25 @@ func CleanupKubernetesLoadbalancers(ctx context.Context, log logr.Logger, client } // CleanupKubernetesRoutes deletes all routes from the router which have a nextHop in the subnet. -func CleanupKubernetesRoutes(_ context.Context, client openstackclient.Networking, routerID, workers string) error { +func CleanupKubernetesRoutes(_ context.Context, client openstackclient.Networking, routerID, subnetID string) error { router, err := client.GetRouterByID(routerID) if err != nil { return err } - if router == nil { return nil } + subnet, err := client.GetSubnetByID(subnetID) + if err != nil { + return err + } + if subnet == nil { + return nil + } + routes := []routers.Route{} - _, workersNet, err := net.ParseCIDR(workers) + _, workersNet, err := net.ParseCIDR(subnet.CIDR) if err != nil { return err } diff --git a/test/integration/infrastructure/infrastructure_test.go b/test/integration/infrastructure/infrastructure_test.go index b28469051..00b63aa05 100644 --- a/test/integration/infrastructure/infrastructure_test.go +++ b/test/integration/infrastructure/infrastructure_test.go @@ -226,7 +226,7 @@ var _ = Describe("Infrastructure tests", func() { }) It("minimum configuration infrastructure", func() { - providerConfig := newProviderConfig("", nil, nil) + providerConfig := newProviderConfig(nil, nil, nil) cloudProfileConfig := newCloudProfileConfig(*region, *authURL) namespace, err := generateNamespaceName() Expect(err).NotTo(HaveOccurred()) @@ -253,7 +253,7 @@ var _ = Describe("Infrastructure tests", func() { framework.RemoveCleanupAction(cleanupHandle) }) - providerConfig := newProviderConfig(*routerID, nil, nil) + providerConfig := newProviderConfig(routerID, nil, nil) cloudProfileConfig := newCloudProfileConfig(*region, *authURL) err = runTest(ctx, log, c, namespace, providerConfig, decoder, cloudProfileConfig) @@ -277,7 +277,7 @@ var _ = Describe("Infrastructure tests", func() { framework.RemoveCleanupAction(cleanupHandle) }) - providerConfig := newProviderConfig("", networkID, nil) + providerConfig := newProviderConfig(nil, networkID, nil) cloudProfileConfig := newCloudProfileConfig(*region, *authURL) err = runTest(ctx, log, c, namespace, providerConfig, decoder, cloudProfileConfig) @@ -310,7 +310,7 @@ var _ = Describe("Infrastructure tests", func() { framework.RemoveCleanupAction(cleanupHandle) }) - providerConfig := newProviderConfig(*routerID, networkID, nil) + providerConfig := newProviderConfig(routerID, networkID, nil) cloudProfileConfig := newCloudProfileConfig(*region, *authURL) err = runTest(ctx, log, c, namespace, providerConfig, decoder, cloudProfileConfig) @@ -354,14 +354,14 @@ var _ = Describe("Infrastructure tests", func() { framework.RemoveCleanupAction(cleanupHandle) }) - providerConfig := newProviderConfig(*routerID, subnetID, networkID) + providerConfig := newProviderConfig(routerID, networkID, subnetID) cloudProfileConfig := newCloudProfileConfig(*region, *authURL) err = runTest(ctx, log, c, namespace, providerConfig, decoder, cloudProfileConfig) Expect(err).NotTo(HaveOccurred()) }) - FIt("with infrastructure that uses existing network and subnet", func() { + It("with infrastructure that uses existing network and subnet", func() { namespace, err := generateNamespaceName() Expect(err).NotTo(HaveOccurred()) @@ -385,7 +385,7 @@ var _ = Describe("Infrastructure tests", func() { framework.RemoveCleanupAction(cleanupHandle) }) - providerConfig := newProviderConfig("", subnetID, networkID) + providerConfig := newProviderConfig(nil, networkID, subnetID) cloudProfileConfig := newCloudProfileConfig(*region, *authURL) err = runTest(ctx, log, c, namespace, providerConfig, decoder, cloudProfileConfig) @@ -599,11 +599,11 @@ func checkOperationAnnotationRemoved(obj client.Object) error { // newProviderConfig creates a providerConfig with the network and router details. // If routerID is set to "", it requests a new router creation. // Else it reuses the supplied routerID. -func newProviderConfig(routerID string, networkID *string, subnetID *string) *openstackv1alpha1.InfrastructureConfig { +func newProviderConfig(routerID *string, networkID *string, subnetID *string) *openstackv1alpha1.InfrastructureConfig { var router *openstackv1alpha1.Router - if routerID != "" { - router = &openstackv1alpha1.Router{ID: routerID} + if routerID != nil { + router = &openstackv1alpha1.Router{ID: *routerID} } return &openstackv1alpha1.InfrastructureConfig{