Closed ciroque closed 10 months ago
NOTE: The documentation states that no-tls
is the default mode. Just be sure to update the docs if that changes!
How about this, make this configurable: failOnInsecure
where the Controller will fail on startup when no-tls
is chosen. The default would be on, turn it off to succeed with starting in no-tls
mode.
Something to consider here: the only time no-tls
is highly recommended is in production. Through development, and most likely testing, startup failure due to no-tls
mode will be irritating at best.
@4141done thoughts?
So I think having the controller default to no-tls
is fine and probably does not need an additional flag (if you're going to set the additional flag, you would want to just set the tls-mode
right?). The main thing I think is an issue is defaulting to no-tls
if the value of tls-mode
is specified but not one of the supported values.
I would not want to have a scenario where tls-mode: ca-ntls
makes it through code review and then it's running unsecured but the deployer thinks that it is.
My proposal is:
no-tls
as the defaulttls-mode
is specified in the config map and is NOT one of the supported values, we fail as loudly as possible in the kubernetes context.I think we'd ideally handle the following cases:
ConfigMap
-> Don't apply the config, generate an error Event
, log errorsConfigMap
when one is already valid and in use -> Don't apply the new config, old config is maintained, generate an Error
event, log errors.These should be true whether or not the controller is installed.
I think the best approach based on my reading is to gate the application of a ConfigMap
on validation of the values. I'm new to that concept but it looks like Admission Control would be the way or Dynamic Admission Control. Implementing Admission controllers would allow us to handle all the above cases
This would likely be a smaller lift, but we'd ideally:
Sauce: https://github.com/nginxinc/nginx-loadbalancer-kubernetes/pull/120#discussion_r1377879277
If someone were to typo the TLS Mode in the config, we may wish to fail startup. Couple of thoughts here:
no-tls
) or possibly fail to start and have it not be noticed?iota
-based values, NOT STRINGS. This will help avoid a whole class of issues.