From 75281ee0677178fa82a8f542e412c8c3cd3c0188 Mon Sep 17 00:00:00 2001 From: Aleksandra Gacek Date: Wed, 15 Nov 2023 18:07:17 +0100 Subject: [PATCH] Allow overriding domain suffix in GCE cloud provider. --- .../gce/autoscaling_gce_client.go | 6 +- .../gce/autoscaling_gce_client_test.go | 2 +- .../cloudprovider/gce/gce_cloud_provider.go | 5 +- .../cloudprovider/gce/gce_manager.go | 5 +- .../cloudprovider/gce/gce_url.go | 27 +- .../cloudprovider/gce/gce_url_test.go | 260 ++++++++++++++++-- .../config/autoscaling_options.go | 2 + 7 files changed, 270 insertions(+), 37 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go index 164f7221df78..e75689a77cd5 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go @@ -115,6 +115,7 @@ type autoscalingGceClientV1 struct { gceService *gce.Service projectId string + domainUrl string // These can be overridden, e.g. for testing. operationWaitTimeout time.Duration @@ -141,7 +142,7 @@ func NewAutoscalingGceClientV1(client *http.Client, projectId string, userAgent // NewCustomAutoscalingGceClientV1 creates a new client using custom server url and timeouts // for communicating with GCE v1 API. -func NewCustomAutoscalingGceClientV1(client *http.Client, projectId, serverUrl, userAgent string, +func NewCustomAutoscalingGceClientV1(client *http.Client, projectId, serverUrl, userAgent, domainUrl string, waitTimeout, pollInterval time.Duration, deletionPollInterval time.Duration) (*autoscalingGceClientV1, error) { gceService, err := gce.New(client) if err != nil { @@ -153,6 +154,7 @@ func NewCustomAutoscalingGceClientV1(client *http.Client, projectId, serverUrl, return &autoscalingGceClientV1{ projectId: projectId, gceService: gceService, + domainUrl: domainUrl, operationWaitTimeout: waitTimeout, operationPollInterval: pollInterval, operationDeletionPollInterval: deletionPollInterval, @@ -295,7 +297,7 @@ func (client *autoscalingGceClientV1) DeleteInstances(migRef GceRef, instances [ SkipInstancesOnValidationError: true, } for _, i := range instances { - req.Instances = append(req.Instances, GenerateInstanceUrl(i)) + req.Instances = append(req.Instances, GenerateInstanceUrl(client.domainUrl, i)) } op, err := client.gceService.InstanceGroupManagers.DeleteInstances(migRef.Project, migRef.Zone, migRef.Name, &req).Do() if err != nil { diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go index d4968591233b..ef142fb9f90e 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go @@ -228,7 +228,7 @@ func TestErrors(t *testing.T) { func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { const goodInstanceUrlTempl = "https://content.googleapis.com/compute/v1/projects/myprojid/zones/myzone/instances/myinst_%d" - const badInstanceUrl = "https://badurl.com/compute/v1/projects/myprojid/zones/myzone/instances/myinst" + const badInstanceUrl = "https://badurl.com/compute/v1/projects3/myprojid/zones/myzone/instances/myinst" server := test_util.NewHttpServerMock() defer server.Close() g := newTestAutoscalingGceClient(t, "project1", server.URL, "") diff --git a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go index 76dd7964f6c3..d32c64dcb686 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go @@ -197,6 +197,7 @@ type gceMig struct { gceManager GceManager minSize int maxSize int + domainUrl string } // GceRef returns Mig's GceRef @@ -307,7 +308,7 @@ func (mig *gceMig) DeleteNodes(nodes []*apiv1.Node) error { // Id returns mig url. func (mig *gceMig) Id() string { - return GenerateMigUrl(mig.gceRef) + return GenerateMigUrl(mig.domainUrl, mig.gceRef) } // Debug returns a debug string for the Mig. @@ -369,7 +370,7 @@ func BuildGCE(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDiscover defer config.Close() } - manager, err := CreateGceManager(config, do, opts.Regional, opts.GCEOptions.ConcurrentRefreshes, opts.UserAgent, opts.GCEOptions.MigInstancesMinRefreshWaitTime) + manager, err := CreateGceManager(config, do, opts.Regional, opts.GCEOptions.ConcurrentRefreshes, opts.UserAgent, opts.GCEOptions.DomainUrl, opts.GCEOptions.MigInstancesMinRefreshWaitTime) if err != nil { klog.Fatalf("Failed to create GCE Manager: %v", err) } diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager.go b/cluster-autoscaler/cloudprovider/gce/gce_manager.go index e00dec554113..a7f565f6abf8 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager.go @@ -114,6 +114,7 @@ type gceManagerImpl struct { location string projectId string + domainUrl string templates *GceTemplateBuilder interrupt chan struct{} regional bool @@ -123,7 +124,7 @@ type gceManagerImpl struct { } // CreateGceManager constructs GceManager object. -func CreateGceManager(configReader io.Reader, discoveryOpts cloudprovider.NodeGroupDiscoveryOptions, regional bool, concurrentGceRefreshes int, userAgent string, migInstancesMinRefreshWaitTime time.Duration) (GceManager, error) { +func CreateGceManager(configReader io.Reader, discoveryOpts cloudprovider.NodeGroupDiscoveryOptions, regional bool, concurrentGceRefreshes int, userAgent, domainUrl string, migInstancesMinRefreshWaitTime time.Duration) (GceManager, error) { // Create Google Compute Engine token. var err error tokenSource := google.ComputeTokenSource("") @@ -192,6 +193,7 @@ func CreateGceManager(configReader io.Reader, discoveryOpts cloudprovider.NodeGr explicitlyConfigured: make(map[GceRef]bool), concurrentGceRefreshes: concurrentGceRefreshes, reserved: &GceReserved{}, + domainUrl: domainUrl, } if err := manager.fetchExplicitMigs(discoveryOpts.NodeGroupSpecs); err != nil { @@ -416,6 +418,7 @@ func (m *gceManagerImpl) buildMigFromSpec(s *dynamic.NodeGroupSpec) (Mig, error) gceManager: m, minSize: s.MinSize, maxSize: s.MaxSize, + domainUrl: m.domainUrl, } return mig, nil } diff --git a/cluster-autoscaler/cloudprovider/gce/gce_url.go b/cluster-autoscaler/cloudprovider/gce/gce_url.go index e0f04ee91008..de9cd73c6978 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_url.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_url.go @@ -22,12 +22,9 @@ import ( ) const ( - gceUrlSchema = "https" - gceDomainSuffix = "googleapis.com/compute/v1/projects/" - // Cluster Autoscaler previously used "content" instead of "www" here, for reasons unknown. - gcePrefix = gceUrlSchema + "://www." + gceDomainSuffix - instanceUrlTemplate = gcePrefix + "%s/zones/%s/instances/%s" - migUrlTemplate = gcePrefix + "%s/zones/%s/instanceGroups/%s" + gceUrlSchema = "https" + projectsSubstring = "/projects/" + defaultDomainUrl = "https://www.googleapis.com/compute/v1" ) // ParseMigUrl expects url in format: @@ -64,24 +61,32 @@ func ParseInstanceUrlRef(url string) (GceRef, error) { } // GenerateInstanceUrl generates url for instance. -func GenerateInstanceUrl(ref GceRef) string { +func GenerateInstanceUrl(domainUrl string, ref GceRef) string { + if domainUrl == "" { + domainUrl = defaultDomainUrl + } + instanceUrlTemplate := domainUrl + projectsSubstring + "%s/zones/%s/instances/%s" return fmt.Sprintf(instanceUrlTemplate, ref.Project, ref.Zone, ref.Name) } // GenerateMigUrl generates url for instance. -func GenerateMigUrl(ref GceRef) string { +func GenerateMigUrl(domainUrl string, ref GceRef) string { + if domainUrl == "" { + domainUrl = defaultDomainUrl + } + migUrlTemplate := domainUrl + projectsSubstring + "%s/zones/%s/instanceGroups/%s" return fmt.Sprintf(migUrlTemplate, ref.Project, ref.Zone, ref.Name) } func parseGceUrl(url, expectedResource string) (project string, zone string, name string, err error) { - errMsg := fmt.Errorf("wrong url: expected format https://www.googleapis.com/compute/v1/projects//zones//%s/, got %s", expectedResource, url) - if !strings.Contains(url, gceDomainSuffix) { + errMsg := fmt.Errorf("wrong url: expected format /projects//zones//%s/, got %s", expectedResource, url) + if !strings.Contains(url, projectsSubstring) { return "", "", "", errMsg } if !strings.HasPrefix(url, gceUrlSchema) { return "", "", "", errMsg } - splitted := strings.Split(strings.Split(url, gceDomainSuffix)[1], "/") + splitted := strings.Split(strings.Split(url, projectsSubstring)[1], "/") if len(splitted) != 5 || splitted[1] != "zones" { return "", "", "", errMsg } diff --git a/cluster-autoscaler/cloudprovider/gce/gce_url_test.go b/cluster-autoscaler/cloudprovider/gce/gce_url_test.go index e048396b6cf2..1317ef76e605 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_url_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_url_test.go @@ -17,29 +17,249 @@ limitations under the License. package gce import ( + "fmt" "testing" "github.com/stretchr/testify/assert" ) -func TestParseUrl(t *testing.T) { - proj, zone, name, err := parseGceUrl("https://www.googleapis.com/compute/v1/projects/mwielgus-proj/zones/us-central1-b/instanceGroups/kubernetes-minion-group", "instanceGroups") - assert.Nil(t, err) - assert.Equal(t, "mwielgus-proj", proj) - assert.Equal(t, "us-central1-b", zone) - assert.Equal(t, "kubernetes-minion-group", name) - - // Cluster Autoscaler previously used this format for MIG id (with "content" instead of "www"). Make sure it's still accepted - // just to be safe. - proj, zone, name, err = parseGceUrl("https://content.googleapis.com/compute/v1/projects/mwielgus-proj/zones/us-central1-b/instanceGroups/kubernetes-minion-group", "instanceGroups") - assert.Nil(t, err) - assert.Equal(t, "mwielgus-proj", proj) - assert.Equal(t, "us-central1-b", zone) - assert.Equal(t, "kubernetes-minion-group", name) - - _, _, _, err = parseGceUrl("www.onet.pl", "instanceGroups") - assert.NotNil(t, err) - - _, _, _, err = parseGceUrl("https://www.googleapis.com/compute/vabc/projects/mwielgus-proj/zones/us-central1-b/instanceGroups/kubernetes-minion-group", "instanceGroups") - assert.NotNil(t, err) +func TestGenerateInstanceUrl(t *testing.T) { + tests := []struct { + name string + domainUrl string + ref GceRef + want string + }{ + { + name: "empty domain url", + ref: GceRef{ + Project: "proj1", + Name: "name1", + Zone: "us-central1-a", + }, + want: "https://www.googleapis.com/compute/v1/projects/proj1/zones/us-central1-a/instances/name1", + }, + { + name: "custom url", + ref: GceRef{ + Project: "proj1", + Name: "name1", + Zone: "us-central1-a", + }, + domainUrl: "https://www.googleapis.com/compute-custom/v2", + want: "https://www.googleapis.com/compute-custom/v2/projects/proj1/zones/us-central1-a/instances/name1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, GenerateInstanceUrl(tt.domainUrl, tt.ref), "GenerateInstanceUrl(%v, %v)", tt.domainUrl, tt.ref) + }) + } +} + +func TestGenerateMigUrl(t *testing.T) { + tests := []struct { + name string + domainUrl string + ref GceRef + want string + }{ + { + name: "no custom url", + ref: GceRef{ + Project: "proj1", + Name: "name1", + Zone: "us-central1-a", + }, + want: "https://www.googleapis.com/compute/v1/projects/proj1/zones/us-central1-a/instanceGroups/name1", + }, + { + name: "custom url", + domainUrl: "https://www.googleapis.com/compute-custom/v2", + ref: GceRef{ + Project: "proj1", + Name: "name1", + Zone: "us-central1-a", + }, + want: "https://www.googleapis.com/compute-custom/v2/projects/proj1/zones/us-central1-a/instanceGroups/name1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, GenerateMigUrl(tt.domainUrl, tt.ref), "GenerateMigUrl(%v, %v)", tt.domainUrl, tt.ref) + }) + } +} + +func TestParseIgmUrl(t *testing.T) { + tests := []struct { + name string + url string + wantProject string + wantZone string + wantName string + wantErr error + }{ + { + name: "default domain", + url: "https://www.googleapis.com/compute/v1/projects/proj1/zones/us-central1-a/instanceGroupManagers/name1", + wantProject: "proj1", + wantName: "name1", + wantZone: "us-central1-a", + }, + { + name: "custom domain", + url: "https://www.googleapis.com/compute_test/v1/projects/proj1/zones/us-central1-a/instanceGroupManagers/name1", + wantProject: "proj1", + wantName: "name1", + wantZone: "us-central1-a", + }, + { + name: "incorrect domain", + url: "https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instanceGroupManagers2/name1", + wantErr: fmt.Errorf("wrong url: expected format /projects//zones//instanceGroupManagers/, got https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instanceGroupManagers2/name1"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotProject, gotZone, gotName, err := ParseIgmUrl(tt.url) + assert.Equal(t, tt.wantErr, err) + if tt.wantErr != nil { + return + } + assert.Equalf(t, tt.wantProject, gotProject, "ParseIgmUrl(%v)", tt.url) + assert.Equalf(t, tt.wantZone, gotZone, "ParseIgmUrl(%v)", tt.url) + assert.Equalf(t, tt.wantName, gotName, "ParseIgmUrl(%v)", tt.url) + }) + } +} + +func TestParseInstanceUrl(t *testing.T) { + tests := []struct { + name string + url string + wantProject string + wantZone string + wantName string + wantErr error + }{ + { + name: "default domain", + url: "https://www.googleapis.com/compute/v1/projects/proj1/zones/us-central1-a/instances/name1", + wantProject: "proj1", + wantName: "name1", + wantZone: "us-central1-a", + }, + { + name: "custom domain", + url: "https://www.googleapis.com/compute_test/v1/projects/proj1/zones/us-central1-a/instances/name1", + wantProject: "proj1", + wantName: "name1", + wantZone: "us-central1-a", + }, + { + name: "incorrect domain", + url: "https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instances2/name1", + wantErr: fmt.Errorf("wrong url: expected format /projects//zones//instances/, got https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instances2/name1"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotProject, gotZone, gotName, err := ParseInstanceUrl(tt.url) + assert.Equal(t, tt.wantErr, err) + if tt.wantErr != nil { + return + } + assert.Equalf(t, tt.wantProject, gotProject, "ParseInstanceUrl(%v)", tt.url) + assert.Equalf(t, tt.wantZone, gotZone, "ParseInstanceUrl(%v)", tt.url) + assert.Equalf(t, tt.wantName, gotName, "ParseInstanceUrl(%v)", tt.url) + }) + } +} + +func TestParseInstanceUrlRef(t *testing.T) { + tests := []struct { + name string + url string + want GceRef + wantErr error + }{ + { + name: "default domain", + url: "https://www.googleapis.com/compute/v1/projects/proj1/zones/us-central1-a/instances/name1", + want: GceRef{ + Project: "proj1", + Name: "name1", + Zone: "us-central1-a", + }, + }, + { + name: "custom domain", + url: "https://www.googleapis.com/compute_test/v1/projects/proj1/zones/us-central1-a/instances/name1", + want: GceRef{ + Project: "proj1", + Name: "name1", + Zone: "us-central1-a", + }, + }, + { + name: "incorrect domain", + url: "https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instances2/name1", + wantErr: fmt.Errorf("wrong url: expected format /projects//zones//instances/, got https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instances2/name1"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseInstanceUrlRef(tt.url) + assert.Equal(t, tt.wantErr, err) + if tt.wantErr != nil { + return + } + assert.Equalf(t, tt.want, got, "ParseInstanceUrlRef(%v)", tt.url) + }) + } +} + +func TestParseMigUrl(t *testing.T) { + + tests := []struct { + name string + url string + wantProject string + wantZone string + wantName string + wantErr error + }{ + { + name: "default domain", + url: "https://www.googleapis.com/compute/v1/projects/proj1/zones/us-central1-a/instanceGroups/name1", + wantProject: "proj1", + wantName: "name1", + wantZone: "us-central1-a", + }, + { + name: "custom domain", + url: "https://www.googleapis.com/compute_test/v1/projects/proj1/zones/us-central1-a/instanceGroups/name1", + wantProject: "proj1", + wantName: "name1", + wantZone: "us-central1-a", + }, + { + name: "incorrect domain", + url: "https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instanceGroups/name1", + wantErr: fmt.Errorf("wrong url: expected format /projects//zones//instanceGroups/, got https://www.googleapis.com/compute_test/v1/projects2/proj1/zones/us-central1-a/instanceGroups/name1"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotProject, gotZone, gotName, err := ParseMigUrl(tt.url) + assert.Equal(t, tt.wantErr, err) + if tt.wantErr != nil { + return + } + assert.Equalf(t, tt.wantProject, gotProject, "ParseMigUrl(%v)", tt.url) + assert.Equalf(t, tt.wantZone, gotZone, "ParseMigUrl(%v)", tt.url) + assert.Equalf(t, tt.wantName, gotName, "ParseMigUrl(%v)", tt.url) + }) + } } diff --git a/cluster-autoscaler/config/autoscaling_options.go b/cluster-autoscaler/config/autoscaling_options.go index 9e008a11a6f1..1932b856521a 100644 --- a/cluster-autoscaler/config/autoscaling_options.go +++ b/cluster-autoscaler/config/autoscaling_options.go @@ -62,6 +62,8 @@ type GCEOptions struct { MigInstancesMinRefreshWaitTime time.Duration // ExpanderEphemeralStorageSupport is whether scale-up takes ephemeral storage resources into account. ExpanderEphemeralStorageSupport bool + // DomainUrl is the GCE url used to make calls to GCE API. + DomainUrl string } const (