-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Migrate AWS Verifier to aws-sdk-go-v2 #16483
Migrate AWS Verifier to aws-sdk-go-v2 #16483
Conversation
WithSTSRegionalEndpoint(endpoints.RegionalSTSEndpoint) | ||
sess, err := session.NewSession(config) | ||
func NewAWSAuthenticator(ctx context.Context, region string) (bootstrap.Authenticator, error) { | ||
config, err := awsconfig.LoadDefaultConfig(ctx, awsconfig.WithRegion(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.
We added WithSTSRegionalEndpoint in #12043 . Is it still supported? (Is it automatic now?)
We don't have great unit test coverage here, so I figured it might be good to add some: #16487 TBH I'm struggling to get the test to pass with v2, trying to figure it out... I think we were already very sensitive to the aws-sdk version, so I don't think this is a huge regression version-wise. |
I think I've figured it out ... the V2 API puts a lot more in the query URL. So we likely need to pass along the url/method/headers. The good thing is that eliminates the dependency on the aws-sdk-go version (going forwards!) Here's my WIP / PoC branch ... let me know if you want me e.g. to send a PR to your branch or something like that. I'm happy to fix up the tests, the code change itself here is pretty simple (we encode an |
6a029bb
to
50e66c7
Compare
Yes, feel free to PR to my branch or just push directly to it. I just rebased to resolve conflicts. Adding the other data in to the token makes sense to me 👍🏻 |
0110fea
to
f97ecaf
Compare
/retest |
We pass the full request details, it's less dependent on client versions.
f97ecaf
to
1e5ed58
Compare
/unhold /cc @hakman |
/lgtm |
Thanks @rifelpet /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb 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 |
Closes #16424
From this comment:
While presigned requests are still supported in V2, the presign methods and types no longer provide access to the request body, only their url and headers. See aws/aws-sdk-go-v2#1137. Kops-controller currently reads the request body to perform some validation:
kops/upup/pkg/fi/cloudup/awsup/aws_verifier.go
Lines 153 to 157 in 1c24423
In V1 the presigned request is a POST however in V2 it is converted to a GET request and the normal
Action=GetCallerIdentity&Version=2011-06-15
body is moved to URL query parameters:https://github.com/aws/aws-sdk-go-v2/blob/bc2a669d3241023e20194cdfe042b8c275887e51/service/sts/api_client.go#L641-L645
I've removed the validation that checks the Content-Length signed header matches the size of the request body, given the request body is now empty.
This thread on the original kops-controller PR discusses potential upgrade challenges. In this case I believe we'll have the normal race with this type of change:
kops update cluster --yes
andkops rolling-update --yes
is ranIn reality I dont anticipate this being a problem because the old node would have needed to not join the cluster in the time it took the control plane to bootstrap and launch the kops-controller daemonset pod (a significantly longer process than normal k8s node bootstrap). Eventually the failed node will be cleaned up by cluster-autoscaler or karpenter.
My only concern would be disruption to
kops rolling-update
if the node never joins which causes cluster validation to fail until it is cleaned up.