Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add NS record support #1813

Merged
merged 3 commits into from
Oct 21, 2020
Merged

Add NS record support #1813

merged 3 commits into from
Oct 21, 2020

Conversation

ytsarev
Copy link
Member

@ytsarev ytsarev commented Oct 12, 2020 •
edited

Description

Add NS record support.

End-to-end tested with crd source and aws provider.

Partially fixes #1647

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 12, 2020
@ytsarev
Copy link
Member Author

ytsarev commented Oct 12, 2020

/assign @Raffo

@ytsarev ytsarev mentioned this pull request Oct 15, 2020
Merged
@Raffo
Copy link
Contributor

Raffo commented Oct 15, 2020

End user documentation updated - can't find obvious place for it as the current documentation does not provide clear support list - please advise if needed

Can you add an entry a tutorial to use this feature under https://github.com/kubernetes-sigs/external-dns/tree/master/docs/tutorials ?

@ytsarev ytsarev mentioned this pull request Oct 15, 2020
Closed
@vsychov
Copy link
Contributor

vsychov commented Oct 15, 2020 •
edited

@ytsarev , thanks for PR, it resolve my issue #1817.
Hope it will be merged soon.

@ytsarev
Copy link
Member Author

ytsarev commented Oct 16, 2020

@Raffo added short doc, please check it out

Raffo
Raffo reviewed Oct 18, 2020
View reviewed changes
Copy link
Contributor

@Raffo Raffo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added two comments on test, change LGTM.

provider/rcode0/rcode0_test.go
require.Equal(t, 6, len(endpoints))
require.Equal(t, 10, len(endpoints))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also "NS" test fixture there

external-dns/provider/rcode0/rcode0_test.go

Line 327 in ba46a49

Type: "NS",

After adding NS record support the external-dns will create +4 records: 2 NS and 2 auxiliary TXT ones with the ownership metadata

provider/ovh/ovh_test.go
assert.ElementsMatch(records, []ovhRecord{{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}})
assert.ElementsMatch(records, []ovhRecord{{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, {ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to touch a provider specific test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to not to touch it but it started to fail with NS record support because NS records are started to get created :) If you look at

external-dns/provider/ovh/ovh_test.go

Line 91 in ba46a49

client.On("Get", "/domain/zone/example.org/record/24").Return(ovhRecord{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
, NS record is there in some form of test fixture

@ytsarev ytsarev requested a review from Raffo October 18, 2020 20:58
@Raffo
Copy link
Contributor

Raffo commented Oct 21, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, vsychov, ytsarev

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit 764b944 into kubernetes-sigs:master Oct 21, 2020
@AlbinoDrought AlbinoDrought mentioned this pull request Oct 21, 2020
Closed
@ytsarev ytsarev mentioned this pull request Dec 15, 2020
Closed
ytsarev added a commit to k8gb-io/k8gb that referenced this pull request Dec 21, 2020
@ytsarev
Switch external-dns to upstream v0.7.5 image release
66ab048 
* Follow dependabot's https://github.com/AbsaOSS/k8gb/pull/235
* Switch to new v0.7.5 version and upstream release as
  Absa's implementation of NS record support (kubernetes-sigs/external-dns#1813)
  is included into the release
@ytsarev ytsarev mentioned this pull request Dec 21, 2020
Merged
ytsarev added a commit to k8gb-io/k8gb that referenced this pull request Dec 21, 2020
@ytsarev
Switch external-dns to upstream v0.7.5 image release (#237)
ff17acd 
* Follow dependabot's https://github.com/AbsaOSS/k8gb/pull/235
* Switch to new v0.7.5 version and upstream release as
  Absa's implementation of NS record support (kubernetes-sigs/external-dns#1813)
  is included into the release
@backjo backjo mentioned this pull request Dec 21, 2020
Closed
@ytsarev ytsarev mentioned this pull request Jan 15, 2021
Closed
@haslersn
Copy link

haslersn commented Mar 1, 2021

How is the ownership of the NS record managed in the TXT registry? Is there a single TXT record that indicates ownership of both A and NS? What if external-dns should only own the A record for a domain (or the other way around)?

ytsarev added a commit to k8gb-io/k8gb that referenced this pull request Mar 20, 2021
@ytsarev
Rollback external-dns to get NS record creation back
b0a5b5c 
* external-dns `v0.7.6` included https://github.com/kubernetes-sigs/external-dns/pull/1915/files
* Even with `--managed-record-types=A,CNAME,NS` it silently skips NS
  record type creation
* `test.k8gb.io` NS record was staying orphaned and undeleted in reference setup
  configuration making an illusion that everything is properly working
  with route53 based zone delegation
* Switch back to reliable `v0.7.5` where NS record support was introduced
  at kubernetes-sigs/external-dns#1813

Signed-off-by: Yury Tsarev <yury.tsarev@absa.africa>
@ytsarev ytsarev mentioned this pull request Mar 20, 2021
Merged
ytsarev added a commit to k8gb-io/k8gb that referenced this pull request Mar 21, 2021
@ytsarev
Rollback external-dns to get NS record creation back (#392)
95cd31b 
* external-dns `v0.7.6` included https://github.com/kubernetes-sigs/external-dns/pull/1915/files
* Even with `--managed-record-types=A,CNAME,NS` it silently skips NS
  record type creation
* `test.k8gb.io` NS record was staying orphaned and undeleted in reference setup
  configuration making an illusion that everything is properly working
  with route53 based zone delegation
* Switch back to reliable `v0.7.5` where NS record support was introduced
  at kubernetes-sigs/external-dns#1813

Signed-off-by: Yury Tsarev <yury.tsarev@absa.africa>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsychov vsychov vsychov approved these changes

@njuettner njuettner Awaiting requested review from njuettner

@sheerun sheerun Awaiting requested review from sheerun

@Raffo Raffo Awaiting requested review from Raffo

Assignees

@Raffo Raffo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CRD unable to create DNSEndpoint of record types other than A or CNAME
5 participants
@ytsarev @Raffo @vsychov @k8s-ci-robot @haslersn