Deprecate DNS Poller in v1.8 by soumynathan · Pull Request #10629 · cilium/cilium

soumynathan · 2020-03-19T04:39:27Z

This patch deprecates the use of --tofqdns-enable-poller and
--tofqdns-enable-poller-events option in v1.8.

Fixes: #8604
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

maintainer-s-little-helper · 2020-03-19T04:39:29Z

Please set the appropriate release note label.

coveralls · 2020-03-19T05:03:34Z

Coverage decreased (-0.006%) to 44.667% when pulling 50ea33b on soumynathan:deprecate-dns-poller into 6bcbf2d on cilium:master.

joestringer

Thanks for taking this on! I see that there's still some other policy page documentation which talks about this feature, could you please remove that documentation in this PR too? That way we won't be telling users how to configure an option that's already deprecated.

Also, how do you want to track the final removal in v1.9? If we mark the commit and PR here to "Fix" #8604 then that issue will be closed. Should we keep it open until we fully remove this functionality?

I'll also rope in @raybejjani in case he has any thoughts here.

vadorovsky

Apart from documentation, also this description needs to be changed:

https://github.com/soumynathan/cilium/blob/566558097bda1bd9615bd1950a00513dc09ecb1c/daemon/cmd/daemon_main.go#L683-L684

Regarding the documentation, it probably needs a change here:

https://github.com/soumynathan/cilium/blame/566558097bda1bd9615bd1950a00513dc09ecb1c/Documentation/policy/language.rst#L1041-L1042

soumynathan · 2020-03-19T16:30:04Z

Apart from documentation, also this description needs to be changed:

https://github.com/soumynathan/cilium/blob/566558097bda1bd9615bd1950a00513dc09ecb1c/daemon/cmd/daemon_main.go#L683-L684

Regarding the documentation, it probably needs a change here:

https://github.com/soumynathan/cilium/blame/566558097bda1bd9615bd1950a00513dc09ecb1c/Documentation/policy/language.rst#L1041-L1042

will address your comments

soumynathan · 2020-03-19T16:30:39Z

Thanks for taking this on! I see that there's still some other policy page documentation which talks about this feature, could you please remove that documentation in this PR too? That way we won't be telling users how to configure an option that's already deprecated.

Also, how do you want to track the final removal in v1.9? If we mark the commit and PR here to "Fix" #8604 then that issue will be closed. Should we keep it open until we fully remove this functionality?

I'll also rope in @raybejjani in case he has any thoughts here.

Sure will remove the fix tag and we can keep this bug open in that case.

joestringer · 2020-03-19T16:32:52Z

Sure will remove the fix tag and we can keep this bug open in that case.

@soumynathan FYI you can use "Related: ..." tag to link the issue without autoclosing it.

soumynathan · 2020-03-19T17:17:21Z

Sure will remove the fix tag and we can keep this bug open in that case.

@soumynathan FYI you can use "Related: ..." tag to link the issue without autoclosing it.

Thanks will do.

vadorovsky

Only one nit. Otherwise looks good, thanks for addressing my previous comments!

vadorovsky

Now looks awesome, thanks!

vadorovsky · 2020-03-19T20:11:51Z

test-me-please

vadorovsky · 2020-03-19T20:11:58Z

test-docs-please

aanm · 2020-03-20T12:03:13Z

test-me-please

aanm

@raybejjani PTAL

raybejjani · 2020-04-01T09:22:15Z

I'm ok removing this but since one can still choose to enable the poller this logic still applies (at https://github.com/cilium/cilium/blob/master/pkg/option/config.go#L2149-L2156). It might be best to edit this comment once that logic is removed.

aanm · 2020-04-15T06:07:31Z

ping @soumynathan

soumynathan · 2020-04-15T06:11:16Z

ping @soumynathan

Hi @aanm

joestringer · 2020-04-20T16:27:46Z

test-me-please

joestringer · 2020-04-21T00:49:00Z

Cilium-Ginkgo-Tests hit this:

