From 6ee0f95056bb0c7d6a18e4bb39e559d67cb770e2 Mon Sep 17 00:00:00 2001 From: dantiandb Date: Tue, 17 Oct 2023 13:26:53 -0700 Subject: [PATCH 1/3] fix bug where maintenance exclusions were not being set independently from other maintenance policy options --- ...ster.x-k8s.io_gcpmanagedcontrolplanes.yaml | 4 +- .../v1beta1/gcpmanagedcontrolplane_types.go | 6 +-- .../gcpmanagedcontrolplane_webhook_test.go | 3 +- exp/api/v1beta1/types.go | 37 ++++++++++++------- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml index 60e400af0..166f97d28 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml @@ -60,7 +60,7 @@ spec: properties: addonsConfig: description: AddonsConfig represents the configuration options for - GKE cluster add ons. + GKE cluster add-ons. properties: dnsCacheConfig: description: DNSCacheConfig represents a configuration for NodeLocalDNS, @@ -424,7 +424,7 @@ spec: workloadIdentityConfig: description: WorkloadIdentityConfig represents configuration options for the use of Kubernetes Service Accounts in GCP IAM policies. - This feature is diabled if this field is not specified. + This feature is disabled if this field is not specified. properties: workloadPool: description: WorkloadPool represents the node pool to attach all diff --git a/exp/api/v1beta1/gcpmanagedcontrolplane_types.go b/exp/api/v1beta1/gcpmanagedcontrolplane_types.go index a204e262e..28a20c60c 100644 --- a/exp/api/v1beta1/gcpmanagedcontrolplane_types.go +++ b/exp/api/v1beta1/gcpmanagedcontrolplane_types.go @@ -60,7 +60,7 @@ type GCPManagedControlPlaneSpec struct { // Endpoint represents the endpoint used to communicate with the control plane. // +optional Endpoint clusterv1.APIEndpoint `json:"endpoint"` - // AddonsConfig represents the configuration options for GKE cluster add ons. + // AddonsConfig represents the configuration options for GKE cluster add-ons. // +optional AddonsConfig *AddonsConfig `json:"addonsConfig,omitempty"` // LoggingConfig represents the configuration options for GKE cluster logging. @@ -79,7 +79,7 @@ type GCPManagedControlPlaneSpec struct { // +optional PrivateClusterConfig *PrivateClusterConfig `json:"privateClusterConfig,omitempty"` // WorkloadIdentityConfig represents configuration options for the use of Kubernetes Service Accounts in GCP IAM - // policies. This feature is diabled if this field is not specified. + // policies. This feature is disabled if this field is not specified. // +optional WorkloadIdentityConfig *WorkloadIdentityConfig `json:"workloadIdentityConfig,omitempty"` // ResourceLabels represents the resource labels for the GKE cluster to use to annotate any related @@ -279,7 +279,7 @@ const ( NoMinorOrNodeUpgrades MaintenanceExclusionOption = "no-minor-or-node-upgrades" ) -// AddonsConfig contains configurations for various add ons available to run in the GKE cluster. +// AddonsConfig contains configurations for various add-ons available to run in the GKE cluster. type AddonsConfig struct { // DNSCacheConfig represents a configuration for NodeLocalDNS, a dns cache running on GKE cluster nodes // If omitted it is disabled by default. diff --git a/exp/api/v1beta1/gcpmanagedcontrolplane_webhook_test.go b/exp/api/v1beta1/gcpmanagedcontrolplane_webhook_test.go index d4bc1651c..4c468119f 100644 --- a/exp/api/v1beta1/gcpmanagedcontrolplane_webhook_test.go +++ b/exp/api/v1beta1/gcpmanagedcontrolplane_webhook_test.go @@ -304,8 +304,7 @@ var _ = Describe("GCPManagedControlPlaneWebhook", func() { oldControlPlane.Spec.NetworkConfig = &NetworkConfig{ DNSConfig: &DNSConfig{}, } - warnings, err := controlPlane.ValidateUpdate(oldControlPlane) - Expect(warnings).To(BeNil()) + err := controlPlane.ValidateUpdate(oldControlPlane) Expect(err).To(Not(BeNil())) Expect(err.Error()).To(ContainSubstring("spec.NetworkConfig.DNSConfig")) }) diff --git a/exp/api/v1beta1/types.go b/exp/api/v1beta1/types.go index 132f565e1..1ccba323b 100644 --- a/exp/api/v1beta1/types.go +++ b/exp/api/v1beta1/types.go @@ -237,16 +237,23 @@ func ConvertToSdkMaintenancePolicy(policy *MaintenancePolicy) (*containerpb.Main } exclusions[k] = tw } + maintenancePolicy.Window = &containerpb.MaintenanceWindow{ + MaintenanceExclusions: exclusions, + } } if policy.DailyMaintenanceWindow != nil { - maintenancePolicy.Window = &containerpb.MaintenanceWindow{ - Policy: &containerpb.MaintenanceWindow_DailyMaintenanceWindow{ - DailyMaintenanceWindow: &containerpb.DailyMaintenanceWindow{ - StartTime: policy.DailyMaintenanceWindow.StartTime, - }, + dailyMaintenanceWindowPolicy := &containerpb.MaintenanceWindow_DailyMaintenanceWindow{ + DailyMaintenanceWindow: &containerpb.DailyMaintenanceWindow{ + StartTime: policy.DailyMaintenanceWindow.StartTime, }, - MaintenanceExclusions: exclusions, + } + if maintenancePolicy.Window == nil { + maintenancePolicy.Window = &containerpb.MaintenanceWindow{ + Policy: dailyMaintenanceWindowPolicy, + } + } else { + maintenancePolicy.Window.Policy = dailyMaintenanceWindowPolicy } } @@ -255,14 +262,18 @@ func ConvertToSdkMaintenancePolicy(policy *MaintenancePolicy) (*containerpb.Main if err != nil { return nil, err } - maintenancePolicy.Window = &containerpb.MaintenanceWindow{ - Policy: &containerpb.MaintenanceWindow_RecurringWindow{ - RecurringWindow: &containerpb.RecurringTimeWindow{ - Window: tw, - Recurrence: policy.RecurringMaintenanceWindow.Recurrence, - }, + recurringWindowPolicy := &containerpb.MaintenanceWindow_RecurringWindow{ + RecurringWindow: &containerpb.RecurringTimeWindow{ + Window: tw, + Recurrence: policy.RecurringMaintenanceWindow.Recurrence, }, - MaintenanceExclusions: exclusions, + } + if maintenancePolicy.Window == nil { + maintenancePolicy.Window = &containerpb.MaintenanceWindow{ + Policy: recurringWindowPolicy, + } + } else { + maintenancePolicy.Window.Policy = recurringWindowPolicy } } From c6a23975264188305f75840e5ede93563df3d4c8 Mon Sep 17 00:00:00 2001 From: Cong Yue Date: Fri, 3 Nov 2023 11:35:29 -0700 Subject: [PATCH 2/3] Support using self-managed networking for GKE clusters (#67) * Support using self-managed networking for GKE clusters --------- Co-authored-by: Richard Chen --- cloud/scope/managedcluster.go | 8 +++ .../services/container/clusters/reconcile.go | 4 +- .../gcpmanagedcluster_controller.go | 49 +++++++++++-------- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/cloud/scope/managedcluster.go b/cloud/scope/managedcluster.go index 1ace53d87..c4d6c038d 100644 --- a/cloud/scope/managedcluster.go +++ b/cloud/scope/managedcluster.go @@ -282,3 +282,11 @@ func (s *ManagedClusterScope) PatchObject() error { func (s *ManagedClusterScope) Close() error { return s.PatchObject() } + +// IsUsingSelfManagerNetworkingForManagedCluster checks if the GKE cluster is using self-managed networking. +// In this case, users specify the VPC network and subnetwork for the cluster. +func (s *ManagedClusterScope) IsUsingSelfManagerNetworkingForManagedCluster() bool { + // The GCPManagedCluster is using self-managed networking if it has a VPC network name and a subnetwork name. + // The subnetwork name is required if the VPC network name is specified. + return s.GCPManagedCluster.Spec.Network.Name != nil && s.GCPManagedCluster.Spec.Network.Subnets != nil && len(s.GCPManagedCluster.Spec.Network.Subnets[0].Name) > 0 +} diff --git a/cloud/services/container/clusters/reconcile.go b/cloud/services/container/clusters/reconcile.go index 6d2da5bc1..9a894b206 100644 --- a/cloud/services/container/clusters/reconcile.go +++ b/cloud/services/container/clusters/reconcile.go @@ -275,7 +275,9 @@ func (s *Service) createCluster(ctx context.Context, log *logr.Logger) error { if !s.scope.IsAutopilotCluster() { cluster.NodePools = scope.ConvertToSdkNodePools(nodePools, machinePools, isRegional, cluster.Name) } - + if s.scope.GCPManagedCluster.Spec.Network.Name != nil && s.scope.GCPManagedCluster.Spec.Network.Subnets != nil && len(s.scope.GCPManagedCluster.Spec.Network.Subnets[0].Name) > 0 { + cluster.Subnetwork = s.scope.GCPManagedCluster.Spec.Network.Subnets[0].Name + } if s.scope.GCPManagedControlPlane.Spec.ClusterIpv4Cidr != nil { cluster.ClusterIpv4Cidr = *s.scope.GCPManagedControlPlane.Spec.ClusterIpv4Cidr } diff --git a/exp/controllers/gcpmanagedcluster_controller.go b/exp/controllers/gcpmanagedcluster_controller.go index 80d4d5fe3..873fc3b12 100644 --- a/exp/controllers/gcpmanagedcluster_controller.go +++ b/exp/controllers/gcpmanagedcluster_controller.go @@ -194,18 +194,22 @@ func (r *GCPManagedClusterReconciler) reconcile(ctx context.Context, clusterScop } clusterScope.SetFailureDomains(failureDomains) - reconcilers := map[string]cloud.Reconciler{ - "networks": networks.New(clusterScope), - "subnets": subnets.New(clusterScope), - } + if !clusterScope.IsUsingSelfManagerNetworkingForManagedCluster() { + reconcilers := map[string]cloud.Reconciler{ + "networks": networks.New(clusterScope), + "subnets": subnets.New(clusterScope), + } - for name, r := range reconcilers { - log.V(4).Info("Calling reconciler", "reconciler", name) - if err := r.Reconcile(ctx); err != nil { - log.Error(err, "Reconcile error", "reconciler", name) - record.Warnf(clusterScope.GCPManagedCluster, "GCPManagedClusterReconcile", "Reconcile error - %v", err) - return ctrl.Result{}, err + for name, r := range reconcilers { + log.V(4).Info("Calling reconciler", "reconciler", name) + if err := r.Reconcile(ctx); err != nil { + log.Error(err, "Reconcile error", "reconciler", name) + record.Warnf(clusterScope.GCPManagedCluster, "GCPManagedClusterReconcile", "Reconcile error - %v", err) + return ctrl.Result{}, err + } } + } else { + log.Info("Using shared VPC, skipping network and subnetwork reconcile") } clusterScope.SetReady() @@ -232,19 +236,22 @@ func (r *GCPManagedClusterReconciler) reconcileDelete(ctx context.Context, clust log.Info("GCPManagedControlPlane not deleted yet, retry later") return ctrl.Result{RequeueAfter: reconciler.DefaultRetryTime}, nil } + if !clusterScope.IsUsingSelfManagerNetworkingForManagedCluster() { + reconcilers := map[string]cloud.Reconciler{ + "subnets": subnets.New(clusterScope), + "networks": networks.New(clusterScope), + } - reconcilers := map[string]cloud.Reconciler{ - "subnets": subnets.New(clusterScope), - "networks": networks.New(clusterScope), - } - - for name, r := range reconcilers { - log.V(4).Info("Calling reconciler delete", "reconciler", name) - if err := r.Delete(ctx); err != nil { - log.Error(err, "Reconcile error", "reconciler", name) - record.Warnf(clusterScope.GCPManagedCluster, "GCPManagedClusterReconcile", "Reconcile error - %v", err) - return ctrl.Result{}, err + for name, r := range reconcilers { + log.V(4).Info("Calling reconciler delete", "reconciler", name) + if err := r.Delete(ctx); err != nil { + log.Error(err, "Reconcile error", "reconciler", name) + record.Warnf(clusterScope.GCPManagedCluster, "GCPManagedClusterReconcile", "Reconcile error - %v", err) + return ctrl.Result{}, err + } } + } else { + log.Info("Using shared VPC, skipping network and subnetwork deletion") } controllerutil.RemoveFinalizer(clusterScope.GCPManagedCluster, infrav1exp.ClusterFinalizer) From 1359b16dfe017f8e6f45906a63537e342ed556f9 Mon Sep 17 00:00:00 2001 From: Cong Yue Date: Thu, 4 Jan 2024 16:23:22 -0800 Subject: [PATCH 3/3] Support additional pod ranges for GKE clusters (#68) * support additional pod ranges for GKE clusters --- cloud/services/container/clusters/compare.go | 21 ++++++++ .../services/container/clusters/reconcile.go | 54 +++++++++++++++++++ ...ster.x-k8s.io_gcpmanagedcontrolplanes.yaml | 7 +++ .../v1beta1/gcpmanagedcontrolplane_types.go | 4 ++ .../v1beta1/gcpmanagedcontrolplane_webhook.go | 12 ++++- exp/api/v1beta1/types.go | 3 ++ 6 files changed, 99 insertions(+), 2 deletions(-) diff --git a/cloud/services/container/clusters/compare.go b/cloud/services/container/clusters/compare.go index 85bddffab..21850d835 100644 --- a/cloud/services/container/clusters/compare.go +++ b/cloud/services/container/clusters/compare.go @@ -226,3 +226,24 @@ func compareWorkloadIdentityConfig(a, b *containerpb.WorkloadIdentityConfig) boo return cmp.Equal(a, b, cmpopts.IgnoreUnexported(containerpb.WorkloadIdentityConfig{})) } +func diffTwoArrays(a, b []string) (inA, inB []string) { + m := make(map[string]bool) + + for _, item := range a { + m[item] = true + } + + for _, item := range b { + if _, ok := m[item]; ok { + delete(m, item) + } else { + inB = append(inB, item) + } + } + + for item := range m { + inA = append(inA, item) + } + + return inA, inB +} diff --git a/cloud/services/container/clusters/reconcile.go b/cloud/services/container/clusters/reconcile.go index 9a894b206..ae94c8955 100644 --- a/cloud/services/container/clusters/reconcile.go +++ b/cloud/services/container/clusters/reconcile.go @@ -478,6 +478,19 @@ func (s *Service) checkDiffAndUpdateCluster(ctx context.Context, existingCluster log.Info("cluster MaintenancePolicy updating in progress") return true, nil } + + // check if need update AdditionalPodRangesConfig + needUpdateAdditionalPodRangesConfig, updateAdditionalPodRangesConfigRequest := s.checkDiffAndPrepareUpdateAdditionalPodRangesConfig(existingCluster, log) + if needUpdateAdditionalPodRangesConfig { + log.Info("AdditionalPodRangesConfig update required") + err := s.updateCluster(ctx, updateAdditionalPodRangesConfigRequest, log) + if err != nil { + return false, err + } + log.Info("cluster AdditionalPodRangesConfig updating in progress") + return true, nil + } + // No updates needed return false, nil } @@ -689,3 +702,44 @@ func (s *Service) checkDiffAndPrepareUpdateMaintenancePolicy(existingCluster *co log.V(4).Info("MaintenancePolicy update request. ", "needUpdate", needUpdate, "updateClusterRequest", &setMaintenancePolicyRequest) return needUpdate, &setMaintenancePolicyRequest, nil } + +func (s *Service) checkDiffAndPrepareUpdateAdditionalPodRangesConfig(existingCluster *containerpb.Cluster, log *logr.Logger) (bool, *containerpb.UpdateClusterRequest) { + needUpdate := false + clusterUpdate := containerpb.ClusterUpdate{} + // Handle if either desired or existing IPAllocationPolicy is nil. In this case, it could suggest that IPAllocationPolicy is not used + if s.scope.GCPManagedControlPlane.Spec.IPAllocationPolicy == nil || existingCluster.IpAllocationPolicy == nil { + updateClusterRequest := containerpb.UpdateClusterRequest{ + Name: s.scope.ClusterFullName(), + Update: &clusterUpdate, + } + return needUpdate, &updateClusterRequest + } + + // compare AdditionalRanges between desired and existing + // Add pod ranges into AdditionalPodRangesConfig if they are not in existing AdditionalPodRangesConfig but only in the new AdditionalPodRangesConfig + // Add pod ranges into RemovedAdditionalPodRangesConfig if they are in existing AdditionalPodRangesConfig but not in the new AdditionalPodRangesConfig + existingPodRangeNames := []string{} + if existingCluster.IpAllocationPolicy.AdditionalPodRangesConfig != nil { + existingPodRangeNames = existingCluster.IpAllocationPolicy.AdditionalPodRangesConfig.PodRangeNames + } + desiredPodRangeNames := s.scope.GCPManagedControlPlane.Spec.IPAllocationPolicy.AdditionalPodRangeNames + if !cmp.Equal(s.scope.GCPManagedControlPlane.Spec.IPAllocationPolicy.AdditionalPodRangeNames, existingPodRangeNames) { + needUpdate = true + additionalPodRanges, removedPodRanges := diffTwoArrays(desiredPodRangeNames, existingPodRangeNames) + additionalPodRangesConfig := &containerpb.AdditionalPodRangesConfig{ + PodRangeNames: additionalPodRanges, + } + removedPodRangesConfig := &containerpb.AdditionalPodRangesConfig{ + PodRangeNames: removedPodRanges, + } + clusterUpdate.AdditionalPodRangesConfig = additionalPodRangesConfig + clusterUpdate.RemovedAdditionalPodRangesConfig = removedPodRangesConfig + log.V(2).Info("AdditionalPodRangesConfig update required", "added", additionalPodRanges, "removed", removedPodRanges, "current", existingPodRangeNames, "desired", desiredPodRangeNames) + } + updateClusterRequest := containerpb.UpdateClusterRequest{ + Name: s.scope.ClusterFullName(), + Update: &clusterUpdate, + } + log.V(4).Info("AdditionalPodRangesConfig update cluster request. ", "needUpdate", needUpdate, "updateClusterRequest", &updateClusterRequest) + return needUpdate, &updateClusterRequest +} diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml index 166f97d28..c44039bcd 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml @@ -139,6 +139,13 @@ spec: GKE cluster IP allocation. If not specified then GKE default values will be used. properties: + additionalPodRangeNames: + description: AdditionalRanges represents the list of secondary + pod ranges to be used for the GKE cluster. This field is only + applicable when use_ip_aliases is set to true. + items: + type: string + type: array clusterIpv4CidrBlock: description: ClusterIpv4CidrBlock represents the IP address range for the GKE cluster pod IPs. If this field is set, then GCPManagedControlPlaneSpec.ClusterIpv4Cidr diff --git a/exp/api/v1beta1/gcpmanagedcontrolplane_types.go b/exp/api/v1beta1/gcpmanagedcontrolplane_types.go index 28a20c60c..1fdc0d138 100644 --- a/exp/api/v1beta1/gcpmanagedcontrolplane_types.go +++ b/exp/api/v1beta1/gcpmanagedcontrolplane_types.go @@ -207,6 +207,10 @@ type IPAllocationPolicy struct { // If not specified the range will be chosen with the default size. // +optional ServicesIpv4CidrBlock *string `json:"servicesIpv4CidrBlock,omitempty"` + // AdditionalRanges represents the list of secondary pod ranges to be used for the GKE cluster. + // This field is only applicable when use_ip_aliases is set to true. + // +optional + AdditionalPodRangeNames []string `json:"additionalPodRangeNames,omitempty"` } // MaintenancePolicy represents configuration options for the GKE cluster maintenance policy. diff --git a/exp/api/v1beta1/gcpmanagedcontrolplane_webhook.go b/exp/api/v1beta1/gcpmanagedcontrolplane_webhook.go index d19912a20..ac48bec1a 100644 --- a/exp/api/v1beta1/gcpmanagedcontrolplane_webhook.go +++ b/exp/api/v1beta1/gcpmanagedcontrolplane_webhook.go @@ -149,11 +149,19 @@ func (r *GCPManagedControlPlane) ValidateUpdate(oldRaw runtime.Object) error { r.Spec.ResourceLabels, "field is immutable"), ) } + rCopy := r.DeepCopy() + oldCopy := old.DeepCopy() - if !cmp.Equal(r.Spec.IPAllocationPolicy, old.Spec.IPAllocationPolicy) { + if rCopy.Spec.IPAllocationPolicy != nil { + rCopy.Spec.IPAllocationPolicy.AdditionalPodRangeNames = []string{} + } + if oldCopy.Spec.IPAllocationPolicy != nil { + oldCopy.Spec.IPAllocationPolicy.AdditionalPodRangeNames = []string{} + } + if !cmp.Equal(rCopy.Spec.IPAllocationPolicy, oldCopy.Spec.IPAllocationPolicy) { allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "IPAllocationPolicy"), - r.Spec.IPAllocationPolicy, "field is immutable"), + r.Spec.IPAllocationPolicy, "field is immutable except for AdditionalPodRangesConfig"), ) } diff --git a/exp/api/v1beta1/types.go b/exp/api/v1beta1/types.go index 1ccba323b..c45fc3f57 100644 --- a/exp/api/v1beta1/types.go +++ b/exp/api/v1beta1/types.go @@ -178,6 +178,9 @@ func ConvertToSdkIPAllocationPolicy(policy *IPAllocationPolicy) *containerpb.IPA ServicesSecondaryRangeName: pointer.StringDeref(policy.ServicesSecondaryRangeName, ""), ClusterIpv4CidrBlock: pointer.StringDeref(policy.ClusterIpv4CidrBlock, ""), ServicesIpv4CidrBlock: pointer.StringDeref(policy.ServicesIpv4CidrBlock, ""), + AdditionalPodRangesConfig: &containerpb.AdditionalPodRangesConfig{ + PodRangeNames: policy.AdditionalPodRangeNames, + }, } }