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

Support passing extra arguments to injected envoy binary #378

Merged
merged 12 commits into from
Nov 6, 2020

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Nov 3, 2020

Changes proposed in this PR:

You can set it via either one, but if you set the args both ways, the pod annotation arguments will be used.

How I've tested this PR:

  • Built a docker image (gcr.io/nitya-293720/consul-k8s-dev:extraEnvoyArgs) and ran it on a cluster, while passing --log-level debug as an extra envoy flag, and saw that when we deployed a connect-inject enabled service, the envoy logs were showing debug logs.
  • Set the pod annotation to be a different set of arguments, and saw that the arguments passed to the envoy binary were overwritten.

How I expect reviewers to test this PR:

  • Code review

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@ndhanushkodi ndhanushkodi requested review from ishustava, a team and thisisnotashwin and removed request for a team November 3, 2020 00:45
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Have a few minor questions but love this overall!!

connect-inject/handler.go Outdated Show resolved Hide resolved
connect-inject/envoy_sidecar_test.go Outdated Show resolved Hide resolved
connect-inject/envoy_sidecar.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-requesting review: addressed PR comments, and added functionality to pass the envoy binary arguments via pod annotations as well.

Also planning to squash and merge this one when its ready since the git history on this one's a bit funky and there's a merge commit in here.

@ndhanushkodi ndhanushkodi changed the title Extra envoy args Support passing extra arguments to injected envoy binary Nov 6, 2020
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🎉 💃 ! I've left a couple of edits, but they are non-blocking.

CHANGELOG.md Outdated Show resolved Hide resolved
connect-inject/handler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳 Brilliant!!

connect-inject/envoy_sidecar.go Outdated Show resolved Hide resolved
connect-inject/envoy_sidecar.go Outdated Show resolved Hide resolved
@ndhanushkodi ndhanushkodi merged commit fde8f5f into master Nov 6, 2020
@ndhanushkodi ndhanushkodi deleted the extra-envoy-args branch November 6, 2020 19:39
ndhanushkodi added a commit to ndhanushkodi/consul-k8s that referenced this pull request Nov 6, 2020
…rgs"

This reverts commit fde8f5f, reversing
changes made to 32ade6c.
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants