Kops Compatibility #2: Add Multi-Target Support #326
Conversation
k8s-ci-robot
added
cncf-cla: yes
plan/plan.go
Outdated
| Current []*endpoint.Endpoint | ||
| // List of desired records | ||
| Desired []*endpoint.Endpoint | ||
| CurrentTargets map[RecordKey][]string |
There was a problem hiding this comment.
It's a bit nit picky, but I would say the Current/Desired are still appropriate. The CurrentTargets made me do a double take on what the map actually represented. The RecordKey makes it more than just the Target
|
I just tested this on GCP and it worked fabolous. I'll need to do lots of testing on AWS. |
|
I will deploy this to our cluster in AWS as well. |
| for _, ep := range eps { | ||
| targets := []string{} | ||
| for _, target := range ep.Targets { | ||
| if ok := isAWSLoadBalancer(ep.RecordType, target); ok { |
There was a problem hiding this comment.
nit: you could simplify this to just if isAWSLoadBalancer(ep.RecordType, target) {
| @@ -21,20 +21,58 @@ import ( | |||
| "github.com/kubernetes-incubator/external-dns/endpoint" | |||
| ) | |||
|
|
|||
| // RecordKey is used to group records | |||
| type RecordKey struct { | |||
There was a problem hiding this comment.
Since this is not used outside of plan, it should not be exported.
|
I'll try to merge this with master. |
|
@sethpollack would you rebase and fix the comments, please? |
|
Is there a timeline/commitment to get this merged in then? I know that this will be at least the 3rd time there has been a major rebase to get this merged in. |
|
@linki can you please comment. Kops would be a major step forward |
| @@ -43,7 +43,7 @@ func (ms *dedupSource) Endpoints() ([]*endpoint.Endpoint, error) { | |||
| } | |||
|
|
|||
| for _, ep := range endpoints { | |||
| identifier := ep.DNSName + " / " + ep.Target | |||
| identifier := ep.DNSName + " / " + ep.Targets[0] | |||
There was a problem hiding this comment.
why only first target is considered ?
There was a problem hiding this comment.
We only aggregate the targets in the plan, so at this point we would only have 1 target
| @@ -73,7 +73,7 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) { | |||
| endpoints = append(endpoints, record) | |||
| continue | |||
| } | |||
| ownerID := im.extractOwnerID(record.Target) | |||
| ownerID := im.extractOwnerID(record.Targets[0]) | |||
There was a problem hiding this comment.
keeping ownerID in the target is probably not the best idea as it seems now.
|
was this tested on Route53 ? If anyone has tried it on Route53 please let us know |
| @@ -271,12 +269,39 @@ func changesByZone(zones map[string]*route53.HostedZone, changeSet []*route53.Ch | |||
| return changes | |||
| } | |||
|
|
|||
| func expandAliasTargets(eps []*endpoint.Endpoint) (endpoints, aliasTargets []*endpoint.Endpoint) { | |||
There was a problem hiding this comment.
does it expand or it splits alias from rest ?
There was a problem hiding this comment.
yeah, i guess split makes more sense
|
@ideahitme I tested with Route53 a few months ago. |
|
@sethpollack, do you mind rebasing it? |
|
@tamalsaha not until we are actually ready to merge it. I already did a few rebases and its a ton of work. |
|
ah! I know exactly what you mean! |
|
@sethpollack We recently merged another branch adding multi-target support: #418. It's structurally possible to have a list of targets now, however full multi-target functionality only works on Google at the moment. I'll review your PR soon again to see if we missed anything that you solved here. I remember you already started work on a |
|
Multi-target functionality has successfully landed in another PR. Therefore I'm closing this. Feel free to re-open if you think it's wrong. Thanks @sethpollack for all your effort! @sethpollack I remember you already started work on a |
No description provided.