-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Apply applicable ASM_OPTS in config_map #1183
Conversation
@Monkeyanator PTAL |
Thanks for the PR! 🚀 |
@@ -45,6 +45,7 @@ resource "kubernetes_config_map" "asm_options" { | |||
|
|||
data = { | |||
multicluster_mode = var.multicluster_mode | |||
ASM_OPTS = var.enable_cni ? "CNI=on" : null |
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: should this be
ASM_OPTS = var.enable_cni ? "CNI=on" : null | |
ASM_OPTS = var.enable_cni ? "CNI=on" : "" |
?
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.
Do we want it to be set to ""
or excluded entirely? Usually I think excluding is better.
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.
When set to null the value is not applied to the configMap. Empty value could trigger unintended behavior.
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.
Agreed, let's stick to null
.
[In autopilot clusters,] subsequent terraform runs overwrite the
ASM_OPTS: "CNI=on"
value in the asm_option config map. This prevents new or restating workloads from being scheduled as the proxy tries to cap add NET_ADMIN on start. This PR conditionally adds the expected setting.