-
Notifications
You must be signed in to change notification settings - Fork 5
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
only add istio automtls when label has value #10574
only add istio automtls when label has value #10574
Conversation
Issues linked to changelog: |
func AddIstioAutomtlsMetadata( | ||
metadata *envoy_config_core_v3.Metadata, | ||
workloadLabels map[string]string, | ||
enableAutoMtls bool, |
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.
nit: I know this is a copy/paste of an existing function, so I'm happy to leave it as is. It feels strange to me that we pass a boolean and only perform an action if that's true. It feels like we could just have the function only be called if the enableAutoMtls value is true
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.
Idk, I'd rather have "pass the global setting in, let some shared code interpret it" than have that conditional in multiple places, as easy as it may be.
enableAutoMtls bool, | ||
) *envoy_config_core_v3.Metadata { | ||
if enableAutoMtls { | ||
// Valid label values are 'istio', 'disabled' |
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.
is it possible to add a link to the istio ref for these values? I already have forgotten the istio semantics regarding these
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.
also just for for clarity i was thinking adding this link to the comment
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.
Linked to the Istio API def that outlines it.
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.
LGTM!
The previous logic didn't cover the possible values described in Istio.
disabled
should not result in us sending mTLS.BOT NOTES:
resolves #10575