Skip to content

Commit 8a6a459

Browse files
ingwonsongingwonsong
authored
Use the zone from the metadata for creating locality (istio#53821)
* Use the zone from the metadata Change-Id: I27f1b699f7ea464eb43993eb7870ee15f7ffc8ad * Fix the test cases. Change-Id: I63c0f955d6ed3a6051e34baa05ef9f05bbd46001 * Reduce the timeout to 3 sec Change-Id: Iac06b870add610464da13bc92d54817b8f5753bb * Reflect comments and add GCP_ZONE env var for overriding Change-Id: Ibc72d0dc3800fca7de781edb3858e454fdb9716b * Fix the failure in the unit test Change-Id: I5e38ccdc61621d933e595bfab9a45a0bede1bc32
1 parent aa93c1f commit 8a6a459

File tree

2 files changed

+96
-72
lines changed

2 files changed

+96
-72
lines changed

‎pkg/bootstrap/platform/gcp.go

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727

2828
"cloud.google.com/go/compute/metadata"
2929
core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
30+
"go.uber.org/atomic"
3031
"golang.org/x/oauth2"
3132
"golang.org/x/oauth2/google"
3233

@@ -48,12 +49,14 @@ const (
4849
GCEInstanceTemplate = "gcp_gce_instance_template"
4950
GCEInstanceCreatedBy = "gcp_gce_instance_created_by"
5051
GCPQuotaProject = "gcp_quota_project"
52+
GCPZone = "gcp_zone"
5153
)
5254

5355
// GCPStaticMetadata holds the statically defined GCP metadata
5456
var GCPStaticMetadata = func() map[string]string {
5557
gcpm := env.Register("GCP_METADATA", "", "Pipe separated GCP metadata, schemed as PROJECT_ID|PROJECT_NUMBER|CLUSTER_NAME|CLUSTER_ZONE").Get()
5658
quota := env.Register("GCP_QUOTA_PROJECT", "", "Allows specification of a quota project to be used in requests to GCP APIs.").Get()
59+
zone := env.Register("GCP_ZONE", "", "GCP Zone where the workload is running on.").Get()
5760
if len(gcpm) == 0 {
5861
return map[string]string{}
5962
}
@@ -73,6 +76,10 @@ var GCPStaticMetadata = func() map[string]string {
7376
if clusterURL, err := constructGKEClusterURL(md); err == nil {
7477
md[GCPClusterURL] = clusterURL
7578
}
79+
80+
if zone != "" {
81+
md[GCPZone] = zone
82+
}
7683
return md
7784
}()
7885

@@ -87,6 +94,7 @@ var (
8794
numericProjectIDFn = metadata.NumericProjectID
8895
instanceNameFn = metadata.InstanceName
8996
instanceIDFn = metadata.InstanceID
97+
zoneFn = metadata.ZoneWithContext
9098

9199
clusterNameFn = func() (string, error) {
92100
cn, err := metadata.InstanceAttributeValue("cluster-name")
@@ -148,6 +156,8 @@ type gcpEnv struct {
148156
sync.Mutex
149157
metadata map[string]string
150158
fillMetadata lazy.Lazy[bool]
159+
160+
cachedZone *atomic.String
151161
}
152162

153163
// IsGCP returns whether or not the platform for bootstrapping is Google Cloud Platform.
@@ -167,6 +177,7 @@ func NewGCP() Environment {
167177
fillMetadata: lazy.New(func() (bool, error) {
168178
return shouldFillMetadata(), nil
169179
}),
180+
cachedZone: atomic.NewString(GCPStaticMetadata[GCPZone]),
170181
}
171182
}
172183

@@ -249,33 +260,50 @@ func waitForMetadataSuppliers(suppliers []metadataSupplier, md map[string]string
249260
return &wg
250261
}
251262

263+
// Zones are in the form <country_code>-<region_suffix>-<zone_suffix>.
264+
var zoneRE = regexp.MustCompile(`^([^-]+-[^-]+)-[^-]+$`)
265+
252266
// Converts a GCP zone into a region.
253267
func zoneToRegion(z string) (string, error) {
254-
// Zones are in the form <region>-<zone_suffix>, so capture everything but the suffix.
255-
re := regexp.MustCompile("(.*)-.*")
256-
m := re.FindStringSubmatch(z)
268+
m := zoneRE.FindStringSubmatch(z)
257269
if len(m) != 2 {
258270
return "", fmt.Errorf("unable to extract region from GCP zone: %s", z)
259271
}
260272
return m[1], nil
261273
}
262274

275+
// Returns the zone where the pod is located in.
276+
func (e *gcpEnv) getPodZone() (string, error) {
277+
z := e.cachedZone.Load()
278+
if z != "" {
279+
return z, nil
280+
}
281+
ctx, cfn := context.WithTimeout(context.Background(), 10*time.Second)
282+
defer cfn()
283+
z, err := zoneFn(ctx)
284+
if err != nil {
285+
log.Warnf("Error fetching GCP zone from the metadata server: %v", err)
286+
return "", err
287+
}
288+
e.cachedZone.Store(z)
289+
return z, nil
290+
}
291+
263292
// Locality returns the GCP-specific region and zone.
264293
func (e *gcpEnv) Locality() *core.Locality {
265294
var l core.Locality
266-
loc := e.Metadata()[GCPLocation]
267-
if loc == "" {
268-
log.Warnf("Error fetching GCP zone: %v", loc)
295+
z, err := e.getPodZone()
296+
if err != nil {
269297
return &l
270298
}
271-
r, err := zoneToRegion(loc)
299+
r, err := zoneToRegion(z)
272300
if err != nil {
273-
log.Warnf("Error fetching GCP region: %v", err)
301+
log.Warnf("Error fetching GCP region from zone: %v", z, err)
274302
return &l
275303
}
276304
return &core.Locality{
277305
Region: r,
278-
Zone: loc,
306+
Zone: z,
279307
SubZone: "", // GCP has no subzone concept
280308
}
281309
}

‎pkg/bootstrap/platform/gcp_test.go

Lines changed: 59 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package platform
1616

1717
import (
18+
"context"
1819
"errors"
1920
"testing"
2021

@@ -361,92 +362,87 @@ func TestDefaultPort(t *testing.T) {
361362

362363
func TestLocality(t *testing.T) {
363364
tests := []struct {
364-
name string
365-
shouldFill shouldFillFn
366-
projectIDFn metadataFn
367-
numericProjectIDFn metadataFn
368-
locationFn metadataFn
369-
clusterNameFn metadataFn
370-
instanceNameFn metadataFn
371-
instanceIDFn metadataFn
372-
instanceTemplateFn metadataFn
373-
instanceCreatedByFn metadataFn
374-
env map[string]string
375-
want map[string]string
365+
name string
366+
zoneFn func(context.Context) (string, error)
367+
env map[string]string
368+
want map[string]string
376369
}{
377370
{
378371
"fill by env variable",
379-
func() bool { return true },
380-
func() (string, error) { return "pid", nil },
381-
func() (string, error) { return "npid", nil },
382-
func() (string, error) { return "location", nil },
383-
func() (string, error) { return "cluster", nil },
384-
func() (string, error) { return "instanceName", nil },
385-
func() (string, error) { return "instance", nil },
386-
func() (string, error) { return "instanceTemplate", nil },
387-
func() (string, error) { return "createdBy", nil },
372+
func(context.Context) (string, error) { return "us-east1-ir", nil },
388373
map[string]string{
389-
GCPProject: "env_pid",
390-
GCPProjectNumber: "env_pn",
391-
GCPCluster: "env_cluster",
392-
GCPLocation: "us-west1-ir",
374+
GCPZone: "us-central2-ir",
393375
},
394-
map[string]string{"Zone": "us-west1-ir", "Region": "us-west1"},
376+
map[string]string{"Zone": "us-central2-ir", "Region": "us-central2"},
395377
},
396378
{
397-
"no env variable",
398-
func() bool { return true },
399-
func() (string, error) { return "pid", nil },
400-
func() (string, error) { return "npid", nil },
401-
func() (string, error) { return "us-west1-ir", nil },
402-
func() (string, error) { return "cluster", nil },
403-
func() (string, error) { return "instanceName", nil },
404-
func() (string, error) { return "instance", nil },
405-
func() (string, error) { return "instanceTemplate", nil },
406-
func() (string, error) { return "createdBy", nil },
379+
"fill by metadata server",
380+
func(context.Context) (string, error) { return "us-east1-ir", nil },
407381
map[string]string{},
408-
map[string]string{"Zone": "us-west1-ir", "Region": "us-west1"},
382+
map[string]string{"Zone": "us-east1-ir", "Region": "us-east1"},
409383
},
410384
{
411-
"empty result",
412-
func() bool { return false },
413-
func() (string, error) { return "pid", nil },
414-
func() (string, error) { return "npid", nil },
415-
func() (string, error) { return "us-west1-ir", nil },
416-
func() (string, error) { return "cluster", nil },
417-
func() (string, error) { return "instanceName", nil },
418-
func() (string, error) { return "instance", nil },
419-
func() (string, error) { return "instanceTemplate", nil },
420-
func() (string, error) { return "createdBy", nil },
421-
map[string]string{},
422-
map[string]string{},
385+
"fill by env variable without compute metadata",
386+
func(context.Context) (string, error) { return "", errors.New("error") },
387+
map[string]string{
388+
GCPZone: "us-central2-ir",
389+
},
390+
map[string]string{"Zone": "us-central2-ir", "Region": "us-central2"},
423391
},
424392
{
425-
"unable to reach compute metadata",
426-
func() bool { return true },
427-
func() (string, error) { return "", errors.New("error") },
428-
func() (string, error) { return "", errors.New("error") },
429-
func() (string, error) { return "", errors.New("error") },
430-
func() (string, error) { return "cluster", nil },
431-
func() (string, error) { return "instanceName", nil },
432-
func() (string, error) { return "instance", nil },
433-
func() (string, error) { return "instanceTemplate", nil },
434-
func() (string, error) { return "createdBy", nil },
393+
"no env variable and unable to reach compute metadata",
394+
func(context.Context) (string, error) { return "", errors.New("error") },
435395
map[string]string{},
436396
map[string]string{},
437397
},
438398
}
439399
for _, tt := range tests {
440400
t.Run(tt.name, func(t *testing.T) {
441401
test.SetForTest(t, &GCPStaticMetadata, tt.env)
442-
shouldFillMetadata, projectIDFn, numericProjectIDFn, clusterLocationFn, clusterNameFn,
443-
instanceNameFn, instanceIDFn, instanceTemplateFn, createdByFn = tt.shouldFill, tt.projectIDFn,
444-
tt.numericProjectIDFn, tt.locationFn, tt.clusterNameFn, tt.instanceNameFn, tt.instanceIDFn, tt.instanceTemplateFn, tt.instanceCreatedByFn
445-
402+
zoneFn = tt.zoneFn
446403
e := NewGCP()
447404
got := e.Locality()
448405
assert.Equal(t, got.Zone, tt.want["Zone"])
449406
assert.Equal(t, got.Region, tt.want["Region"])
450407
})
451408
}
452409
}
410+
411+
func TestZoneToRegion(t *testing.T) {
412+
tests := []struct {
413+
zone string
414+
wantRegion string
415+
wantErr bool
416+
}{
417+
{
418+
zone: "us-central1-f",
419+
wantRegion: "us-central1",
420+
},
421+
{
422+
zone: "us-central1",
423+
wantErr: true,
424+
},
425+
{
426+
zone: "abcd",
427+
wantErr: true,
428+
},
429+
}
430+
431+
for _, tc := range tests {
432+
t.Run(tc.zone, func(t *testing.T) {
433+
got, err := zoneToRegion(tc.zone)
434+
if tc.wantErr {
435+
if err == nil {
436+
t.Errorf("expected error was not raised")
437+
}
438+
} else {
439+
if err != nil {
440+
t.Fatalf("unexpected error was raised")
441+
}
442+
if got != tc.wantRegion {
443+
t.Errorf("unexpected region was returned. (got: %v, want: %v)", got, tc.wantRegion)
444+
}
445+
}
446+
})
447+
}
448+
}

0 commit comments

Comments
 (0)