CORS-4062: Migrate endpoints in pkg/types/aws/platform.go to sdk v2 #9759
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4062 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @patrickdillon |
|
[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 |
barbacbd
force-pushed
the
CORS-4062
branch
2 times, most recently
from
3abaee7 to
38bbd03
Compare
There was a problem hiding this comment.
Looks good!
I agree with the idea here! There is also an "unanswered" question aws/aws-sdk-go-v2#2351. But more importantly, there is a V2 version of EndpointResolver (which does not help) that I am unsure if it will replace V1 one day.
| if trustBundle != "" && awstypes.IsSecretRegion(installConfig.Config.AWS.Region) { | ||
| isSecretRegion, err := awsic.IsSecretRegion(ctx, installConfig.Config.AWS.Region) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to determine is aws region is secret: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("failed to determine is aws region is secret: %w", err) | |
| return fmt.Errorf("failed to determine if AWS region is secret: %w", err) |
nit: typo ^-^
| ctx, cancel := context.WithTimeout(ctx, time.Second) | ||
| defer cancel() | ||
| // Need to make a call with the client to activate the endpoint resolver. | ||
| _, err = client.DescribeRegions(ctx, &ec2.DescribeRegionsInput{AllRegions: aws.Bool(false)}) | ||
| if err != nil { | ||
| if !errors.Is(err, context.DeadlineExceeded) { | ||
| return false, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks like we can resolve the endpoint directly without initiating a probe API call, maybe? I did the following (basically just this part from your code only) 🤔
// IsSecretRegion determines if the region is part of a secret partition.
func IsSecretRegion(region string) (bool, error) {
endpoint, err := ec2.NewDefaultEndpointResolver().ResolveEndpoint(region, ec2.EndpointResolverOptions{})
if err != nil {
return false, fmt.Errorf("failed to resolve aws ec2 endpoint: %w", err)
}
// TODO: Remove this
logrus.Infof("Endpoint Partition: %s", endpoint.PartitionID)
switch endpoint.PartitionID {
case isoPartition, isobPartition:
return true, nil
}
return false, nil
}And I tested it with command openshift-install create permissions-policy --log-level=debug --dir=. (also need setting amiID for contrplane and compute in install-config.yaml)
us-iso-east-1: INFO Endpoint Partition: aws-iso
us-isob-east-1: INFO Endpoint Partition: aws-iso-b
us-east-1: INFO Endpoint Partition: aws
barbacbd
force-pushed
the
CORS-4062
branch
2 times, most recently
from
679d3eb to
d1fc77a
Compare
| if trustBundle != "" && awstypes.IsSecretRegion(installConfig.Config.AWS.Region) { | ||
| isSecretRegion, err := awsic.IsSecretRegion(installConfig.Config.AWS.Region) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to determine is AWS region is secret: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("failed to determine is AWS region is secret: %w", err) | |
| return fmt.Errorf("failed to determine if AWS region is secret: %w", err) |
typo?
pkg/types/aws: ** Remove the endpoints package from aws sdk v1. ** Remove the function to check for secret regions pkg/asset/installconfig/aws: ** Add a function to check for secret regions. ** The function was moved here (and changed) because the types package should not make api calls. As we migrated to aws sdk v2 the partitions are no longer publicly available BUT we can access them through endpoints. To get the partition we go through the ec2 api (make a proxy call) and get the endpoint from that call then the endpoint will provide us with the partition.
|
/label platform/aws |
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
pkg/types/aws:
** Remove the endpoints package from aws sdk v1.
** Remove the function to check for secret regions
pkg/asset/installconfig/aws:
** Add a function to check for secret regions.
** The function was moved here (and changed) because the types package should not make api calls. As we migrated to aws sdk v2 the partitions are no longer publicly available BUT we can access them through endpoints. To get the partition we go through the ec2 api (make a proxy call) and get the endpoint from that call then the endpoint will provide us with the partition.