Keystone auth webhook integration #34
Conversation
heytrav
changed the title
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
from
84e5623 to
0892205
Compare
There was a problem hiding this comment.
I am curious why we don't want to let users override the default using a template variable?
I am not against the default coming from config, but would on by default meet your use case better? That might be simpler?
What version of the chart do we require to get this working? Maybe lets bump the default chart version to match that version in this patch?
magnum_capi_helm/driver.py
Outdated
| @@ -685,6 +685,26 @@ def _update_helm_release(self, context, cluster, nodegroups=None): | |||
| } | |||
| values = helm.mergeconcat(values, network_details) | |||
|
|
|||
| # CatalystCloud: K8s keystone auth webhook | |||
| if CONF.capi_helm.k8s_keystone_auth_enabled: | |||
There was a problem hiding this comment.
I think it would be more consistent to use a template variable for this? But I am guessing this doesn't work for you use case? Would making it on by default help, because I see a strong case for that.
There was a problem hiding this comment.
Indeed I hadn't considered using a template variable because, as you mention, it doesn't fit our use case. However I am happy to change this to a template variable.
There was a problem hiding this comment.
I've changed it to a template variable and adjusted the tests and minimum helm chart version accordingly.
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
3 times, most recently
from
0e5ecc6 to
e3adb2a
Compare
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
5 times, most recently
from
6d7eb14 to
bf5a478
Compare
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
from
bf5a478 to
5624549
Compare
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
2 times, most recently
from
e2b85aa to
ba42d13
Compare
|
Similar to #32, since rebasing the linter now complains about some files that weren't changed. Running |
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
from
ba42d13 to
d2cfb6f
Compare
updated |
| "enabled": True, | ||
| "values": { | ||
| "openstackAuthUrl": context.auth_url, | ||
| "projectId": context.project_id, |
There was a problem hiding this comment.
This might create issues with admin users updating clusters, but I don't want to block on that now, might be worth a follow up doing cluster.project_id instead here, for safety.
|
Arg, sorry, it conflicted with your other change again 🤦 |
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
from
4dac7a3 to
6434175
Compare
Thanks @JohnGarbutt! No worries. I expected there would be some rebasing required. Just pushed that up so hopefully all the tests and linting are all good. |
* Make keystone auth webhook configurable with a template label * on by default * Add unittests for k8s-keystone-auth config
heytrav
force-pushed
the
feat/k8s-keystone-auth-webhook
branch
from
6434175 to
3c9c9de
Compare
| @@ -1189,6 +1189,9 @@ def _get_cluster_helm_standard_values(self): | |||
| "machineSSHKeyName": None, | |||
| } | |||
|
|
|||
| @mock.patch.object( | |||
| driver.Driver, "_get_k8s_keystone_auth_enabled", return_value=False | |||
There was a problem hiding this comment.
Nit: it would be good to have one test with return_value is True, but I don't want to block this one any longer!
No description provided.