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

Disable service links to prevent environment variable conflict #1158

Closed
mikelorant opened this issue Aug 18, 2020 · 9 comments · Fixed by #1161
Closed

Disable service links to prevent environment variable conflict #1158

mikelorant opened this issue Aug 18, 2020 · 9 comments · Fixed by #1161
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest

Comments

@mikelorant
Copy link
Contributor

mikelorant commented Aug 18, 2020

It is common to name the Jaeger agent service jaeger-agent. This forces Kubernetes to create an environment variable JAEGER_AGENT_PORT as part of the service links. This conflicts with the an internal environmental variable used in the client library.

https://github.com/jaegertracing/jaeger-client-go/blob/704ce2849e9abb8af82edb2a3375e9e163092427/config/config_env.go#L51

This will cause the query UI to crash with the following error:

{"level":"fatal","ts":1597712251.5144613,"caller":"command-line-arguments/main.go:76","msg":"Failed to read tracer configuration","error":"cannot obtain reporter config from env: cannot parse env var JAEGER_AGENT_PORT=udp://100.69.94.248:5775: strconv.ParseInt: parsing \"udp://100.69.94.248:5775\": invalid syntax" ...

If this variable is set it is meant to be 6831 but Kubernetes sets it to the first port listed in the service for the agent. Jaeger query UI does not expect this value.

Service links can be disabled as part of the pod specification.

If the service environment variables are not desired (because possible clashing with expected program ones, too many variables to process, only using DNS, etc) you can disable this mode by setting the enableServiceLinks flag to false on the pod spec.

See also: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#pod-v1-core

By automatically setting enableServiceLinks to false this edge case will no longer occur.

This should be added to all deployment specifications created by the operator.

@ghost ghost added the needs-triage New issues, in need of classification label Aug 18, 2020
@mikelorant
Copy link
Contributor Author

This improvement would fix the following open issue:
jaegertracing/jaeger#1986

@jpkrohling
Copy link
Contributor

By automatically setting enableServiceLinks to false this edge case will no longer occur.

TIL! Thank you for this information. Would you be open to sending a PR with this change?

@jpkrohling jpkrohling added enhancement New feature or request good first issue Good for newcomers hacktoberfest and removed needs-triage New issues, in need of classification labels Aug 18, 2020
@mikelorant
Copy link
Contributor Author

I don't know much about writing Go but I decided to give it a shot. The unit tests are the main challenge for me so it is likely I have made mistakes there. Getting some assistance from my work to verify the code works correctly before I put up a proper pull request but you can see my changes:

master...fairfaxmedia:master

@jpkrohling
Copy link
Contributor

I left a couple of comments there. You are certainly on track!

@mikelorant
Copy link
Contributor Author

Have someone from work helping solving the issues now. Hopefully have this done soon :)

@mikelorant
Copy link
Contributor Author

Want to thank @dackroyd for sorting out all the mistakes I made and assisting with resolving the issues you mentioned.

@mikelorant
Copy link
Contributor Author

We have an image and will deploy it tomorrow. Are you able to confirm the spec changes required in the operator to set a custom query image?

@jpkrohling
Copy link
Contributor

Looks very good now! Once you confirm that it works as intended, do send a PR with that change. Not sure what you mean with your question though.

@mikelorant
Copy link
Contributor Author

Have just had confirmation that the unit tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants