AWS Route 53 provider: fix various problems with handling of alias records #1860
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @njuettner |
|
@igor-nikiforov unfortunately not, that's in a different code path. |
|
/kind bug |
There was a problem hiding this comment.
@devkid Thanks for fixing the issues with ALIAS records.
I support the idea of the EndpointModifier approach. It removes unrelated code from the ideally provider-independent Plan phase into each provider. I've ran this version against our e2e tests to see if it succeeds in addition to the unit tests here. It passes just fine (don't mind the unrelated failing test) but note that we don't use custom values for provider-specific stuff like EvaluateTargetHealth, WeightedRecords and so on there, I think.
Regarding the code: I don't understand the purpose of all the changes. Maybe you can shed some more light on the different parts, such as the removal of the recordsCache and the need to rewrite the ALIAS detection. We should make sure that we don't break the existing (working) behaviour.
provider/aws/aws.go
Outdated
| @@ -394,15 +398,13 @@ func (p *AWSProvider) DeleteRecords(ctx context.Context, endpoints []*endpoint.E | |||
|
|
|||
| func (p *AWSProvider) doRecords(ctx context.Context, action string, endpoints []*endpoint.Endpoint) error { | |||
| zones, err := p.Zones(ctx) | |||
| p.ModifyEndpoints(endpoints) | |||
There was a problem hiding this comment.
This should come after the if err != nil { below.
provider/aws/aws.go
Outdated
| @@ -509,20 +502,43 @@ 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.
Please summarize the modification logic in a comment or in the code. The code alone is hard to understand.
|
@linki similar to what I commented in #1849 and #1861 , I'm using this for a refactor. I do think the idea is good as well and can solve #1849 as well. I don't think the additional interface is a good idea though even though simpler than just modifying the provider interface. I think we should embrace the change that we want to do to the provider. I will submit soon a PR that should likely replace this one and #1849. Can you give us a way to at least run your end to end tests on the ExternalDNS repo? |
|
Sounds good to me. I think we don't have an automated setup for PR builds from this repository at the moment but I'll check again. |
k8s-ci-robot
added
the
needs-rebase
|
@devkid thank you for your effort 🙏🏻 , can we push this again? Sorry it didn't get much attention the last months. I saw there are still some open comments, do you mind address them so we can merge it hopefully 🤞🏻 ? |
|
I'll look into it. |
alfredkrohmer
force-pushed
the
aws-fix-alias-records
branch
from
ea6aec7
to
ae1fa42
Compare
k8s-ci-robot
removed
the
needs-rebase
alfredkrohmer
force-pushed
the
aws-fix-alias-records
branch
from
ae1fa42
to
4345ce6
Compare
|
@njuettner rebased |
|
@njuettner I already had adjusted the test for |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devkid, njuettner 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 |
k8s-ci-robot
added
the
approved
This PR supersedes #1356. It's rebased against master, has some commits squashed and I will keep rebasing until it's merged.
Description
The management of Route 53 alias records is utterly broken in its current state, see issues #1105 and #1175.
This PR introduces a new way to handle alias records.
ModifyEndpointmethods is introduced in a new (optional)EndpointModifyingProviderinterface that providers can implement. It allows the AWS provider to convert CNAME endpoints (generated from various sources) to alias A records before a change plan is calculated. This allows proper deletion and update handling of alias records. Due to this change, the provider-specific check for theaws/evaluate-target-healthprovider-specific property can be removed from the plan calculation. See here for a lenghtly explanation on how this works.isAWSAliasfunction is completely reimplemented to fix creation of alias records for targets that are not a load balancer.Fixes #1105. Fixes #1175.
Checklist