-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Adding hostedZoneID and alias to endpoints to assist deletion of alias entries on AWS #1356
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @spohner! |
9114d0b
to
9b5584b
Compare
/assign @Raffo |
Great work, @devkid ! I'll test this later today and resolve any conflicts. |
Tested in our cluster and seems to work great for us. |
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 only reason for this change is to differentiate between "ALIAS" and "CNAME"?
Could you explain it to me what the problem is? I probably don't get the full context here.
plan/plan.go
Outdated
@@ -181,12 +181,6 @@ func shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool { | |||
return false | |||
} | |||
for _, c := range current.ProviderSpecific { | |||
// don't consider target health when detecting changes |
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.
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.
@spohner can you please comment on this part?
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.
See point 7 from my lengthly explanation above.
@@ -122,6 +122,12 @@ func (c *Controller) RunOnce() error { | |||
} | |||
sourceEndpointsTotal.Set(float64(len(endpoints))) | |||
|
|||
if prov := c.Registry.Provider(); prov != nil { |
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.
why do you need this inside the controller logic?
@njuettner This is a "multi-fold" problem.
|
Great write up @devkid. Any comments or suggestions @njuettner? |
Is this PR rejected? Let me know if I can assist to get this through. |
It would be much appreciated if this PR could get merged. I deployed 0.6.0 with this PR merged in without any problems which was necessary to address #1175 encountered in our setup. |
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.
Thank you for your PR @spohner and most of all for your patience. I reviewed it as promised on slack and added a few comments.
While I appreciate the explanation in #1356 (comment) , I am not sure of the following:
- I'm not convinced that we need a new interface and I'd love to understand more about that. I guess it is done not to change things everywhere, but it seems like we are breaking the contract with the other provider. Isn't there a way to fix that without modifying the interface?
- Are you sure we strictly need to do the fix in the controller code?
- Where is the part of code that requires the context to be passed and that justifies changing a total of 60 files? If the addition of context is justified, I would propose something to ease the review: let's split this PR in two: one with all the context, and leave this one with all the core parts of the change.
plan/plan.go
Outdated
@@ -181,12 +181,6 @@ func shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool { | |||
return false | |||
} | |||
for _, c := range current.ProviderSpecific { | |||
// don't consider target health when detecting changes |
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.
@spohner can you please comment on this part?
provider/aws.go
Outdated
@@ -471,6 +477,30 @@ func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint, | |||
return changes | |||
} | |||
|
|||
func (p *AWSProvider) ModifyEndpoints(endpoints []*endpoint.Endpoint) { |
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.
Can you add a comment to this function?
provider/aws.go
Outdated
@@ -667,6 +686,18 @@ func changesByZone(zones map[string]*route53.HostedZone, changeSet []*route53.Ch | |||
continue | |||
} | |||
for _, z := range zones { | |||
if c.ResourceRecordSet.AliasTarget != nil && aws.StringValue(c.ResourceRecordSet.AliasTarget.HostedZoneId) == "same-zone" { |
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.
Can we avoid using magic strings like "same-zone"
? If this is the only way to signal such a case, we should at least have a constant.
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.
See my other comment. I agree that having a constant is better.
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.
Converted "same-zone" to a constant with 8d63ec6
provider/aws.go
Outdated
} | ||
|
||
// if not, target needs to be in the same zone | ||
return "same-zone" |
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.
If we are using this to not break the signature of this method, I'd rather break it rather than have magic strings.
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.
It's not only to not break the signature, it's the only way of passing this around in the existing route53.Change
structure (that comes from the AWS SDK). Of course, we could create another struct wrapping this one but this seems a bit overkill.
ApplyChanges(ctx context.Context, changes *plan.Changes) error | ||
} | ||
|
||
type EndpointModifyingProvider interface { |
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.
Why does this needs to be a different interface? Do we have other options?
Also: this should have a comment.
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 just introduced this so that I wouldn't need to implement this as an empty method for all other providers and because I would consider this an "optional" interface of a provider. However, I wouldn't mind at all if you prefer to have this in the Provider
interface.
…erface This avoids that providers can only modify endpoints in their ApplyChanges method, possibly leading to no-op upsert operations because external-dns can't foresee these automatic changes. The prime example for this is automatic conversion of CNAME records to alias records in the Route 53 provider if their target is an ELB/ALB. When retrieving these records from the zone, the provider adds the provider-specific properties for "alias" and "evaluate-target-health" but when extracting the corresponding endpoints from the cluster state, external-dns will not know that the provider will set these attributes automatically and will therefor issue a (no-op) update operation. This change calls a "ModifyEndpoints" method in the provider to let it do these automatic modifications before external-dns calculates the change plan, allowing it do skip the no-op update. As a consequence, the Route 53 provider can move the automatic CNAME to alias conversion in this method (where it only needs to the add the correct provider-specific properties) and in ApplyChanges only needs to act upon these. This also gets rid of the special-case handling of evaluate-target-health in the plan calculation.
…erface This avoids that providers can only modify endpoints in their ApplyChanges method, possibly leading to no-op upsert operations because external-dns can't foresee these automatic changes. The prime example for this is automatic conversion of CNAME records to alias records in the Route 53 provider if their target is an ELB/ALB. When retrieving these records from the zone, the provider adds the provider-specific properties for "alias" and "evaluate-target-health" but when extracting the corresponding endpoints from the cluster state, external-dns will not know that the provider will set these attributes automatically and will therefor issue a (no-op) update operation. This change calls a "ModifyEndpoints" method in the provider to let it do these automatic modifications before external-dns calculates the change plan, allowing it do skip the no-op update. As a consequence, the Route 53 provider can move the automatic CNAME to alias conversion in this method (where it only needs to the add the correct provider-specific properties) and in ApplyChanges only needs to act upon these. This also gets rid of the special-case handling of evaluate-target-health in the plan calculation.
…ded with ModifyEndpoints
8d63ec6
to
c3f7eeb
Compare
New changes are detected. LGTM label has been removed. |
@Raffo this is rebased now. |
/kind bug |
@@ -500,22 +529,12 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint, recordsCac | |||
if val, ok := ep.Labels[endpoint.DualstackLabelKey]; ok { | |||
dualstack = val == "true" | |||
} | |||
|
|||
change.ResourceRecordSet.Type = aws.String(route53.RRTypeA) |
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.
What about CNAME aliases?
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.
Hm this was part wasn't changed in this pull request. I assume CNAME alias records don't work (and didn't work before either). I guess that would be a new feature request.
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.
You're right. This PR's scope is already quite large, and has been pending for a long time. Would love to see it merged so we can add CNAME alias support in a subsequent issue/PR.
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@spohner: PR needs rebase. 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. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Well this is embarassing, the kubernetes bot failed at merging this PR, then it conficted multiple times... can you resolve the conflicts and we try again merging this @spohner ? |
I agree. Let´s close this in favor of #1860 . |
This PR tries to fix #1105 by adding hostedZoneID and alias to endpoints via ProviderSpecific attributes. This allows ALIAS entries targeting record sets in an hosted zone to be deleted. Previously this would fail if the target was not an AWS address.
Unsure if this allows to remove the hardcoded values for hosted zones ids, but it might. I can update this PR with this change if wanted.