Update AWS private hosted zones in addition to public zone #356
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
LGTM this needs to be rebased on master. Regarding the specific zone matching for AWS (as opposed to common zone filtering for other providers) seems fine to me, we can maybe come up with a common algorithm in the future. |
coreypobrien
force-pushed
the
multizone
branch
from
a062100
to
78feefe
Compare
|
👍 Rebased on master |
|
👍 from my side |
|
@geojaz: changing LGTM is restricted to assignees, and only kubernetes-incubator org members may be assigned issues. 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. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| @@ -267,12 +267,15 @@ func changesByZone(zones map[string]*route53.HostedZone, changeSet []*route53.Ch | |||
| for _, c := range changeSet { | |||
| hostname := ensureTrailingDot(aws.StringValue(c.ResourceRecordSet.Name)) | |||
|
|
|||
| zoneID, _ := zoneNameIDMapper.FindZone(hostname) | |||
There was a problem hiding this comment.
zoneNameIDMapper seems unsed to me now. Can we remove it, @coreypobrien ?
There was a problem hiding this comment.
Good call. Removed.
linki
removed
the
lgtm
|
👍 @ideahitme @linki please merge, the last open comment was already fixed |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
please one last thing, could you add the change to the changelog ? |
ideahitme
added
needs-clarification
and removed
lgtm
|
@here would like to discuss first whether merging this will cause any issues in the future, if at some point we would like to support "stickiness" to certain hosted zones (instead of migrating record in case of dynamically adding new zones). We could possibly want to make zone part as part of the TXT record, and how this PR would affect this decision |
coreypobrien
force-pushed
the
multizone
branch
from
acf3f9d
to
6a0666a
Compare
|
@ideahitme Added the changelog Targeting a specific hosted zone or a set of hosted zones when multiple match will be solved by #322. Right now, the records will be added to all private zones. |
coreypobrien
force-pushed
the
multizone
branch
from
6a0666a
to
059187c
Compare
|
@ideahitme Where do you think we are on this? Do you think we could merge this now and handle any future concerns when those features are addressed? |
|
@coreypobrien sorry for the delay, I think we should consider merging this PR this week, @linki did you already have a look ? |
|
@linki any update? |
|
I'll try to get it in this week. |
k8s-ci-robot
added
size/L
|
Fixed a couple of minor things with the changelog. @ideahitme @mikkeloscar @coreypobrien Do you think this change requires a minor version bump? |
|
I guess it makes sense to bump the minor version since the behavior might be unexpected for people with both public and private zones. |
|
@mikkeloscar thanks for the feedback. |
|
@coreypobrien Thanks for the PR! |
…s-sigs#356) * Update AWS private hosted zones in addition to public zone * Sort slices for consistent ordering in TestAWSSuitableZones * ref: use len to check for empty list of matched zones * feat: mention contributors in changelog * fix: move changelog entry to the unreleased section * fix: add one more missing attribution to the changelog
Fixes #335
Currently, either a public or a private hosted zone on AWS will be updated, but never two identically named hosted zones. This implementation updates any matching private hosted zones as well as the one best matching public zone.
I implemented as updating all private zones because of the way AWS private zones work with VPCs. When a private hosted zone is attached to a VPC, the private hosted zone is the only source of truth for all DNS records in that domain. There is no possibility of delegation or forwarding. Therefore although it is obvious that a record for
a.sub.example.comshould be placed in the publicsub.example.comhosted zone, that record could legitimately be required in private zonessub.example.comandexample.com.When #322 is solved, it will allow users to choose specific private hosted zones to allow for more specificity and less sprawl.