-
Notifications
You must be signed in to change notification settings - Fork 617
✨ Migrate ec2 to AWS SDK V2 #5521
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @yiannistri! |
Hi @yiannistri. 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. |
52a7c2f
to
543a41c
Compare
/ok-to-test |
9704c3a
to
9711485
Compare
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.
Thank you for taking on this piece in particular, it's a big one!
metadataOptions.InstanceMetadataTags = infrav1.InstanceMetadataState(*v.MetadataOptions.InstanceMetadataTags) | ||
} | ||
metadataOptions.HTTPEndpoint = infrav1.InstanceMetadataState(string(v.MetadataOptions.HttpEndpoint)) | ||
metadataOptions.HTTPPutResponseHopLimit = int64(*v.MetadataOptions.HttpPutResponseHopLimit) |
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 one would still need a nil check since it's de-referencing a pointer.
9711485
to
4f9ce70
Compare
/retest |
@yiannistri would you be able to rebase? Thanks! |
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.
@yiannistri
Thank you for working on this.
I am late to party but these changes are important for maintaining functionality between SDK v1 and v2
pkg/cloud/scope/clients.go
Outdated
func NewEC2Client(scopeUser cloud.ScopeUsage, session cloud.Session, logger logger.Wrapper, target runtime.Object) *ec2.Client { | ||
cfg := session.SessionV2() | ||
|
||
ec2opts := []func(*ec2.Options){ | ||
func(o *ec2.Options) { | ||
o.Logger = logger.GetAWSLogger() | ||
o.ClientLogMode = awslogs.GetAWSLogLevelV2(logger.GetLogger()) | ||
}, | ||
ec2.WithAPIOptions( | ||
awsmetricsv2.WithMiddlewares(scopeUser.ControllerName(), target), | ||
awsmetricsv2.WithCAPAUserAgentMiddleware(), | ||
), |
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.
@yiannistri Can you create EndpointResolver like this - https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/pkg/cloud/endpointsv2/endpoints.go#L109-L123
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.
Also, the serviceLimiter implementation is also required.
// ToInt64Value converts an int32 pointer to an int64 value. | ||
func ToInt64Value(to *int32) int64 { | ||
if to == nil { | ||
panic("Cannot convert nil pointer to int64") | ||
} | ||
return int64(*to) | ||
} | ||
|
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.
Can we not use aws.Int64 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 don't think we can because the aws
pointer conversion methods convert to a value of the same bit size as the pointer, i.e. int64 to *int64 and vice versa and in this case we convert from *int32 to int64.
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.
Apologigies, I was suggesting aws.ToInt64.
We could use the same logic with it like aws.ToInt64(int64(*int32))
. This way we don't need to this additional code/method and we can avoid panicking.
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.
isn't this code aws.ToInt64(int64(*int32))
going to panic too if *int32 is nil?
@yiannistri |
4f9ce70
to
f7de9ec
Compare
f7de9ec
to
cc17ae5
Compare
cc17ae5
to
e4897d8
Compare
e4897d8
to
182a570
Compare
4604776
to
e9a8513
Compare
e9a8513
to
969ef40
Compare
@yiannistri: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
@yiannistri hey would you be able to rebate now that the GC or merged? Thanks! |
PR needs rebase. 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. |
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
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 #5403, #5411
Special notes for your reviewer:
Checklist:
Release note: