Do not throw spurious error when minTTL=0 for the domain in egress network policy #19950
Conversation
openshift-ci-robot
added
size/XS
|
@openshift/sig-networking @knobunc PTAL |
|
@knobunc We are already adding "." to the end of the domain name before querying the dns server. |
pkg/network/common/dns.go
Outdated
| // We could potentially get zero TTL as we try to query the DNS | ||
| // server at the end of the TTL value for the domain. | ||
| // Adding 1 sec will ensure non zero minTTL | ||
| ttl := t.Hdr.Ttl + 1 |
There was a problem hiding this comment.
Can we make minTTL an int32 (what does dns spec the ttl as?) Then we can set the minTTL to -1 when we initialize it.
Then we can just compare the ttl against the minTTL and not have to do the 1s offset.
There was a problem hiding this comment.
github.com/miekg/dns has t.Hdr.Ttl as uint32 which is as per the DNS spec. So converting to int32 will be an issue (not sure whether so high TTL values are set in real world or not)
pkg/network/common/dns.go
Outdated
| // Adding 1 sec will ensure non zero minTTL | ||
| ttl := t.Hdr.Ttl + 1 | ||
|
|
||
| if minTTL == 0 || ttl < minTTL { |
There was a problem hiding this comment.
Can't this just be:
if ttl < minTTL {
Why do we care if minTTL == 0
There was a problem hiding this comment.
We were overloading this field to know whether this field is set or not otherwise we don't care about the value of minTTL. Let me fix this one.
|
I never see a TTL of 0 returned from dig... if I query repeatedly, it always rolls around from 1 second back to the max TTL without the returned TTL ever being 0. So I wonder if the problem isn't something else. Maybe add some more debugging/error checking? |
|
On Fri, Jun 8, 2018 at 11:27 AM, Dan Winship ***@***.***> wrote:
I never see a TTL of 0 returned from dig... if I query repeatedly, it
always rolls around from 1 second back to the max TTL without the returned
TTL ever being 0.
So I wonder if the problem isn't something else. Maybe add some more
debugging/error checking?
This is not a hunch. I have added more debugging and was able to reproduce
the issue:
It has printed the error message along with dns nameservers and TTL value
returned from the library which was zero.
And also I was able to see TTL of 0 returned from dig:
[root@ip-172-18-9-78 ~]# oc exec sdn-285qm -n openshift-sdn -- dig
www.amazon.com
...
d3ag4hukkh62yn.cloudfront.net. 0 IN A 52.85.131.125
...
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19950 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABM0hpNHIUt7VCi8AC1Zw9068d-5Pt7kks5t6sItgaJpZM4UgsKD>
.
|
- This will ensure dns failure of one domain will not block periodic updates for other domains in the egress network policy.
pravisankar
force-pushed
the
fix-ttl-handling
branch
from
f6cf2ff to
5fa1323
Compare
openshift-ci-robot
added
size/S
pravisankar
changed the title
|
@knobunc @danwinship updated, PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knobunc, pravisankar 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 |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1575583#c23