Migrate EC2 clients in discovery service to AWS SDK v2 #48950
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
atburke
added
the
no-changelog
rosstimothy
reviewed
Nov 14, 2024
lib/cloud/aws/config/config.go
Outdated
| "github.com/aws/aws-sdk-go-v2/service/sts" | ||
| "github.com/gravitational/trace" | ||
|
|
||
| "github.com/gravitational/teleport/lib/cloud" |
There was a problem hiding this comment.
lib/cloud is a heavy dependency. Consider refactoring/duplicating code to keep this package as self contained as possible.
greedy52
reviewed
Nov 14, 2024
rosstimothy
approved these changes
Nov 15, 2024
lib/srv/discovery/discovery.go
Outdated
Comment on lines
229
to
235
| token, err := c.AccessPoint.GenerateAWSOIDCToken(ctx, integrationName) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| if integration.GetAWSOIDCIntegrationSpec() == nil { | ||
| return nil, trace.BadParameter("integration does not have aws oidc spec fields %q", integrationName) | ||
| } |
There was a problem hiding this comment.
Should a token be generated if the integration spec is nil? Change the order of operations here?
Suggested change
| token, err := c.AccessPoint.GenerateAWSOIDCToken(ctx, integrationName) | |
| if err != nil { | |
| return nil, trace.Wrap(err) | |
| } | |
| if integration.GetAWSOIDCIntegrationSpec() == nil { | |
| return nil, trace.BadParameter("integration does not have aws oidc spec fields %q", integrationName) | |
| } | |
| if integration.GetAWSOIDCIntegrationSpec() == nil { | |
| return nil, trace.BadParameter("integration does not have aws oidc spec fields %q", integrationName) | |
| } | |
| token, err := c.AccessPoint.GenerateAWSOIDCToken(ctx, integrationName) | |
| if err != nil { | |
| return nil, trace.Wrap(err) | |
| } | |
lib/srv/server/ec2_watcher_test.go
Outdated
| output *ec2.DescribeInstancesOutput | ||
| } | ||
|
|
||
| func instanceMatches(inst *ec2.Instance, filters []*ec2.Filter) bool { | ||
| func (m *mockEC2Client) DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, opts ...func(*ec2.Options)) (*ec2.DescribeInstancesOutput, error) { | ||
| output := ec2.DescribeInstancesOutput{} |
There was a problem hiding this comment.
Suggested change
| output := ec2.DescribeInstancesOutput{} | |
| var output ec2.DescribeInstancesOutput |
lib/cloud/aws/config/config.go
Outdated
| return aws.Config{}, trace.BadParameter("missing aws integration credential provider") | ||
| } | ||
|
|
||
| slog.DebugContext(ctx, "Initializing AWS config with integration.", "region", region, "integration", options.integration) |
There was a problem hiding this comment.
Suggested change
| slog.DebugContext(ctx, "Initializing AWS config with integration.", "region", region, "integration", options.integration) | |
| slog.DebugContext(ctx, "Initializing AWS config with integration", "region", region, "integration", options.integration) |
lib/cloud/aws/config/config.go
Outdated
| return aws.Config{}, trace.Wrap(err) | ||
| } | ||
| } else { | ||
| slog.DebugContext(ctx, "Initializing AWS config from environment.", "region", region) |
There was a problem hiding this comment.
Suggested change
| slog.DebugContext(ctx, "Initializing AWS config from environment.", "region", region) | |
| slog.DebugContext(ctx, "Initializing AWS config from environment", "region", region) |
greedy52
reviewed
Nov 18, 2024
lib/srv/server/ec2_watcher.go
Outdated
| for paginator.HasMorePages() { | ||
| page, err := paginator.NextPage(ctx) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) |
There was a problem hiding this comment.
We need to convert the SDK errors to trace errors. Multiple places check trace.IsAccessDenied and trace.IsNotFound.
Conversion example (which is ridiculously tedious):
teleport/lib/cloud/aws/errors.go
Lines 91 to 92 in 3db72b0
greedy52
reviewed
Nov 18, 2024
atburke
force-pushed
the
atburke/ec2-discovery-v2-sdk
branch
from
94c1158 to
d3a486c
Compare
greedy52
approved these changes
Nov 19, 2024
public-teleport-github-review-bot
bot
removed request for
marcoandredinis,
tigrato,
nklaassen and
EdwardDowling
This change migrates the EC2 clients used in the discovery service to the v2 AWS SDK.
atburke
force-pushed
the
atburke/ec2-discovery-v2-sdk
branch
from
d3a486c to
eefd522
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change migrates the EC2 clients used in the discovery service to use the v2 AWS SDK (and also removes EC2 from the giant shared client interface in lib/cloud).