-
Notifications
You must be signed in to change notification settings - Fork 319
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
add sts regional endpoint logic #313
Conversation
Welcome @prasita123! |
Hi @prasita123. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
pkg/providers/v1/aws.go
Outdated
@@ -5130,3 +5134,28 @@ func (c *Cloud) describeNetworkInterfaces(nodeName string) (*ec2.NetworkInterfac | |||
} | |||
return eni.NetworkInterfaces[0], nil | |||
} | |||
|
|||
func getRegionFromMetadata(cfg *CloudConfig) (string, error) { |
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.
since this is almost identical to the passage
cloud-provider-aws/pkg/providers/v1/aws.go
Lines 1300 to 1317 in 8cdf512
metadata, err := awsServices.Metadata() | |
if err != nil { | |
return nil, fmt.Errorf("error creating AWS metadata client: %q", err) | |
} | |
err = updateConfigZone(&cfg, metadata) | |
if err != nil { | |
return nil, fmt.Errorf("unable to determine AWS zone from cloud provider config or EC2 instance metadata: %v", err) | |
} | |
zone := cfg.Global.Zone | |
if len(zone) <= 1 { | |
return nil, fmt.Errorf("invalid AWS zone in config file: %s", zone) | |
} | |
regionName, err := azToRegion(zone) | |
if err != nil { | |
return nil, err | |
} |
could you refactor that to also use this function?
/ok-to-test |
pkg/providers/v1/aws.go
Outdated
} | ||
|
||
return regionName, nil | ||
} |
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.
newline is needed her.
Run ./hack/verify-gofmt.sh locally to verify.
pkg/providers/v1/aws.go
Outdated
zone string | ||
} | ||
|
||
func getRegionFromMetadata(cfg CloudConfig, metadata EC2Metadata) (*regionDetails, error) { |
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.
consider returning multiple return values like
(string, string, err)
region, zone, err
instead. I like it better than a special struct
pkg/providers/v1/aws.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
regiondetails, err := getRegionFromMetadata(cfg, metadata) |
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.
must check for err here
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.
I think the lack of err check causes TestNewAWSCloud to fail now after the refactor, try the test locally as well to verify .. https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/cloud-provider-aws/313/pull-cloud-provider-aws-test/1499235237555605504
/test pull-cloud-provider-aws-e2e |
/retest |
/test pull-cloud-provider-aws-e2e |
tested the change on a v1.22 cluster (manifest
|
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: prasita123, wongma7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Yeah let's add one. Which reminds me... we need some mechanism to get release notes out of this don't we? I'm assuming that is buried in the kubernetes release automation but doesn't apply to other repositories. |
not sure it is usable outside k/k but https://github.com/kubernetes/sig-release/blob/d30b2484d7b18f869dd7f560ef5fb52a3b536a32/release-team/role-handbooks/release-notes/editing-flow.md#workflow-operation . @prasita123 plz draft a release note, we can review that as well |
/test pull-cloud-provider-aws-check |
1 similar comment
/test pull-cloud-provider-aws-check |
…-upstream-release-1.22 Automated cherry pick of #313: add sts regional endpoint logic
…f-#313-release-1.21 Automated cherry pick of #313: add sts regional endpoint logic
…f-#313-release-1.20 Automated cherry pick of #313: add sts regional endpoint logic
What type of PR is this?
What this PR does / why we need it:
STS suggests using regional endpoints. (source)
The PR uses sts regional endpoints, fetched using the metadata, instead of global endpoints.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: