-
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 namespace toggle to tproxy #510
Conversation
// Check if tproxy is enabled on this pod. | ||
tproxyEnabled, err := transparentProxyEnabled(pod, h.EnableTransparentProxy) | ||
tproxyEnabled, err := transparentProxyEnabled(namespace, pod, h.EnableTransparentProxy) |
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.
We now pass the namespace into containerInit from the handler, this allows us to (1) re-use the transparentProxyEnabled()
function between containerInit/endpointsController and (2) makes testing simpler so we do not need to load up a k8s clientset for every containerInit test.
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.
This looks really great!! Have a few suggestions for improvements!!
CHANGELOG.md
Outdated
* If set as a `pod annotation` - This will define whether tproxy is enabled/disabled for the pod. | ||
* If set as a `namespace label` - This will define the default behaviour for pods in this namespace which do not have their respective annotation set. | ||
* If not set on either pod or namespace - The default behaviour will be defined by the `connectInject.transparentProxy.defaultEnabled` helm value. |
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.
Given this a change in consul-k8s
, can you rephrase the connectInject.transparentProxy.defaultEnabled
helm value as the equivalent flag and sub-command it gets set on in consul-k8s
?
Additionally, I'd phrase it as annotation on the Pod and label on the Namespace. I don't think pod annotation
and namespace label
seem accurate.
Conversely:
Setting the annotations consul.hashicorp.com/transparent-proxy
to true/false
on the pod will define whether tproxy is enabled/disabled for the pod. Maybe?
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.
Looks great Kyle!!
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.
great work bud!!
Co-authored-by: Ashwin Venkatesh <ashwin@hashicorp.com>
Changes proposed in this PR:
renames tproxy annotation to "consul.hashicorp.com/transparent-proxy-default"How I've tested this PR:
Unit tests, manual testing via deploying a simple counting service with no tproxy annotations set, then toggling the namespace label true/false and redeploying:
NOTE: hashicorp/consul-helm#942 must be merged in order to support this.
How I expect reviewers to test this PR:
Unit tests, manual test
Checklist: