-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GCE: Use network project id for firewall/route mgmt and zone listing #48560
GCE: Use network project id for firewall/route mgmt and zone listing #48560
Conversation
/assign @bowei |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly comments about comments
@@ -132,6 +134,7 @@ type Config struct { | |||
TokenURL string `gcfg:"token-url"` | |||
TokenBody string `gcfg:"token-body"` | |||
ProjectID string `gcfg:"project-id"` | |||
NetworkProjectID string `gcfg:"network-project-id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to have a comment here why this is different than ProjectID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add documentation about which objects are associated with instances vs network...
@@ -157,26 +160,38 @@ func (g *GCECloud) GetComputeService() *compute.Service { | |||
// newGCECloud creates a new instance of GCECloud. | |||
func newGCECloud(config io.Reader) (*GCECloud, error) { | |||
apiEndpoint := "" | |||
projectID, zone, err := getProjectAndZone() | |||
|
|||
// projectNumber is a project identifier known to be numeric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is somewhat confusing. Probably should expand to something like:
// projectNumber is the numeric identifier. Note: there is also a unique string-based project identifier as well (see https://cloud.google.com/resource-manager/docs/creating-managing-projects#identifying_projects)
@@ -132,6 +134,7 @@ type Config struct { | |||
TokenURL string `gcfg:"token-url"` | |||
TokenBody string `gcfg:"token-body"` | |||
ProjectID string `gcfg:"project-id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment // ProjectID can either be the numeric or string-based unique identifier that starts with [a-z]
// Default networkProjectID to known network project number | ||
networkProjectID := networkProjectNumber | ||
|
||
// Rebuild the network URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this comment?
networkProjectID = cfg.Global.NetworkProjectID | ||
} | ||
|
||
// Determine if cluster is on shared VPC network based off config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] seems clearer:
... VPC network according to config
@@ -46,6 +46,8 @@ import ( | |||
const ( | |||
ProviderName = "gce" | |||
|
|||
defaultAPIEndpoint = "https://www.googleapis.com/compute/v1/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] looks a bit better below if leave off the trailing '/'
if err != nil { | ||
return nil, mc.Observe(err) | ||
} | ||
return list.Items, mc.Observe(err) | ||
} | ||
|
||
func (gce *GCECloud) getRegionLink(region string) string { | ||
return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%v/regions/%v", gce.projectID, region) | ||
return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%v/regions/%v", gce.networkProjectID, region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix API endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving to #48568
0d67e66
to
7f12eb3
Compare
741c5ea
to
c255635
Compare
c255635
to
06e3286
Compare
#48574 uses |
/test pull-kubernetes-kubemark-e2e-gce |
1 similar comment
/test pull-kubernetes-kubemark-e2e-gce |
/assign @vishh |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, nicksardo Associated issue: 48515 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 48497, 48604, 48599, 48560, 48546) |
Revert "Merge pull request #48560 from nicksardo/gce-network-project"
if
conditions: fixes GCE: Config's network name ignored when it's not a url #48521getNetworkNameViaAPICall
which was used as a last resort for thenetworkURL
(if empty) which was previously filled with the metadata network project & name.Release note: