resolving host before enabling tracing #6872
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @timmysilv. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: timmysilv The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
| @@ -1165,7 +1165,7 @@ func TestBuildOpenTracing(t *testing.T) { | |||
|
|
|||
| cfgJaeger := config.Configuration{ | |||
| EnableOpentracing: true, | |||
| JaegerCollectorHost: "jaeger-host.com", | |||
| JaegerCollectorHost: "localhost", | |||
There was a problem hiding this comment.
What do you think to use: jaeger.lvh.me or similar? It would test resolving not just by /etc/hosts, but also use DNS resolving:
$ nslookup jaeger.lvh.me - 1.1.1.1
Server: 1.1.1.1
Address: 1.1.1.1#53
Non-authoritative answer:
Name: jaeger.lvh.me
Address: 127.0.0.1I have not tested locally, if there are any restrictions in the test environment.
There was a problem hiding this comment.
I made this change, should pass tests fine. I don't think it's necessarily important to test that resolvers themselves work here, but why not match production as much as possible?
There was a problem hiding this comment.
Thank you for the changes.
why not match production as much as possible?
I think a general deployment of ingress and collector will be deployed to different node pools.
Discovery would be through the kubernetes services, that would use resolving through nameservers.
timmysilv
force-pushed
the
resolve-tracing-hosts
branch
from
7d8b085
to
6a479de
Compare
k8s-ci-robot
added
the
needs-rebase
timmysilv
force-pushed
the
resolve-tracing-hosts
branch
from
6a479de
to
e3ea6e7
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
using lvh.me to actually test with DNS
timmysilv
force-pushed
the
resolve-tracing-hosts
branch
from
e3ea6e7
to
2c6c221
Compare
| return "" | ||
| } | ||
| buf.WriteString("opentracing_load_tracer /usr/local/lib/libjaegertracing_plugin.so /etc/nginx/opentracing.json;") | ||
| host = strings.Split(hostMatch[4], ":")[0] |
There was a problem hiding this comment.
the above logic is taken from the client code. I'm keeping the regex confined to cfg.JaegerEndpoint because that regex expects something more specific. For example, just saying mydomain.com (for Zipkin, let's say) would actually result in regex match group 5 (not 4) but should still be valid. Meeting Jaeger endpoint standards simplifies the logic required in ingress-nginx, but I still do the split on :// afterward in case another configuration included a protocol.
|
After thinking on it, changing nginx-opentracing to not fail hard when the client-specific tracer doesn't load properly would be more elegant so I'll look into that |
What this PR does / why we need it:
While testing a new Jaeger C++ client, @miry found that configuring tracing with a domain that doesn't resolve causes ingress-nginx to fail bootup. This tries to resolve the domain in Go before letting the tracing client libraries do it and cause fatal errors. If it can't resolve, it simply won't enable tracing.
Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
Updated the old test cases to use
localhostsince that should always resolve (and not hit this error), and added a new test case to ensure that an invalid domain causes an error and doesn't write to the template file.Checklist: