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

dmc5179 · 2022-02-17T04:45:15Z

Currently the operator defaults to using the us-east-1 region endpoint for route53 in the us-isob-east-1 region which does not work. Add support for the route53 in the us-isob-east-1 region.

openshift-ci · 2022-02-17T04:45:48Z

Hi @dmc5179. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

dmc5179 · 2022-02-17T16:11:47Z

/assign @Miciah

pkg/dns/aws/dns.go

…egions which can change

staebler · 2022-03-04T12:55:23Z

pkg/dns/aws/dns.go

@@ -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)


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.

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

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

staebler · 2022-03-04T12:58:43Z

pkg/dns/aws/dns.go

 		tagConfig = nil
-		// Do not override the region in C2s
+		// Do not override the region in C2S or SC2S


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?

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.

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?

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.

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",

},

},

},

},

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

staebler · 2022-04-11T14:20:55Z

/lgtm
@Miciah Could you take a look at this for approval?

staebler · 2022-04-11T14:21:11Z

/label ok-to-test

openshift-ci · 2022-04-11T14:21:13Z

@staebler: The label(s) /label ok-to-test cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, backport-risk-assessed, cherry-pick-approved

In response to this:

/label ok-to-test

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/test-infra repository.

openshift-ci · 2022-04-11T14:21:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dmc5179, staebler
To complete the pull request process, please ask for approval from miciah after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

staebler · 2022-04-11T14:21:53Z

/ok-to-test

rfredette · 2022-04-13T19:57:28Z

/retest

openshift-ci · 2022-04-13T21:59:42Z

@dmc5179: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

patrickdillon · 2022-04-26T20:24:39Z

This is for https://issues.redhat.com/browse/NE-792

mjpytlak · 2022-04-27T13:21:52Z

Providing doc ack. No doc is required for this PR

yunjiang29 · 2022-04-27T13:55:13Z

/qe-approved

yunjiang29 · 2022-04-27T13:58:09Z

/label qe-approved

Miciah · 2022-05-02T16:01:35Z

/label docs-approved
based on #703 (comment).

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

91a8318

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 17, 2022

openshift-ci bot requested review from frobware and Miciah February 17, 2022 04:45

openshift-ci bot assigned Miciah Feb 17, 2022

staebler reviewed Feb 24, 2022

View reviewed changes

pkg/dns/aws/dns.go Outdated Show resolved Hide resolved

Switch to using the AWS partition rather than the hardcoded list of r…

b468cd5

…egions which can change

dmc5179 requested a review from staebler March 4, 2022 11:53

staebler reviewed Mar 4, 2022

View reviewed changes

openshift-ci bot assigned staebler Apr 11, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2022

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 11, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 27, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label May 2, 2022

r4f4 mentioned this pull request May 4, 2022

Add support for route53 in the us-isob-east-1 region + fix #754

Merged

openshift-merge-robot merged commit b468cd5 into openshift:master May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dmc5179 commented Feb 17, 2022

openshift-ci bot commented Feb 17, 2022

dmc5179 commented Feb 17, 2022

staebler Mar 4, 2022

dmc5179 Mar 4, 2022

rfredette Apr 26, 2022

staebler Mar 4, 2022

dmc5179 Mar 4, 2022

dmc5179 Mar 12, 2022

staebler Mar 12, 2022

staebler Mar 12, 2022

staebler Apr 11, 2022

staebler commented Apr 11, 2022

staebler commented Apr 11, 2022

openshift-ci bot commented Apr 11, 2022

openshift-ci bot commented Apr 11, 2022

staebler commented Apr 11, 2022

rfredette commented Apr 13, 2022

openshift-ci bot commented Apr 13, 2022

patrickdillon commented Apr 26, 2022

mjpytlak commented Apr 27, 2022

yunjiang29 commented Apr 27, 2022

yunjiang29 commented Apr 27, 2022

Miciah commented May 2, 2022

	"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",
	},
	},
	},
	},

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

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

Conversation

dmc5179 commented Feb 17, 2022

openshift-ci bot commented Feb 17, 2022

dmc5179 commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staebler commented Apr 11, 2022

staebler commented Apr 11, 2022

openshift-ci bot commented Apr 11, 2022

openshift-ci bot commented Apr 11, 2022

staebler commented Apr 11, 2022

rfredette commented Apr 13, 2022

openshift-ci bot commented Apr 13, 2022

patrickdillon commented Apr 26, 2022

mjpytlak commented Apr 27, 2022

yunjiang29 commented Apr 27, 2022

yunjiang29 commented Apr 27, 2022

Miciah commented May 2, 2022