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.

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

plugin/template: Add client IP data #4034

Merged
merged 1 commit into from Aug 10, 2020
Merged

Conversation

Miouge1
Copy link
Contributor

@Miouge1 Miouge1 commented Jul 30, 2020

1. Why is this pull request needed and what does it do?

This PR adds a {{ .Remote }} data in the template module.

I have tested it with the following minimal Corefile (kind of emulating the whoami plugin):

. {
    template IN A {
      answer "{{ .Name }} 60 {{ .Class }} {{ .Type }} {{ .Remote }}"
    }
}
$ dig @127.0.0.1 A example.com +short
127.0.0.1

In terms of use case, the client IP can be useful to make decisions.

2. Which issues (if any) are related?

None

3. Which documentation changes (if any) need to be made?

plugin/template/README.md included in the PR

4. Does this introduce a backward incompatible change or deprecation?

Adding a field should be backwards compatible.

@Miouge1 Miouge1 requested a review from rtreffer as a code owner July 30, 2020 08:41
@Miouge1 Miouge1 force-pushed the template-ip branch 2 times, most recently from 953d48f to 75a7301 Compare July 30, 2020 08:51
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2020

Codecov Report

Merging #4034 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4034      +/-   ##
==========================================
+ Coverage   57.06%   57.08%   +0.01%     
==========================================
  Files         222      222              
  Lines       11281    11281              
==========================================
+ Hits         6438     6440       +2     
+ Misses       4351     4350       -1     
+ Partials      492      491       -1     
Impacted Files Coverage Δ
plugin/template/template.go 88.42% <100.00%> (ø)
plugin/file/reload.go 75.00% <0.00%> (+5.55%) ⬆️

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 ec906fe...865776b. Read the comment docs.

Copy link
Collaborator

@rtreffer rtreffer left a comment

Choose a reason for hiding this comment

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

Looks cool, although it would be nice to say it might be the IP of an intermediate (at which point it becomes pretty pointless for the original client).

@miekg
Copy link
Member

miekg commented Jul 31, 2020

In the log plugin we call this (for better or worse) remote. i think we should have this consistent between plugin, by either renaming it here; or adding {ip} to log. Think the more generic name 'remote' is slightly better

Signed-off-by: Maxime Guyot <maxime@root314.com>
@Miouge1
Copy link
Contributor Author

Miouge1 commented Jul 31, 2020

Good points. I've replaced .IP to .Remote in the latest commit and re-tested this locally:

. {
    log
    template IN ANY {
      answer "{{ .Name }} 60 {{ .Class }} {{ .Type }} {{ .Remote }}"
    }
}
$ dig @127.0.0.1 TXT example.com +short
"127.0.0.1"
$ dig @::1 TXT example.com +short
"::1"

The log module documents {{remote}} as client's IP address, for IPv6 addresses these are enclosed in brackets: [::1], since the brackets enclosing doesn't happen here I've skipped that from the description in this PR.

@miekg
Copy link
Member

miekg commented Jul 31, 2020 via email

@Miouge1
Copy link
Contributor Author

Miouge1 commented Aug 4, 2020

{{ .Port }} does not exist, but would not be too hard to add.

Are brackets enclosed IPv6 IPs supported in AAAA records? (I couldn't find an example of that in the codebase).

@miekg
Copy link
Member

miekg commented Aug 4, 2020 via email

@miekg miekg merged commit b3d08f9 into coredns:master Aug 10, 2020
nyodas pushed a commit to DataDog/coredns that referenced this pull request Oct 26, 2020
Signed-off-by: Maxime Guyot <maxime@root314.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants