New plugin: loop #1989

miekg · 2018-07-20T13:44:11Z

Add a plugin that detects loops. It does this by sending an unique query
to our selves. If we see the query more than twice we stop the process.
If there isn't a loop, the plugin disables it self and becomes a noop
plugin.

Signed-off-by: Miek Gieben miek@miek.nl

corbot · 2018-07-20T13:44:15Z

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked greenpau (via /OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

miekg · 2018-07-20T13:44:54Z

Added @chrisohaver as reviewer as well, as I've volunteered him in the owners file

greenpau · 2018-07-20T13:45:42Z

@miekg , this one is pretty cool! 👍

chrisohaver

lgtm

greenpau

LGTM 👍

codecov-io · 2018-07-20T13:58:13Z

Codecov Report

Merging #1989 into master will decrease coverage by 0.12%.
The diff coverage is 36.9%.

@@            Coverage Diff             @@
##           master    #1989      +/-   ##
==========================================
- Coverage   54.79%   54.67%   -0.13%     
==========================================
  Files         198      200       +2     
  Lines        9639     9723      +84     
==========================================
+ Hits         5282     5316      +34     
- Misses       3948     3994      +46     
- Partials      409      413       +4

Impacted Files	Coverage Δ
plugin/pkg/log/plugin.go	`25% <0%> (-2.28%)`	⬇️
plugin/loop/loop.go	`25% <25%> (ø)`
plugin/loop/setup.go	`47.82% <47.82%> (ø)`
plugin/forward/connect.go	`86.48% <0%> (+4.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547f155...a5784d3. Read the comment docs.

chrisohaver · 2018-07-20T13:58:51Z

plugin/loop/loop.go

+
+	if l.seen() > 2 {
+		// TODO(miek): add log.Fatal(f) and use here.
+		log.Errorf("Seen \"HINFO IN %s\" more than twice, loop detected", l.qname)


Can we also log the IP address that the loop was detected in? i.e the ip that the request came in on...

Added, I'm my local testing this will always be localhost because there is no remote server at play.

chrisohaver · 2018-07-20T14:04:54Z

looks great. Thanks Miek for whipping this one out lightning fast!

Add a plugin that detects loops. It does this by sending an unique query to our selves. If we see the query more than twice we stop the process. If there isn't a loop, the plugin disables it self and becomes a noop plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

Signed-off-by: Miek Gieben <miek@miek.nl>

chrisohaver · 2018-07-20T14:24:39Z

Added, I'm my local testing this will always be localhost because there is no remote server at play.

Ah - I meant to log the IP address that coredns received the packet on (i.e. the destination ip of the incoming packet).
In the Ubuntu resolved case, this would be 127.0.0.53, which may make it easier to understand (since that IP is in resolv.conf).

Signed-off-by: Miek Gieben <miek@miek.nl>

fturib

I think there may be some issues if bloc has several keys, or if config has several hosts.
Please see my comments.

fturib · 2018-07-20T14:21:13Z

plugin/loop/loop.go

+	}
+
+	if l.seen() > 2 {
+		// TODO(miek): add log.Fatal(f) and use here.


I understand we create only one Loop struct. and we send several queries (every 2 sec).
so, this l.seen > 2 will be true at the third msg we send .. no ?

Should we have here a count for each msg.id we create for the test ?

fturib · 2018-07-20T14:23:40Z

plugin/loop/setup.go

+				}
+
+				if ok == len(conf.ListenHosts) {
+					go func() {


looks like a way to disable sooner if at least one msg sent per Listening host.
Because of the double loop (30 sec, and for each Host) - and because of 'continue" at line 46, we are not sure that all Host are tested. (some may be tested several time, some not at all).

fturib · 2018-07-20T14:24:13Z

plugin/loop/setup.go

+	c.OnStartup(func() error {
+		// Another Go function, otherwise we block startup and can't send the packet.
+		go func() {
+			deadline := time.Now().Add(30 * time.Second)


I guess 30 sec could be adapted to >= 2 * Nb of Listening Host.
I agree 15 seems already a big number ...

fturib · 2018-07-20T14:33:36Z

plugin/loop/setup.go

+
+	zone := "."
+	if len(c.ServerBlockKeys) > 0 {
+		zone = plugin.Host(c.ServerBlockKeys[0]).Normalize()


I think that setup is called for each of the serverBlocKeys.
There is a way to know which one we are called for : use c.Key

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg · 2018-07-20T14:53:46Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] New plugin: l..." ]

I understand we create only one Loop struct. and we send several queries (every 2 sec). so, this l.seen > 2 will be true at the third msg we send .. no ?

excellent point! I've only the first listenhost now. We can't use the query Id, because if something else forwards this again, it will be a different Id.

I guess 30 sec could be adapted to >= 2 * Nb of Listening Host. I agree 15 seems already a big number ...

I'm fine with 15, but I think load zones in the file plugin blocks startup, so that can be a long time...

I think that setup is called for each of the serverBlocKeys. There is a way to know which one we are called for : use c.Key

I'll add the standard code that this can only be used once. Thanks for reviewing! PTAL.

fturib · 2018-07-20T16:33:52Z

I'll add the standard code that this can only be used once.

It is not about having several time the "loop" plugin in the same stanza.
It is about having several keys on top of the stanza, like:

coredns.io:53, coredns.com:443, infobloxcom:667 {
    loop
    procxy ...
}

the setup of loop will be triggered 3 times : one for each key on the top.
and from the setup(...) function you get that key with : c.Key

miekg · 2018-07-20T17:21:05Z

This should result in three different queries because I suffix the zone for each generated name. Let me see if I can add a test for that.

…

On Fri, 20 Jul 2018, 17:33 Francois Tur, ***@***.***> wrote: I'll add the standard code that this can only be used once. It is not about having several time the "loop" plugin in the same stanza. It is about having several keys on top of the stanza, like: coredns.io:53, coredns.com:443, infobloxcom:667 { loop procxy ... } the setup of loop will be triggered 3 times : one for each key on the top. and from the setup(...) function you get that key with : c.Key — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1989 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkW_g52zA98gBPkChdsdTD3ynxzt6cks5uIgZygaJpZM4VYDrd> .

miekg · 2018-07-20T17:58:50Z

coredns.io:1053 coredns.com:1054 infoblox.com:1055 {
    loop
    forward . 127.0.0.1:1054
    log
}

crashes with:

127.0.0.1:47153 - [20/Jul/2018:18:57:23 +0100] 738 "HINFO IN 1687376426597664323.1119963552489870178.coredns.io. udp 68 false 512" REFUSED qr,rd 68 0.000187187s
127.0.0.1:34498 - [20/Jul/2018:18:57:23 +0100] 45646 "HINFO IN 1687376426597664323.1119963552489870178.coredns.io. udp 68 false 512" REFUSED qr,rd 68 0.000087221s
2018/07/20 18:57:23 [FATAL] plugin/loop: Seen "HINFO IN 1687376426597664323.1119963552489870178.coredns.io." to [::]:1053 more than twice, loop detected

Signed-off-by: Miek Gieben <miek@miek.nl>

luxas · 2018-07-23T08:54:37Z

@rajansandeep @miekg @johnbelamaric Will this option make it into the next k8s manifest that has a new enough CoreDNS version?

miekg · 2018-07-23T12:09:04Z

This is as yet unreleased, but CoreDNS 1.2.1 will have it. What's the timeframe?

…

On Mon, 23 Jul 2018, 09:54 Lucas Käldström, ***@***.***> wrote: @rajansandeep <https://github.com/rajansandeep> @miekg <https://github.com/miekg> @johnbelamaric <https://github.com/johnbelamaric> Will this option make it into the next k8s manifest that has a new enough CoreDNS version? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1989 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkWxpvkmKKNGtam5mYK82i4PGZZ8P1ks5uJY9PgaJpZM4VYDrd> .

chrisohaver · 2018-07-23T12:52:35Z

Will this option make it into the next k8s manifest that has a new enough CoreDNS version?

Yes

chrisohaver · 2018-07-23T12:53:11Z

What's the timeframe?

Plenty of time to get it into the next k8s release.

fturib · 2018-07-23T15:08:59Z

In that case it would not be considered as a feature.
rather update of existing feature. Code slush is Aug 28th.
We would need to have CoreDNS 1.2.1 released by Aug20th.

chrisohaver · 2018-07-23T15:11:28Z

Yes, thanks. I relayed the wrong freeze ... FYI...

https://github.com/kubernetes/sig-release/blob/master/releases/release-1.12/release-1.12.md

* New plugin: loop Add a plugin that detects loops. It does this by sending an unique query to our selves. If we see the query more than twice we stop the process. If there isn't a loop, the plugin disables it self and becomes a noop plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

corbot bot requested a review from greenpau July 20, 2018 13:44

miekg requested a review from chrisohaver July 20, 2018 13:44

chrisohaver approved these changes Jul 20, 2018

View reviewed changes

greenpau approved these changes Jul 20, 2018

View reviewed changes

chrisohaver reviewed Jul 20, 2018

View reviewed changes

miekg mentioned this pull request Jul 20, 2018

CoreDNS pod dies when trying to resolve? #1986

Closed

miekg added 5 commits July 20, 2018 14:08

New plugin: loop

a1ba05e

Add a plugin that detects loops. It does this by sending an unique query to our selves. If we see the query more than twice we stop the process. If there isn't a loop, the plugin disables it self and becomes a noop plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

add owners

242eca6

Signed-off-by: Miek Gieben <miek@miek.nl>

mimic standard logging better

2012197

Signed-off-by: Miek Gieben <miek@miek.nl>

Log remoteAddr as well

cb42729

Signed-off-by: Miek Gieben <miek@miek.nl>

golint

124b992

Signed-off-by: Miek Gieben <miek@miek.nl>

Fix log.Fatal(f)

73239ef

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg force-pushed the loop branch from b733d80 to 73239ef Compare July 20, 2018 14:29

tweaks

a5124de

Signed-off-by: Miek Gieben <miek@miek.nl>

fturib reviewed Jul 20, 2018

View reviewed changes

PR review

09b1c16

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg added 2 commits July 20, 2018 19:13

Tweaks

5ae8910

Signed-off-by: Miek Gieben <miek@miek.nl>

fix readme

a5784d3

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg merged commit 84ec780 into master Jul 20, 2018

corbot bot deleted the loop branch July 20, 2018 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New plugin: loop #1989

New plugin: loop #1989

miekg commented Jul 20, 2018

corbot bot commented Jul 20, 2018

miekg commented Jul 20, 2018

greenpau commented Jul 20, 2018

chrisohaver left a comment

greenpau left a comment

codecov-io commented Jul 20, 2018 •

edited

Loading

chrisohaver Jul 20, 2018 •

edited

Loading

miekg Jul 20, 2018

chrisohaver commented Jul 20, 2018

chrisohaver commented Jul 20, 2018

fturib left a comment

fturib Jul 20, 2018

fturib Jul 20, 2018

fturib Jul 20, 2018

fturib Jul 20, 2018

miekg commented Jul 20, 2018 via email

fturib commented Jul 20, 2018

miekg commented Jul 20, 2018 via email

miekg commented Jul 20, 2018

luxas commented Jul 23, 2018

miekg commented Jul 23, 2018 via email

chrisohaver commented Jul 23, 2018

chrisohaver commented Jul 23, 2018

fturib commented Jul 23, 2018

chrisohaver commented Jul 23, 2018 •

edited

Loading

New plugin: loop #1989

New plugin: loop #1989

Conversation

miekg commented Jul 20, 2018

corbot bot commented Jul 20, 2018

miekg commented Jul 20, 2018

greenpau commented Jul 20, 2018

chrisohaver left a comment

Choose a reason for hiding this comment

greenpau left a comment

Choose a reason for hiding this comment

codecov-io commented Jul 20, 2018 • edited Loading

Codecov Report

chrisohaver Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

miekg Jul 20, 2018

Choose a reason for hiding this comment

chrisohaver commented Jul 20, 2018

chrisohaver commented Jul 20, 2018

fturib left a comment

Choose a reason for hiding this comment

fturib Jul 20, 2018

Choose a reason for hiding this comment

fturib Jul 20, 2018

Choose a reason for hiding this comment

fturib Jul 20, 2018

Choose a reason for hiding this comment

fturib Jul 20, 2018

Choose a reason for hiding this comment

miekg commented Jul 20, 2018 via email

fturib commented Jul 20, 2018

miekg commented Jul 20, 2018 via email

miekg commented Jul 20, 2018

luxas commented Jul 23, 2018

miekg commented Jul 23, 2018 via email

chrisohaver commented Jul 23, 2018

chrisohaver commented Jul 23, 2018

fturib commented Jul 23, 2018

chrisohaver commented Jul 23, 2018 • edited Loading

codecov-io commented Jul 20, 2018 •

edited

Loading

chrisohaver Jul 20, 2018 •

edited

Loading

chrisohaver commented Jul 23, 2018 •

edited

Loading