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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugin/cache: update comment to conform to the implementation #3573

Merged
merged 1 commit into from Jan 3, 2020

Conversation

@xiez
Copy link
Contributor

xiez commented Dec 30, 2019

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

Update comment accordingly to the code fix in 159a8fb 馃槈

2. Which issues (if any) are related?

no

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

no

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

no

@xiez xiez requested review from grobie and miekg as code owners Dec 30, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 30, 2019

Codecov Report

Merging #3573 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3573   +/-   ##
=======================================
  Coverage   56.71%   56.71%           
=======================================
  Files         220      220           
  Lines       11000    11000           
=======================================
  Hits         6239     6239           
  Misses       4282     4282           
  Partials      479      479
Impacted Files Coverage 螖
plugin/cache/item.go 88.37% <酶> (酶) 猬嗭笍

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 908508a...1531f16. Read the comment docs.

@xiez xiez force-pushed the xiez:patch-1 branch from 38a3681 to 8844ea3 Dec 30, 2019
@@ -49,7 +49,7 @@ func newItem(m *dns.Msg, now time.Time, d time.Duration) *item {
}

// toMsg turns i into a message, it tailors the reply to m.
// The Authoritative bit is always set to 0, because the answer is from the cache.

This comment has been minimized.

Copy link
@miekg

miekg Dec 30, 2019

Member

good catch. Please make this comment longer.

The Authoritative bit should be set to 0, but some client stub resolver implementations, most notably, the one from Python will discard answers that do not have this bit set. So we're forced to always set this to 1; regardless if the answer came from the cache or not.

This comment has been minimized.

Copy link
@xiez

xiez Dec 30, 2019

Author Contributor

updated. 馃槃

Just a little FYI, Python use low-level glibc function getaddrinfo (https://github.com/python/cpython/blob/3.6/Modules/socketmodule.c#L6061) to perform address resolution. On some legacy systems(e.g. ubuntu 14.04 with glib version 2.20), this function will discard answers that do not have Authoritative bit set, which means not only Python, other languages like ruby/C also suffer the same issue; On newer systems(e.g. ubuntu 16.04 with glib version 2.23), this issue is resolved.

This comment has been minimized.

Copy link
@miekg

miekg Dec 30, 2019

Member

Can you add that too; I don't mind that we have a long comment here capturing all these bits.

This comment has been minimized.

Copy link
@xiez

xiez Dec 30, 2019

Author Contributor

ok, how about this:

// The Authoritative bit should be set to 0, but some client stub resolver implementations, most notably,
// on some legacy systems(e.g. ubuntu 14.04 with glib version 2.20), low-level glibc function `getaddrinfo`
// useb by Python/Ruby/etc.. will discard answers that do not have this bit set.
// So we're forced to always set this to 1; regardless if the answer came from the cache or not.
// On newer systems(e.g. ubuntu 16.04 with glib version 2.23), this issue is resolved.
// So we may set this bit back to 0 in the future ?
@xiez xiez force-pushed the xiez:patch-1 branch 2 times, most recently from c2c23f4 to 8ef711d Dec 30, 2019
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Dec 30, 2019

Signed-off-by: zheng xie <xiez1989@gmail.com>
@xiez xiez force-pushed the xiez:patch-1 branch from 8ef711d to 1531f16 Dec 31, 2019
@miekg miekg merged commit f81f28d into coredns:master Jan 3, 2020
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 56.71% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.