Add password auth support #47
Conversation
bobbyiliev
requested review from
alex-hunt-materialize,
kay-kim,
def- and
jubrad
| - Determines how users authenticate with the Materialize instance. | ||
| - Valid values are: | ||
| - `"None"` (default): No password authentication is enabled. | ||
| - `"Password"`: Enables password authentication for the `mz_system` user. When set to `"Password"`, you **must** provide a value for `external_login_password_mz_system`. |
There was a problem hiding this comment.
This is totally fine, but we could also generate one for them if they don't provide one.
| ] | ||
| ``` | ||
|
|
||
| If `authenticator_kind` is not set or set to `"None"`, password authentication is disabled and `external_login_password_mz_system` is ignored. |
There was a problem hiding this comment.
Could we cause an error instead? I'm afraid someone might set external_login_password_mz_system and be confused that they can still log in without a password because they forgot to set the authenticator_kind.
An even more intuitive scheme would be to only have a password, and if it's set, switch the authenticator kind to password, otherwise none.
There was a problem hiding this comment.
Good point! Just updated this.
There was a problem hiding this comment.
I do agree this is the easiest to use, but it's also creates an interface that doesn't map with the k8s crd or envd binary. In this case, I think the cleanest thing to do is mirror the underlying behavior.
There was a problem hiding this comment.
hm yes, I see your point. I do like the simplicity of the implicit behavior, but happy to revert the last change if @def- and @alex-hunt-materialize agree that staying closer to the original model is the better call here.
There was a problem hiding this comment.
We could also make it more intuitive in k8s crd/envd binary!
There was a problem hiding this comment.
From my experience it's generally acceptable to only read flags for a feature when that feature is enabled and to ignore them when the feature is disabled without error. This is especially true on the helm chart side.
I think keeping our logic to match the CRD (not doing any TF magic) is going to provide the best continuity of understanding as users dig deeper into problems and start looking at k8s directly. It's definitely possible that we want to simplify the crd/envd binary. I think we can resist that later, especially if the configuration gets more complex.
Co-authored-by: alex-hunt-materialize <[email protected]>
Once the new release is ready and this is merged, we should also merge the following along with bumping the module version accordingly: