Migrate IAM to aws-sdk-go-v2 #16435
Migrate IAM to aws-sdk-go-v2 #16435
Conversation
k8s-ci-robot
added
cncf-cla: yes
rifelpet
force-pushed
the
aws-sdk-go-v2-iam
branch
from
3d2f019 to
926bc56
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman 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
| } | ||
|
|
||
| func (m *MockIAM) CreateInstanceProfile(request *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { | ||
| func (m *MockIAM) CreateInstanceProfile(ctx context.Context, request *iam.CreateInstanceProfileInput, optFns ...func(*iam.Options)) (*iam.CreateInstanceProfileOutput, error) { |
There was a problem hiding this comment.
Ah cool, I like that the context is there by default now :-)
| m.mutex.Lock() | ||
| defer m.mutex.Unlock() | ||
|
|
||
| klog.Infof("CreateInstanceProfile: %v", request) | ||
|
|
||
| ip := m.InstanceProfiles[aws.StringValue(request.InstanceProfileName)] | ||
| if ip == nil { | ||
| ip, ok := m.InstanceProfiles[aws.ToString(request.InstanceProfileName)] |
There was a problem hiding this comment.
I'm guessing you were thinking about making this a map of structs, rather than a map of pointers, but then decided not to (?)
| @@ -21,8 +21,8 @@ import ( | |||
| "sort" | |||
| "strings" | |||
|
|
|||
| awsIam "github.com/aws/aws-sdk-go-v2/service/iam" | |||
There was a problem hiding this comment.
Nit: we should probably switch awsIam -> awsiam at some stage, it's more go idiomatic I believe
| if err != nil { | ||
| getRoleErr = fmt.Errorf("calling IAM GetRole on %s: %w", fi.ValueOf(role.RoleName), err) |
There was a problem hiding this comment.
Ah, this is going to be a much more ergonomic API :-)
| @@ -500,13 +505,9 @@ func (b *IAMModelBuilder) FindDeletions(context *fi.CloudupModelBuilderContext, | |||
| } | |||
| } | |||
| } | |||
| return true | |||
| }) | |||
| } | |||
| if getRoleErr != nil { | |||
There was a problem hiding this comment.
Nit: this isn't used any more, I believe?
| if errors.As(err, &nse) { | ||
| klog.Warningf("could not find IAM OIDC Provider %q. Resource may already have been deleted: %v", aws.StringValue(arn), nse) | ||
| continue | ||
| } else if awserror, ok := err.(smithy.APIError); ok && awserror.ErrorCode() == "403" { |
There was a problem hiding this comment.
This feels like a more fragile API than we had before. Maybe we should hide if awserror, ok := err.(smithy.APIError); ok && awserror.ErrorCode() == "403" behind a function like IsHTTP403Error(err error) bool ?
| if awsErr.Code() == iam.ErrCodeNoSuchEntityException { | ||
| response, err := cloud.IAM().GetRolePolicy(ctx, request) | ||
| if err != nil { | ||
| var nse *iamtypes.NoSuchEntityException |
There was a problem hiding this comment.
I guess we could also have a helper for this check, but this one is simpler.
| c.iam = iam.New(sess, config) | ||
| c.iam.Handlers.Send.PushFront(requestLogger) | ||
| c.addHandlers(region, &c.iam.Handlers) | ||
| c.iam = iam.NewFromConfig(cfgV2) |
There was a problem hiding this comment.
So we are losing 3 things:
- SharedConfigState
- requestlogger
- addHandlers - which configures the cross-request delay
Do we think any of those are still important?
There was a problem hiding this comment.
SharedConfigState is now default: https://aws.github.io/aws-sdk-go-v2/docs/migrating/#client-construction
there is no way to disable or enable SharedConfigState anymore, only specify which shared config file to use (default ~/.aws/config)
|
Thanks for doing this @rifelpet Generally looks good, but my one "blocker" is whether we've lost things by "just" doing |
I mention the config changes here. I'm hoping that the SDK's new built-in retryer and retry logger will be sufficient for our needs but we'll find out once we run scale tests (and likely wont find out until the largest AWS service is migrated - EC2) I'll address your nits in a followup |
ref: #16424