From 99eefe68bd1d9a5124838c293451dbfe44a963a6 Mon Sep 17 00:00:00 2001 From: Shaad7 Date: Thu, 1 Feb 2024 12:42:21 +0600 Subject: [PATCH] Fix GKE Cluster Creation for User Provided Subnet Signed-off-by: Shaad7 Signed-off-by: AbdullahAlShaad --- cloud/services/compute/subnets/reconcile.go | 12 +++- .../services/container/clusters/reconcile.go | 16 ++++- exp/api/v1beta1/gcpmanagedcluster_webhook.go | 36 ++++++++++- test/e2e/config/gcp-ci.yaml | 1 + ...cluster-template-ci-gke-custom-subnet.yaml | 64 +++++++++++++++++++ test/e2e/e2e_gke_test.go | 31 ++++++++- 6 files changed, 152 insertions(+), 8 deletions(-) create mode 100644 test/e2e/data/infrastructure-gcp/cluster-template-ci-gke-custom-subnet.yaml diff --git a/cloud/services/compute/subnets/reconcile.go b/cloud/services/compute/subnets/reconcile.go index 403c49fb7..2a186d6aa 100644 --- a/cloud/services/compute/subnets/reconcile.go +++ b/cloud/services/compute/subnets/reconcile.go @@ -43,7 +43,7 @@ func (s *Service) Delete(ctx context.Context) error { logger := log.FromContext(ctx) for _, subnetSpec := range s.scope.SubnetSpecs() { logger.V(2).Info("Deleting a subnet", "name", subnetSpec.Name) - subnetKey := meta.RegionalKey(subnetSpec.Name, s.scope.Region()) + subnetKey := meta.RegionalKey(subnetSpec.Name, s.getSubnetRegion(subnetSpec)) err := s.subnets.Delete(ctx, subnetKey) if err != nil && !gcperrors.IsNotFound(err) { logger.Error(err, "Error deleting subnet", "name", subnetSpec.Name) @@ -60,7 +60,7 @@ func (s *Service) createOrGetSubnets(ctx context.Context) ([]*compute.Subnetwork subnets := []*compute.Subnetwork{} for _, subnetSpec := range s.scope.SubnetSpecs() { logger.V(2).Info("Looking for subnet", "name", subnetSpec.Name) - subnetKey := meta.RegionalKey(subnetSpec.Name, s.scope.Region()) + subnetKey := meta.RegionalKey(subnetSpec.Name, s.getSubnetRegion(subnetSpec)) subnet, err := s.subnets.Get(ctx, subnetKey) if err != nil { if !gcperrors.IsNotFound(err) { @@ -86,3 +86,11 @@ func (s *Service) createOrGetSubnets(ctx context.Context) ([]*compute.Subnetwork return subnets, nil } + +// getSubnetRegion returns subnet region if user provided it, otherwise returns default scope region. +func (s *Service) getSubnetRegion(subnetSpec *compute.Subnetwork) string { + if subnetSpec.Region != "" { + return subnetSpec.Region + } + return s.scope.Region() +} diff --git a/cloud/services/container/clusters/reconcile.go b/cloud/services/container/clusters/reconcile.go index cc550d9bd..ee047aa52 100644 --- a/cloud/services/container/clusters/reconcile.go +++ b/cloud/services/container/clusters/reconcile.go @@ -250,10 +250,10 @@ func (s *Service) createCluster(ctx context.Context, log *logr.Logger) error { } isRegional := shared.IsRegional(s.scope.Region()) - cluster := &containerpb.Cluster{ - Name: s.scope.ClusterName(), - Network: *s.scope.GCPManagedCluster.Spec.Network.Name, + Name: s.scope.ClusterName(), + Network: *s.scope.GCPManagedCluster.Spec.Network.Name, + Subnetwork: s.getSubnetNameInClusterRegion(), Autopilot: &containerpb.Autopilot{ Enabled: s.scope.GCPManagedControlPlane.Spec.EnableAutopilot, }, @@ -295,6 +295,16 @@ func (s *Service) createCluster(ctx context.Context, log *logr.Logger) error { return nil } +// getSubnetNameInClusterRegion returns the subnet which is in the same region as cluster. If not found it returns empty string. +func (s *Service) getSubnetNameInClusterRegion() string { + for _, subnet := range s.scope.GCPManagedCluster.Spec.Network.Subnets { + if subnet.Region == s.scope.Region() { + return subnet.Name + } + } + return "" +} + func (s *Service) updateCluster(ctx context.Context, updateClusterRequest *containerpb.UpdateClusterRequest, log *logr.Logger) error { _, err := s.scope.ManagedControlPlaneClient().UpdateCluster(ctx, updateClusterRequest) if err != nil { diff --git a/exp/api/v1beta1/gcpmanagedcluster_webhook.go b/exp/api/v1beta1/gcpmanagedcluster_webhook.go index 219c0aff2..d77b01896 100644 --- a/exp/api/v1beta1/gcpmanagedcluster_webhook.go +++ b/exp/api/v1beta1/gcpmanagedcluster_webhook.go @@ -20,6 +20,7 @@ import ( "github.com/google/go-cmp/cmp" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -53,7 +54,7 @@ var _ webhook.Validator = &GCPManagedCluster{} func (r *GCPManagedCluster) ValidateCreate() (admission.Warnings, error) { gcpmanagedclusterlog.Info("validate create", "name", r.Name) - return nil, nil + return r.validate() } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. @@ -96,3 +97,36 @@ func (r *GCPManagedCluster) ValidateDelete() (admission.Warnings, error) { return nil, nil } + +func (r *GCPManagedCluster) validate() (admission.Warnings, error) { + validators := []func() error{ + r.validateCustomSubnet, + } + + var errs []error + for _, validator := range validators { + if err := validator(); err != nil { + errs = append(errs, err) + } + } + + return nil, kerrors.NewAggregate(errs) +} + +func (r *GCPManagedCluster) validateCustomSubnet() error { + gcpmanagedclusterlog.Info("validate custom subnet", "name", r.Name) + if r.Spec.Network.AutoCreateSubnetworks == nil || *r.Spec.Network.AutoCreateSubnetworks { + return nil + } + var isSubnetExistInClusterRegion = false + for _, subnet := range r.Spec.Network.Subnets { + if subnet.Region == r.Spec.Region { + isSubnetExistInClusterRegion = true + } + } + + if !isSubnetExistInClusterRegion { + return field.Required(field.NewPath("spec", "network", "subnet"), "at least one given subnets region should be same as spec.network.region when spec.network.autoCreateSubnetworks is false") + } + return nil +} diff --git a/test/e2e/config/gcp-ci.yaml b/test/e2e/config/gcp-ci.yaml index ca2b5ec5e..16f7630f7 100644 --- a/test/e2e/config/gcp-ci.yaml +++ b/test/e2e/config/gcp-ci.yaml @@ -71,6 +71,7 @@ providers: - sourcePath: "${PWD}/test/e2e/data/infrastructure-gcp/cluster-template-ci-with-creds.yaml" - sourcePath: "${PWD}/test/e2e/data/infrastructure-gcp/cluster-template-ci-gke.yaml" - sourcePath: "${PWD}/test/e2e/data/infrastructure-gcp/cluster-template-ci-gke-autopilot.yaml" + - sourcePath: "${PWD}/test/e2e/data/infrastructure-gcp/cluster-template-ci-gke-custom-subnet.yaml" variables: KUBERNETES_VERSION: "${KUBERNETES_VERSION:-v1.28.3}" diff --git a/test/e2e/data/infrastructure-gcp/cluster-template-ci-gke-custom-subnet.yaml b/test/e2e/data/infrastructure-gcp/cluster-template-ci-gke-custom-subnet.yaml new file mode 100644 index 000000000..75b54264d --- /dev/null +++ b/test/e2e/data/infrastructure-gcp/cluster-template-ci-gke-custom-subnet.yaml @@ -0,0 +1,64 @@ +--- +apiVersion: cluster.x-k8s.io/v1beta1 +kind: Cluster +metadata: + name: "${CLUSTER_NAME}" +spec: + clusterNetwork: + pods: + cidrBlocks: ["192.168.0.0/16"] + infrastructureRef: + apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 + kind: GCPManagedCluster + name: "${CLUSTER_NAME}" + controlPlaneRef: + kind: GCPManagedControlPlane + apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 + name: "${CLUSTER_NAME}-control-plane" +--- +apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 +kind: GCPManagedCluster +metadata: + name: "${CLUSTER_NAME}" +spec: + project: "${GCP_PROJECT}" + region: "${GCP_REGION}" + network: + name: "${GCP_NETWORK_NAME}" + autoCreateSubnetworks: false + subnets: + - name: "${GCP_SUBNET_NAME}" + cidrBlock: "${GCP_SUBNET_CIDR}" + region: "${GCP_REGION}" +--- +apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 +kind: GCPManagedControlPlane +metadata: + name: "${CLUSTER_NAME}-control-plane" +spec: + project: "${GCP_PROJECT}" + location: "${GCP_REGION}" +--- +apiVersion: cluster.x-k8s.io/v1beta1 +kind: MachinePool +metadata: + name: ${CLUSTER_NAME}-mp-0 +spec: + clusterName: ${CLUSTER_NAME} + replicas: ${WORKER_MACHINE_COUNT} + template: + spec: + bootstrap: + dataSecretName: "" + clusterName: ${CLUSTER_NAME} + infrastructureRef: + apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 + kind: GCPManagedMachinePool + name: ${CLUSTER_NAME}-mp-0 +--- +apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 +kind: GCPManagedMachinePool +metadata: + name: ${CLUSTER_NAME}-mp-0 +spec: {} + diff --git a/test/e2e/e2e_gke_test.go b/test/e2e/e2e_gke_test.go index 0f6f2a099..927d572db 100644 --- a/test/e2e/e2e_gke_test.go +++ b/test/e2e/e2e_gke_test.go @@ -25,8 +25,6 @@ import ( "os" "path/filepath" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api/test/framework" @@ -159,4 +157,33 @@ var _ = Describe("GKE workload cluster creation", func() { }, result) }) }) + + Context("Creating a GKE cluster with custom subnet", func() { + It("Should create a cluster with 3 machine pool and custom subnet", func() { + By("Initializes with 3 machine pool") + + ApplyManagedClusterTemplateAndWait(ctx, ApplyManagedClusterTemplateAndWaitInput{ + ClusterProxy: bootstrapClusterProxy, + ConfigCluster: clusterctl.ConfigClusterInput{ + LogFolder: clusterctlLogFolder, + ClusterctlConfigPath: clusterctlConfigPath, + KubeconfigPath: bootstrapClusterProxy.GetKubeconfigPath(), + InfrastructureProvider: clusterctl.DefaultInfrastructureProvider, + Flavor: "ci-gke-custom-subnet", + Namespace: namespace.Name, + ClusterName: clusterName, + KubernetesVersion: e2eConfig.GetVariable(KubernetesVersion), + ControlPlaneMachineCount: ptr.To[int64](1), + WorkerMachineCount: ptr.To[int64(3)], + ClusterctlVariables: map[string]string{ + "GCP_SUBNET_NAME": "capg-test-subnet", + "GCP_SUBNET_CIDR": "172.20.0.0/16", + }, + }, + WaitForClusterIntervals: e2eConfig.GetIntervals(specName, "wait-cluster"), + WaitForControlPlaneIntervals: e2eConfig.GetIntervals(specName, "wait-control-plane"), + WaitForMachinePools: e2eConfig.GetIntervals(specName, "wait-worker-machine-pools"), + }, result) + }) + }) })