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.
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
Bug 1866299: Fixes Route53 Client Handling of GovCloud Partition #454
Bug 1866299: Fixes Route53 Client Handling of GovCloud Partition #454
Conversation
e853eb1
to
c8d0618
Compare
@danehans: This pull request references Bugzilla bug 1866299, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
case sess.Config.Region != nil: | ||
region = aws.StringValue(sess.Config.Region) | ||
log.Info("using region from shared config", "region name", region) | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The region preference has been inverted to prefer infrastructure config over local AWS env vars to improve local development and troubleshooting.
pkg/dns/aws/dns.go
Outdated
@@ -451,6 +453,7 @@ func (m *Provider) updateRecord(domain, zoneID, target, targetHostedZoneID, acti | |||
ResourceRecordSet: &route53.ResourceRecordSet{ | |||
Name: aws.String(domain), | |||
Type: aws.String(route53.RRTypeA), | |||
TTL: aws.Int64(ttl), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miciah I added TTL to Alias records for consistency between the record types. Let me know if you prefer that TTL only be added to CNAME records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing the following repeated in the ingress operator logs from the last e2e-aws-operator run:
2020-09-05T01:08:17.596Z ERROR operator.dns_controller dns/controller.go:181 failed to publish DNS record to zone {"record": {"dnsName":"*.apps.ci-op-5pzpx1c2-43abb.origin-ci-int-aws.dev.rhcloud.com.","targets":["aa404373c70e04efebd0275f95643892-1078231396.us-west-2.elb.amazonaws.com"],"recordType":"CNAME","recordTTL":30}, "dnszone": {"id":"Z2GYOLTZHS5VK"}, "error": "failed to update alias in zone Z2GYOLTZHS5VK: couldn't update DNS record in zone Z2GYOLTZHS5VK: InvalidInput: Invalid request: Expected exactly one of [AliasTarget, all of [TTL, and ResourceRecords], or TrafficPolicyInstanceId], but found more than one in Change with [Action=UPSERT, Name=*.apps.ci-op-5pzpx1c2-43abb.origin-ci-int-aws.dev.rhcloud.com., Type=A, SetIdentifier=null]\n\tstatus code: 400, request id: a8d0dc74-4e7d-492b-bbb3-2d55696052ec"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miciah thanks for catching this. That's a similar error that required I add TTL to the CNAME record. Let me push a new commit with the TTL removed for the Alias record.
c8d0618
to
35fe77d
Compare
@danehans: This pull request references Bugzilla bug 1866299, which is valid. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
1 similar comment
@danehans: This pull request references Bugzilla bug 1866299, which is valid. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
pkg/dns/aws/dns.go
Outdated
case m.config.ServiceEndpoints != nil: | ||
for _, ep := range m.config.ServiceEndpoints { | ||
if ep.Name == Route53Service { | ||
if strings.Contains(ep.URL, govCloudRoute53Region) { | ||
return true | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be simplified like the following?
case m.config.ServiceEndpoints != nil: | |
for _, ep := range m.config.ServiceEndpoints { | |
if ep.Name == Route53Service { | |
if strings.Contains(ep.URL, govCloudRoute53Region) { | |
return true | |
} | |
} | |
} | |
case strings.Contains(m.route53.Endpoint, govCloudRoute53Region): | |
return true |
Is it necessary to check both the partition and the endpoint?
What do you think about making this a function for simplicity and testability?
func (m *Provider) updateRecord(domain, zoneID, target, targetHostedZoneID, action string, ttl int64) error {
// ...
if clientEndpointIsGovCloud(&m.route53.Client.ClientInfo) {
// ...
// clientEndpointIsGovCloud returns true if the provided client info references
// a US GovCloud API endpoint.
func clientEndpointIsGovCloud(clientInfo *metadata.ClientInfo) bool {
return strings.Contains(clientInfo.Endpoint, govCloudRoute53Region)
}
```
8de5780
to
fb5c6a2
Compare
/lgtm |
@Miciah commit |
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/hold |
/test e2e-aws-operator |
fb5c6a2
to
83bedf4
Compare
Commit |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, Miciah 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 |
/hold cancel |
@danehans: All pull requests linked via external trackers have merged:
Bugzilla bug 1866299 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
/assign @Miciah @knobunc
/cc @frobware @sgreene570