Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for route53 in the us-isob-east-1 region #703

Merged
merged 2 commits into from
May 9, 2022
Merged

Add support for route53 in the us-isob-east-1 region #703

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91a8318
Add support for route53 in the us-isob-east-1 region
RedHatDan Feb 17, 2022
b468cd5
Switch to using the AWS partition rather than the hardcoded list of r…
RedHatDan Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions pkg/dns/aws/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,23 +142,28 @@ func NewProvider(config Config, operatorReleaseVersion string) (*Provider, error
elbConfig := aws.NewConfig().WithRegion(region)
tagConfig := aws.NewConfig()

partition, ok := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region)
if !ok {
return nil, fmt.Errorf("Unable to determine partition from region: %q", region)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it may be better here to continue on with an unknown partition and use the default case in the following switch--as opposed to failing when the partition cannot be determined.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the code to use the default case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change still planned? I also agree that it'd be preferable to use the default for unknown regions, rather than error out

}

// If the region is in aws china, cn-north-1 or cn-northwest-1, we should:
// 1. hard code route53 api endpoint to https://route53.amazonaws.com.cn and region to "cn-northwest-1"
// as route53 is not GA in AWS China and aws sdk didn't have the endpoint.
// 2. use the aws china region cn-northwest-1 to setup tagging api correctly instead of "us-east-1"
switch region {
case endpoints.CnNorth1RegionID, endpoints.CnNorthwest1RegionID:
switch partition.ID() {
case endpoints.AwsCnPartitionID:
tagConfig = tagConfig.WithRegion(endpoints.CnNorthwest1RegionID)
r53Config = r53Config.WithRegion(endpoints.CnNorthwest1RegionID).WithEndpoint(chinaRoute53Endpoint)
case endpoints.UsGovEast1RegionID, endpoints.UsGovWest1RegionID:
case endpoints.AwsUsGovPartitionID:
// Route53 for GovCloud uses the "us-gov-west-1" region id:
// https://docs.aws.amazon.com/govcloud-us/latest/UserGuide/using-govcloud-endpoints.html
r53Config = r53Config.WithRegion(endpoints.UsGovWest1RegionID)
// As with other AWS partitions, the GovCloud Tagging client must be
// in the same region as the Route53 client to find the hosted zone
// of managed records.
tagConfig = tagConfig.WithRegion(endpoints.UsGovWest1RegionID)
case endpoints.UsIsoEast1RegionID, endpoints.UsIsobEast1RegionID:
case endpoints.AwsIsoPartitionID, endpoints.AwsIsoBPartitionID:
// The resourcetagging API is not available in C2S or SC2S
tagConfig = nil
// Do not override the region in C2S or SC2S
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the AWS behavior in C2S and SC2S follow other partitions where there is one region used for global resources? In particular, if the region used for the cluster is us-iso-west-1, will the us-iso-east-1 region need to be used when querying the resource tagging API for route53 resources?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to ask AWS about this. I will submit a support case to AWS when I go on site today but might not hear back from them until next week.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted a case to AWS on this topic and heard back today. The "correct" method is to point all route53 requests to route53.c2s.ic.gov but AWS is leaving the route53.us-iso-east-1.c2s.ic.gov alias in place because it has been around for so long that they don't want to break anything by removing it. This means that I need to change the PR because the us-iso-west-1 region will result in route53.us-iso-west-1.cs2.ic.gov which will not exist.

I could change the code to say, if isoB, use route53.sc2s.sgov.gov, if iso use route53.c2s.ic.gov

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we need any of the special logic in this switch any more, regardless of the partition. The AWS SDK selects the correct endpoints to use, and we should be making use of the selections made by the SDK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cluster-ingress-operator/vendor/github.com/aws/aws-sdk-go/aws/endpoints/defaults.go

Lines 10893 to 10905 in fe11656

"route53": service{
PartitionEndpoint: "aws-iso-b-global",
IsRegionalized: boxedFalse,
Endpoints: endpoints{
"aws-iso-b-global": endpoint{
Hostname: "route53.sc2s.sgov.gov",
CredentialScope: credentialScope{
Region: "us-isob-east-1",
},
},
},
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can save the removal of redundant logic for future work.

Expand Down