-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add HOST_IP environment variable to consul_dataplane_sidecar.go #3916
Conversation
This environment variable must be set in order to utilize HOST_IP as part of statsd or dogstatsd sinks. If this variable is unset and the sink is configured to use HOST_IP, the sidecar won't start and pods won't coe up healthy.
@t-eckert @david-yu You guys reviewed a similar change previously about a year ago: #1808 (comment) We're just now circling back to try this upgrade and ran into this issue where the code supports the var expansion, but it seems like the env var isn't being set in the injected dataplane pods. This PR fixes that. Note that I'm not sure why the pr/test and backport checks are failing, but I'm happy to make any changes given proper guidance. We've been waiting a year to get this going, so would love to do anything we can to speed this fix along and finally get onto 1.x. ❤️ |
@woz5999, this is excellent work! Overall, the PR seems right. I would definitely encourage you to run this build in a K8s deployment in order to verify it works correctly with *statsd. I no longer work at HashiCorp so I don't think it's right for me to approve the change, but from a spot check, this is how adding the env var would be done. |
I just wanted to update that I was able to successfully deploy and confirm this fix in our cluster today. |
Any update on this? |
@nathancoleman, I checked this awhile back. I think it's good. |
{ | ||
Name: "HOST_IP", | ||
ValueFrom: &corev1.EnvVarSource{ | ||
FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.hostIP"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this matches the documentation for the Downward API
Looks like this needs a rebase and some backport labels. I can handle that by EOD today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Nathan Coleman <[email protected]>
Thanks @nathancoleman This is updated now |
@woz5999 I reopened this PR using a branch in |
Changes proposed in this PR
HOST_IP
environment variable to consul_dataplane_sidecar.goThis environment variable must be set in order to utilize
HOST_IP
as part of statsd or dogstatsd sinks. If this variable is unset and the sink is configured to useHOST_IP
, the sidecar won't start and pods won't come up healthy.How I've tested this PR
HOST_IP
variable set as expectedHow I expect reviewers to test this PR
The unit tests should do it
Reference to similar change for mesh-gateway deployments:
consul-k8s/CHANGELOG.md
Line 763 in 8ac97bf
Checklist