10:18:28      runtime: --- a/Documentation/cmdref/cilium-agent.md
10:18:28      runtime: +++ b/Documentation/cmdref/cilium-agent.md
10:18:28      runtime: @@ -167,7 +167,7 @@ cilium-agent [flags]
10:18:28      runtime:        --tofqdns-enable-dns-compression                Allow the DNS proxy to compress responses to endpoints that are larger than 512 Bytes or the EDNS0 option, if present (default true)
10:18:28      runtime:        --tofqdns-endpoint-max-ip-per-hostname int      Maximum number of IPs to maintain per FQDN name for each endpoint (default 50)
10:18:28      runtime:        --tofqdns-max-deferred-connection-deletes int   Maximum number of IPs to retain for expired DNS lookups with still-active connections (default 10000)
10:18:28      runtime: -      --tofqdns-min-ttl int                           The minimum time, in seconds, to use DNS data for toFQDNs policies (default 3600 )
10:18:28      runtime: +      --tofqdns-min-ttl int                           The minimum time, in seconds, to use DNS data for toFQDNs policies. (default 3600 )
10:18:28      runtime:        --tofqdns-pre-cache string                      DNS cache data at this path is preloaded on agent startup
10:18:28      runtime:        --tofqdns-proxy-port int                        Global port on which the in-agent DNS proxy should listen. Default 0 is a OS-assigned port.
10:18:28      runtime:        --tofqdns-proxy-response-max-delay duration     The maximum time the DNS proxy holds an allowed DNS response before sending it along. Responses are sent as soon as the datapath is updated with the new IP information. (default 100ms)
10:18:31      runtime: HINT: to fix this, run 'make -C Documentation update-cmdref ; git commit Documentation/cmdref --message "docs: Update cmdref"'
10:18:32      runtime: Makefile:30: recipe for target 'check' failed
10:18:32      runtime: make[1]: Leaving directory '/home/vagrant/go/src/github.com/cilium/cilium/Documentation'
10:18:32      runtime: make[1]: *** [check] Error 1
10:18:32      runtime: Makefile:478: recipe for target 'postcheck' failed
10:18:32      runtime: make: *** [postcheck] Error 2

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18980/execution/node/122/log/

(/cc @errordeveloper why wasn't this picked up by the GH helper? Seems like it should have been run 6 days ago if that makes any difference)

This patch deprecates the use of --tofqdns-enable-poller and --tofqdns-enable-poller-events option in v1.8. Related: cilium#8604 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>

pchaigno · 2020-04-21T08:45:11Z

test-me-please

errordeveloper · 2020-04-21T17:26:26Z

@joestringer the GH Actions job doesn't build the binaries, it operates only on Documentation/Makefile right now.

The DNS poller was deprecated in the 1.8 release and all documentation on how to configure it should have been removed as per [1] [1] #10629 (review) However, the respective helm option (which disables the DNS poller by default) wasn't removed. Do so now. Related: #8604 Fixes: b8971ff ("Deprecate DNS Poller in v1.8") Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

[ upstream commit 3591629 ] The DNS poller was deprecated in the 1.8 release and all documentation on how to configure it should have been removed as per [1] [1] #10629 (review) However, the respective helm option (which disables the DNS poller by default) wasn't removed. Do so now. Related: #8604 Fixes: b8971ff ("Deprecate DNS Poller in v1.8") Signed-off-by: Tobias Klauser <tklauser@distanz.ch> Signed-off-by: Paul Chaignon <paul@cilium.io>

The DNS Poller was deprecated in v1.8 in #10629 and announced to be removed in v1.9. Remove it now. Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

soumynathan requested a review from a team March 19, 2020 04:39

soumynathan requested a review from a team as a code owner March 19, 2020 04:39

maintainer-s-little-helper Bot added the dont-merge/needs-release-note label Mar 19, 2020

soumynathan force-pushed the deprecate-dns-poller branch from ff98005 to 5665580 Compare March 19, 2020 05:16

joestringer requested changes Mar 19, 2020

View reviewed changes

joestringer requested a review from raybejjani March 19, 2020 06:54

joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 19, 2020

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note label Mar 19, 2020

vadorovsky suggested changes Mar 19, 2020

View reviewed changes

soumynathan force-pushed the deprecate-dns-poller branch 2 times, most recently from efa1f51 to d7a2723 Compare March 19, 2020 18:05

vadorovsky suggested changes Mar 19, 2020

View reviewed changes

Comment thread Documentation/policy/language.rst Outdated

soumynathan force-pushed the deprecate-dns-poller branch from d7a2723 to 09efe92 Compare March 19, 2020 20:10

vadorovsky approved these changes Mar 19, 2020

View reviewed changes

aanm added the pending-review label Mar 20, 2020

aanm approved these changes Mar 20, 2020

View reviewed changes

raybejjani approved these changes Apr 1, 2020

View reviewed changes

joestringer requested changes Apr 15, 2020

View reviewed changes

Comment thread Documentation/policy/language.rst Outdated

soumynathan force-pushed the deprecate-dns-poller branch from 09efe92 to ff8435e Compare April 15, 2020 06:29

soumynathan force-pushed the deprecate-dns-poller branch from ff8435e to 6b19215 Compare April 15, 2020 06:35

joestringer reviewed Apr 15, 2020

View reviewed changes

Comment thread Documentation/policy/language.rst Outdated

soumynathan force-pushed the deprecate-dns-poller branch from 6b19215 to a8de096 Compare April 15, 2020 07:39

joestringer approved these changes Apr 21, 2020

View reviewed changes

Deprecate DNS Poller in v1.8

50ea33b

This patch deprecates the use of --tofqdns-enable-poller and --tofqdns-enable-poller-events option in v1.8. Related: cilium#8604 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>

soumynathan force-pushed the deprecate-dns-poller branch from a8de096 to 50ea33b Compare April 21, 2020 03:57

pchaigno merged commit b8971ff into cilium:master Apr 21, 2020

tklauser mentioned this pull request Jul 22, 2020

Deprecate DNS Poller in v1.8, remove in v1.9 #8604

Closed

4 tasks

tklauser mentioned this pull request Jul 22, 2020

install/kubernetes: remove deprecated tofqdns-enable-poller option #12618

Merged

tklauser added a commit that referenced this pull request Sep 21, 2020

daemon, fqdn: remove DNS poller implementation

a7d8891

The DNS Poller was deprecated in v1.8 in #10629 and announced to be removed in v1.9. Remove it now. Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

tklauser mentioned this pull request Sep 21, 2020

Remove DNS poller #13229

Merged

qmonnet pushed a commit that referenced this pull request Sep 22, 2020

daemon, fqdn: remove DNS poller implementation

ec972c7

The DNS Poller was deprecated in v1.8 in #10629 and announced to be removed in v1.9. Remove it now. Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

Conversation

soumynathan commented Mar 19, 2020

Uh oh!

maintainer-s-little-helper Bot commented Mar 19, 2020

Uh oh!

coveralls commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

soumynathan commented Mar 19, 2020

Uh oh!

soumynathan commented Mar 19, 2020

Uh oh!

joestringer commented Mar 19, 2020

Uh oh!

soumynathan commented Mar 19, 2020

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

vadorovsky commented Mar 19, 2020

Uh oh!

vadorovsky commented Mar 19, 2020

Uh oh!

aanm commented Mar 20, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raybejjani Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

aanm commented Apr 15, 2020

Uh oh!

Uh oh!

soumynathan commented Apr 15, 2020

Uh oh!

Uh oh!

joestringer commented Apr 20, 2020

Uh oh!

joestringer commented Apr 21, 2020

Uh oh!

pchaigno commented Apr 21, 2020

Uh oh!

errordeveloper commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

coveralls commented Mar 19, 2020 •

edited

Loading