skip service validation to get the default regions endpoint #733
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @kmala. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
pkg/token/token.go
Outdated
| @@ -392,6 +392,18 @@ type tokenVerifier struct { | |||
| validSTShostnames map[string]bool | |||
| } | |||
|
|
|||
| func getDefaultHostNameForRegion(partition *endpoints.Partition, region string) (string, error) { | |||
| rep, err := partition.EndpointFor(stsServiceID, region, endpoints.STSRegionalEndpointOption, endpoints.ResolveUnknownServiceOption) | |||
There was a problem hiding this comment.
qs for familiarity: Looks like endpoints.ResolveUnknownServiceOption will return an error [here[(https://github.com/aws/aws-sdk-go/blob/fbe26bf118cf10a5c6aef4a105e2a5609c617b1a/aws/endpoints/v3model.go#L218-L221). What was the current behavior of partition.EndpointFor when endpoints.ResolveUnknownServiceOption was false?
There was a problem hiding this comment.
currently it would throw service not found https://github.com/kubernetes-sigs/aws-iam-authenticator/blob/master/pkg/token/token.go#L412-L413 as the services are not yet updated in the sdk go
There was a problem hiding this comment.
discussed offline, will add an unit test here
| if err != nil { | ||
| logrus.WithError(err).Error("Error getting default hostname") | ||
| } else { | ||
| validSTShostnames[stsHostName] = true |
There was a problem hiding this comment.
qs for familiarity: can you describe how this helps? when the sdk does not have the partition, the else block will not be taken. How does the rest of the flow succeed?
There was a problem hiding this comment.
when the sdk doesn't have partition we fail as we are now aware of what a default dns hostname should be. This would help where there is partition but the services are not yet defined under the partition.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jyotimahapatra, kmala 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 |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
size/L
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/ok-to-test |
|
/lgtm |
cherry pick of #733 onto release-0.6 branch
What this PR does / why we need it:
We had previously made changes to validate token if the hostname matches the instance metadata region in #556 . However it won't work if this is a new partition as the new partition may not have the sdk updated with the services. Hence extending the same logic to validate token even if the service doesn't exist in the sdk. This should be fine as we are only validating the instance metadata region and just ignoring if the service exists in the partition in the sdk which is the done by the default sdk client also https://github.com/aws/aws-sdk-go/blob/main/aws/session/session.go#L936
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #