From 54bbe3968e6ce64995dc33d2d738eb2acd32ac7f Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Thu, 26 Jan 2023 10:16:19 -0800 Subject: [PATCH] address feedback --- api/v1alpha3/zz_generated.deepcopy.go | 2 +- api/v1alpha4/zz_generated.deepcopy.go | 2 +- api/v1beta1/zz_generated.deepcopy.go | 2 +- cloud/scope/managedcontrolplane.go | 15 ++-- cloud/scope/managedmachinepool.go | 18 ++--- .../services/container/nodepools/reconcile.go | 14 ++-- config/manager/manager.yaml | 1 - config/rbac/role.yaml | 9 +++ .../gcpmanagedcontrolplane_controller.go | 2 + .../gcpmanagedmachinepool_controller.go | 6 +- feature/feature.go | 17 ++--- main.go | 43 ----------- util/location/location.go | 50 +++++++++++++ util/resourceurl/parse.go | 75 +++++++++++++++++++ 14 files changed, 169 insertions(+), 87 deletions(-) create mode 100644 util/location/location.go create mode 100644 util/resourceurl/parse.go diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 4741b94625..299ea37129 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package v1alpha3 import ( - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" apiv1alpha3 "sigs.k8s.io/cluster-api/api/v1alpha3" "sigs.k8s.io/cluster-api/errors" diff --git a/api/v1alpha4/zz_generated.deepcopy.go b/api/v1alpha4/zz_generated.deepcopy.go index 34de4c2180..97ae2b10b3 100644 --- a/api/v1alpha4/zz_generated.deepcopy.go +++ b/api/v1alpha4/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package v1alpha4 import ( - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" apiv1alpha4 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/errors" diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 97c66e8590..31b3f42d8b 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package v1beta1 import ( - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" apiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/errors" diff --git a/cloud/scope/managedcontrolplane.go b/cloud/scope/managedcontrolplane.go index 028c9b31dd..c981f54453 100644 --- a/cloud/scope/managedcontrolplane.go +++ b/cloud/scope/managedcontrolplane.go @@ -19,7 +19,8 @@ package scope import ( "context" "fmt" - "strings" + + "sigs.k8s.io/cluster-api-provider-gcp/util/location" "google.golang.org/api/option" @@ -126,7 +127,7 @@ type ManagedControlPlaneScope struct { credentialsClient *credentials.IamCredentialsClient credential *Credential - AllMachinePools []clusterv1exp.MachinePool + AllMachinePools []clusterv1exp.MachinePool AllManagedMachinePools []infrav1exp.GCPManagedMachinePool } @@ -201,16 +202,10 @@ func (s *ManagedControlPlaneScope) GetAllNodePools(ctx context.Context) ([]infra return s.AllManagedMachinePools, s.AllMachinePools, nil } -func parseLocation(location string) string { - parts := strings.Split(location, "-") - region := strings.Join(parts[:2], "-") - return region -} - // Region returns the region of the GKE cluster. func (s *ManagedControlPlaneScope) Region() string { - region := parseLocation(s.GCPManagedControlPlane.Spec.Location) - return region + loc, _ := location.Parse(s.GCPManagedControlPlane.Spec.Location) + return loc.Region } // ClusterLocation returns the location of the cluster. diff --git a/cloud/scope/managedmachinepool.go b/cloud/scope/managedmachinepool.go index 3d64427b13..553062b16b 100644 --- a/cloud/scope/managedmachinepool.go +++ b/cloud/scope/managedmachinepool.go @@ -19,7 +19,8 @@ package scope import ( "context" "fmt" - "strings" + + "sigs.k8s.io/cluster-api-provider-gcp/util/location" "google.golang.org/api/option" "sigs.k8s.io/cluster-api/util/conditions" @@ -164,16 +165,7 @@ func (s *ManagedMachinePoolScope) NodePoolVersion() *string { return s.MachinePool.Spec.Template.Spec.Version } -// ParseInstanceGroupURL parses an instance group URL. -func ParseInstanceGroupURL(url string) (project string, zone string, mig string) { - parts := strings.Split(url, "/") - if len(parts) < 11 { - return "", "", "" - } - return parts[6], parts[8], parts[10] -} - -// ConvertToSdkNodePool convertsSetReplicas a node pool to format that is used by GCP SDK. +// ConvertToSdkNodePool converts a node pool to format that is used by GCP SDK. func ConvertToSdkNodePool(nodePool infrav1exp.GCPManagedMachinePool, machinePool clusterv1exp.MachinePool) *containerpb.NodePool { nodePoolName := nodePool.Spec.NodePoolName if len(nodePoolName) == 0 { @@ -218,8 +210,8 @@ func (s *ManagedMachinePoolScope) NodePoolName() string { // Region returns the region of the GKE node pool. func (s *ManagedMachinePoolScope) Region() string { - region := parseLocation(s.GCPManagedControlPlane.Spec.Location) - return region + loc, _ := location.Parse(s.GCPManagedControlPlane.Spec.Location) + return loc.Region } // NodePoolLocation returns the location of the node pool. diff --git a/cloud/services/container/nodepools/reconcile.go b/cloud/services/container/nodepools/reconcile.go index e174b7b0d0..ecb002f3df 100644 --- a/cloud/services/container/nodepools/reconcile.go +++ b/cloud/services/container/nodepools/reconcile.go @@ -21,6 +21,8 @@ import ( "fmt" "reflect" + "sigs.k8s.io/cluster-api-provider-gcp/util/resourceurl" + "google.golang.org/api/iterator" "google.golang.org/grpc/codes" @@ -219,14 +221,14 @@ func (s *Service) getInstances(ctx context.Context, nodePool *containerpb.NodePo instances := []*computepb.ManagedInstance{} for _, url := range nodePool.InstanceGroupUrls { - project, zone, mig := scope.ParseInstanceGroupURL(url) - if mig == "" { - return nil, fmt.Errorf("fail to parse instance group url %s", url) + resourceURL, err := resourceurl.Parse(url) + if err != nil { + return nil, errors.Wrap(err, "error parsing instance group url") } listManagedInstancesRequest := &computepb.ListManagedInstancesInstanceGroupManagersRequest{ - InstanceGroupManager: mig, - Project: project, - Zone: zone, + InstanceGroupManager: resourceURL.Name, + Project: resourceURL.Project, + Zone: resourceURL.Location, } iter := s.scope.InstanceGroupManagersClient().ListManagedInstances(ctx, listManagedInstancesRequest) for { diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 28f6809272..d75d04a07b 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -21,7 +21,6 @@ spec: containers: - args: - --leader-elect - - --feature-gates=GKE=${EXP_CAPG_GKE:=false} - "--metrics-bind-addr=localhost:8080" image: controller:latest imagePullPolicy: IfNotPresent diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index ab33c4c0fc..fd27661a27 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -33,6 +33,15 @@ rules: - get - list - watch +- apiGroups: + - cluster.x-k8s.io + resources: + - machinepools + - machinepools/status + verbs: + - get + - list + - watch - apiGroups: - cluster.x-k8s.io resources: diff --git a/exp/controllers/gcpmanagedcontrolplane_controller.go b/exp/controllers/gcpmanagedcontrolplane_controller.go index ee7f3389d3..b15a8ac2c7 100644 --- a/exp/controllers/gcpmanagedcontrolplane_controller.go +++ b/exp/controllers/gcpmanagedcontrolplane_controller.go @@ -57,6 +57,8 @@ type GCPManagedControlPlaneReconciler struct { //+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedcontrolplanes,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedcontrolplanes/status,verbs=get;update;patch //+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedcontrolplanes/finalizers,verbs=update +//+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedclusters,verbs=get;list;watch +//+kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch // SetupWithManager sets up the controller with the Manager. func (r *GCPManagedControlPlaneReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { diff --git a/exp/controllers/gcpmanagedmachinepool_controller.go b/exp/controllers/gcpmanagedmachinepool_controller.go index 262ce5d96d..071ed17f3f 100644 --- a/exp/controllers/gcpmanagedmachinepool_controller.go +++ b/exp/controllers/gcpmanagedmachinepool_controller.go @@ -221,6 +221,10 @@ func getOwnerMachinePool(ctx context.Context, c client.Client, obj metav1.Object //+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedmachinepools,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedmachinepools/status,verbs=get;update;patch //+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedmachinepools/finalizers,verbs=update +//+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedcontrolplanes,verbs=get;list;watch +//+kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=gcpmanagedclusters,verbs=get;list;watch +//+kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinepools;machinepools/status,verbs=get;list;watch +//+kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch func (r *GCPManagedMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultedLoopTimeout(r.ReconcileTimeout)) @@ -289,7 +293,7 @@ func (r *GCPManagedMachinePoolReconciler) Reconcile(ctx context.Context, req ctr managedMachinePoolScope, err := scope.NewManagedMachinePoolScope(ctx, scope.ManagedMachinePoolScopeParams{ Client: r.Client, Cluster: cluster, - MachinePool: machinePool, + MachinePool: machinePool, GCPManagedCluster: gcpManagedCluster, GCPManagedControlPlane: gcpManagedControlPlane, GCPManagedMachinePool: gcpManagedMachinePool, diff --git a/feature/feature.go b/feature/feature.go index 419aab8938..b5e5afb5cc 100644 --- a/feature/feature.go +++ b/feature/feature.go @@ -20,16 +20,12 @@ import ( ) const ( - // Every capg-specific feature gate should add method here following this template: - // - // // owner: @username - // // alpha: v1.X - // MyFeature featuregate.Feature = "MyFeature". +// Every capg-specific feature gate should add method here following this template: +// +// // owner: @username +// // alpha: v1.X +// MyFeature featuregate.Feature = "MyFeature". - // GKE is used to enable GKE support - // owner: @richardchen331 & @richardcase - // alpha: v0.1 - GKE featuregate.Feature = "GKE" ) func init() { @@ -39,5 +35,6 @@ func init() { // defaultCAPGFeatureGates consists of all known capg-specific feature keys. // To add a new feature, define a key for it above and add it here. var defaultCAPGFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - GKE: {Default: false, PreRelease: featuregate.Alpha}, + // Every feature should be initiated here: + } diff --git a/main.go b/main.go index 184b71f255..cbfc5a21cf 100644 --- a/main.go +++ b/main.go @@ -38,7 +38,6 @@ import ( infrav1beta1 "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" "sigs.k8s.io/cluster-api-provider-gcp/controllers" infrav1exp "sigs.k8s.io/cluster-api-provider-gcp/exp/api/v1beta1" - expcontrollers "sigs.k8s.io/cluster-api-provider-gcp/exp/controllers" "sigs.k8s.io/cluster-api-provider-gcp/feature" "sigs.k8s.io/cluster-api-provider-gcp/util/reconciler" "sigs.k8s.io/cluster-api-provider-gcp/version" @@ -186,32 +185,6 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) error { return fmt.Errorf("setting up GCPCluster controller: %w", err) } - if feature.Gates.Enabled(feature.GKE) { - if err := (&expcontrollers.GCPManagedClusterReconciler{ - Client: mgr.GetClient(), - ReconcileTimeout: reconcileTimeout, - WatchFilterValue: watchFilterValue, - }).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: gcpClusterConcurrency}); err != nil { - return fmt.Errorf("setting up GCPManagedCluster controller: %w", err) - } - if err := (&expcontrollers.GCPManagedControlPlaneReconciler{ - Client: mgr.GetClient(), - ReconcileTimeout: reconcileTimeout, - WatchFilterValue: watchFilterValue, - }).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: gcpClusterConcurrency}); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "GCPManagedControlPlane") - os.Exit(1) - } - if err := (&expcontrollers.GCPManagedMachinePoolReconciler{ - Client: mgr.GetClient(), - ReconcileTimeout: reconcileTimeout, - WatchFilterValue: watchFilterValue, - }).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: gcpClusterConcurrency}); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "GCPManagedMachinePool") - os.Exit(1) - } - } - return nil } @@ -229,22 +202,6 @@ func setupWebhooks(mgr ctrl.Manager) error { return fmt.Errorf("setting up GCPMachineTemplate webhook: %w", err) } - if feature.Gates.Enabled(feature.GKE) { - if err := (&infrav1exp.GCPManagedCluster{}).SetupWebhookWithManager(mgr); err != nil { - return fmt.Errorf("setting up GCPManagedCluster webhook: %w", err) - } - if err := (&infrav1exp.GCPManagedControlPlane{}).SetupWebhookWithManager(mgr); err != nil { - return fmt.Errorf("setting up GCPManagedControlPlane webhook: %w", err) - } - if err := (&infrav1exp.GCPManagedMachinePool{}).SetupWebhookWithManager(mgr); err != nil { - return fmt.Errorf("setting up GCPManagedMachinePool webhook: %w", err) - } - } - if err := (&infrav1exp.GCPManagedControlPlane{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "GCPManagedControlPlane") - os.Exit(1) - } - return nil } diff --git a/util/location/location.go b/util/location/location.go new file mode 100644 index 0000000000..dd50b3034e --- /dev/null +++ b/util/location/location.go @@ -0,0 +1,50 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package location implements location parsing utilities. +package location + +import ( + "strings" + + "github.com/pkg/errors" +) + +// Location captures the region and zone of a GCP location. +// Examples of GCP location: +// us-central1 (region). +// us-central1-c (region with zone). +type Location struct { + Region string + Zone *string +} + +// Parse parses a location string. +func Parse(location string) (Location, error) { + parts := strings.Split(location, "-") + if len(parts) < 2 { + return Location{}, errors.New("invalid location") + } + region := strings.Join(parts[:2], "-") + var zone *string + if len(parts) == 3 { + zone = &parts[2] + } + return Location{ + Region: region, + Zone: zone, + }, nil +} diff --git a/util/resourceurl/parse.go b/util/resourceurl/parse.go new file mode 100644 index 0000000000..b65d23fc82 --- /dev/null +++ b/util/resourceurl/parse.go @@ -0,0 +1,75 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package resourceurl implements resource url parsing utilities. +package resourceurl + +import ( + "strings" + + "github.com/pkg/errors" +) + +const ( + // ResourcePrefix is the prefix of a resource URL. + ResourcePrefix = "https://www.googleapis.com/" + // NumParts is the number of parts in a resource URL. + NumParts = 8 + // ResourceCategoryIndex is the index of the resource category in resource URL parts. + ResourceCategoryIndex = 0 + // ProjectIndex is the index of the project in resource URL parts. + ProjectIndex = 3 + // LocationIndex is the index of the location in resource URL parts. + LocationIndex = 5 + // SubResourceIndex is the index of the sub resource in resource URL parts. + SubResourceIndex = 6 + // NameIndex is the index of the name in resource URL parts. + NameIndex = 7 +) + +// ResourceURL captures the individual fields of a GCP resource URL. +// An example of GCP resource URL: +// https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-b/instanceGroupManagers/gke-capg-gke-demo-mypool-aa1282e0-grp +type ResourceURL struct { + // The resource category (e.g. compute) + ResourceCategory string + // The project where the resource lives in (e.g. my-project) + Project string + // The location where the resource lives in (e.g. us-central1-b) + Location string + // The sub-type of the resource (e.g. instanceGroupManagers) + SubResource string + // The name of the resource (e.g. gke-capg-gke-demo-mypool-aa1282e0-grp) + Name string +} + +// Parse parses a resource url. +func Parse(url string) (ResourceURL, error) { + if !strings.HasPrefix(url, ResourcePrefix) { + return ResourceURL{}, errors.New("invalid resource url") + } + parts := strings.Split(url[len(ResourcePrefix):], "/") + if len(parts) != NumParts { + return ResourceURL{}, errors.New("invalid resource url") + } + return ResourceURL{ + ResourceCategory: parts[ResourceCategoryIndex], + Project: parts[ProjectIndex], + Location: parts[LocationIndex], + SubResource: parts[SubResourceIndex], + Name: parts[NameIndex], + }, nil +}