| noonedeadpunk | I kinda also don't like this spoecial handling much - I somehow thought that we agreed on special "standalone" case for user-provided certs, but I was not following too closely tbh | 05:12 |
|---|---|---|
| jrosser | I think this is actually quite simple | 05:44 |
| jrosser | like filter out everything from the install list that defines “src” and exclude it from the chosen backend | 05:44 |
| jrosser | then as a last step filter again choosing only the things with “src” defined and use something like the copy task that we already have | 05:46 |
| jrosser | that’s what I meant by having some sort of shortcut for user defined certs | 05:48 |
| jrosser | and a probably actually mean filter of values of “src” that are truthy | 05:48 |
| noonedeadpunk | and then we can make `neutron_pki_backend: "{{ openstack_pki_backend | default('standalone') }}" pretty much? | 06:35 |
| noonedeadpunk | as this probably does not address one-for-all catching? | 06:35 |
| noonedeadpunk | but then thinking about it.... | 06:36 |
| noonedeadpunk | Why do we even have that complex condition at the first place.... | 06:37 |
| noonedeadpunk | As `neutron_pki_backend` is the new variable we introduce, which defaults to `standalone` | 06:37 |
| noonedeadpunk | so if there is neutron_user_ssl_cert/neutron_user_ssl_key - we don't need to actually pick anything ,rather then leave a default | 06:38 |
| noonedeadpunk | but also the condition should still be implying the filter you're talking about? | 06:50 |
| noonedeadpunk | as regardless of user cert or not - it's same "standalone" backend? | 06:50 |
| noonedeadpunk | so there should be the filter in pki anyway I'd guess | 06:51 |
| jrosser | I think my suggestion is that independent of backend there is just special handling/filtering for things that define a value for “src” | 06:59 |
| jrosser | then that would simplify handling of user defined certs across all the roles | 07:00 |
| jrosser | when it might be that a role has a mix of standalone/pki/acme/whatever backends and perhaps just one of them is user supplied | 07:00 |
| jrosser | feels simple and flexible rather than building logic into all the role default vars for this | 07:01 |
| noonedeadpunk | just to double check - we're talking about logic for `neutron_pki_backend` in https://review.opendev.org/c/openstack/openstack-ansible-os_neutron/+/949420/4/defaults/main.yml#698 right? | 07:03 |
| noonedeadpunk | as this is not what it's doing | 07:03 |
| noonedeadpunk | as what it's doing is kinda noop, imo.... | 07:03 |
| noonedeadpunk | it does not seem to replace src filtering inside of pki role in any way | 07:04 |
| noonedeadpunk | so what you're saying still have to exist with it, I'd guess | 07:04 |
| noonedeadpunk | and what it's doing, is to try to override the openstack_pki_backend if it's defined, if user certs exist | 07:05 |
| noonedeadpunk | so if you set `openstack_pki_backend: vault` but have user keys - user keys would have precedence | 07:06 |
| noonedeadpunk | and I don't think we need that logic at all | 07:06 |
| noonedeadpunk | as what you're describing seems to be implemented in https://review.opendev.org/c/openstack/ansible-role-pki/+/954239/17/tasks/main_certs.yml right? | 07:08 |
| jrosser | yes but I think we lose that in the later patches | 08:08 |
| jrosser | see in particular the difference between my example changes to os_glance and Damian’s to os_neutron | 08:08 |
| noonedeadpunk | ok, I guess I was not reviewing all of them, and focused only on some "starting" points | 08:08 |
| jrosser | 954239 is not sufficient for multiple backends + flexible handling of user supplied certs | 08:12 |
| jrosser | I think another modification is needed on top to generalise that case | 08:12 |
| noonedeadpunk | yeah, could be | 08:13 |
| jrosser | I’m travelling/in the datacenter this week so can’t look much at the actual code | 08:13 |
| noonedeadpunk | but also I don't see too much difference in os_glance and os_neutron patches to be frank | 08:13 |
| noonedeadpunk | it's that os_neutron just covers all changes for vault support in a single patch | 08:13 |
| noonedeadpunk | inlcuding changing of san format and supplying backend with certs | 08:14 |
| noonedeadpunk | and type of the installable thing | 08:14 |
| noonedeadpunk | oh, type is actually in glance as well | 08:15 |
| jrosser | right - I had hoped that the glance patch would also be totally valid for the vault backend | 08:16 |
| jrosser | which is why so much discussion about group vars | 08:16 |
| noonedeadpunk | I commented today somewhere, that group_vars does not always work. For example - imagine HCI metal environment | 08:17 |
| noonedeadpunk | but also - if you want different cert types for qemu/libvirt and ovn-controller | 08:18 |
| noonedeadpunk | I'm not sure there is a valid usecase for this thing though | 08:18 |
| noonedeadpunk | and we really can discuss if different backends should be even supported for deployment | 08:19 |
| noonedeadpunk | but I don't have much against having per-role backend defenition in defaults | 08:19 |
| damiandabrowski | hello, ouh, seems like a lot of things were raised in the morning :D | 08:20 |
| noonedeadpunk | but the ternary logic there is not needed at all | 08:20 |
| noonedeadpunk | imo | 08:20 |
| damiandabrowski | we need to support multiple backends because hashi_vault cannot handle everything | 08:21 |
| noonedeadpunk | oh, rly? | 08:21 |
| damiandabrowski | octavia is an example, where we have octavia CA and sign new certs with it | 08:21 |
| noonedeadpunk | does it have to be an own CA? | 08:21 |
| noonedeadpunk | as I think we made that more for backwards and upgrade compatability? | 08:22 |
| noonedeadpunk | It might make sense for it to be own intermediate - sure | 08:22 |
| noonedeadpunk | jrosser: and then the second obvious difference to me, is san format - which is changed from string to dict | 08:23 |
| damiandabrowski | you need to store private key of the CA(or intermediate) on octavia_server to sign certs for amphora | 08:24 |
| noonedeadpunk | and, to be fair, dict kinda makes more sense in terms of how to operate it | 08:24 |
| noonedeadpunk | and it's easier to make some kind of combine/overrides in place for it? | 08:24 |
| noonedeadpunk | oh, ok, right | 08:26 |
| jrosser | the san format change is fine yes, really thats just another patch stacked on top which my os_glance change does not account for | 08:54 |
| jrosser | and yes i think i'm trying (badly) to say that a filter in the pki role is better for handling the user supplied cert than a complex condition in the role defaults | 08:55 |
| noonedeadpunk | jrosser: so the thing is, that if you're talking about condition in neutron_pki_backend - I don't think it serves *any* purpose. it's just not needed anywhere, as it covers the usecase which does not exist kinda. | 09:29 |
| noonedeadpunk | with that we likely need to have neutron_pki_backend per role as group_vars not always a solution | 09:31 |
| noonedeadpunk | so I think we should be able to do that without anything else https://paste.openstack.org/show/bzZltLWD67szrJ0hTUvb/ | 09:32 |
| noonedeadpunk | or are you talking about smth completely different? | 09:33 |
| noonedeadpunk | as I have feeling we;'re talkign about different things, but I don't see anything else too much related :D | 09:36 |
| damiandabrowski | okay, let's start from beginning - why you think that the condition in neutron_pki_backend does not serve any purpose? | 09:38 |
| damiandabrowski | let's imagine that openstack_pki_backend: hashi_vault, but for neutron user wants to provide user-defined cert with neutron_user_ssl_cert | 09:38 |
| damiandabrowski | how would you handle this case, assuming that hashi_vault backend cannot handle user-defined certs? | 09:39 |
| noonedeadpunk | `neutron_pki_backend: standalone`? | 09:41 |
| noonedeadpunk | This logic would make sense, if we had to care about backwards compatability in this case. | 09:42 |
| noonedeadpunk | But the thing is, that openstack_pki_backend is the new variable we're introducing | 09:42 |
| noonedeadpunk | So for upgrades current behavior is preserved | 09:42 |
| noonedeadpunk | And then if anyone needs to make a concious decision to override globally the backend - they can do it | 09:43 |
| noonedeadpunk | But then they should also think about their custom certs if they have one | 09:43 |
| noonedeadpunk | as migration from standalone to vault should be thought through and planned anyway, imio | 09:44 |
| noonedeadpunk | also ` but for neutron user wants to provide` clearly states that's a specific case for a specific role. So I don't see why they would not need to also override neutron_pki_backend in this case, given they explicitly defined a global openstack_pki_backend | 09:45 |
| noonedeadpunk | that's why I think that this logic basically overthinking and not needed in fact | 09:46 |
| noonedeadpunk | not saying that I have no idea if anybody uses user-provided certs for anything rather then haproxy... | 09:47 |
| jrosser | my change for using names in the pki role is not compatible with `is defined` for user defined certs | 09:48 |
| damiandabrowski | okay, so in this case, we're okay with user having to manually override neutron_pki_backend - I'm fine with that | 09:48 |
| damiandabrowski | I'm just trying to say that login in neutron_pki_backend had some purpose, we may just not need it :D | 09:49 |
| noonedeadpunk | I think it's more then fair expectation in this circumstances | 09:49 |
| damiandabrowski | logic* | 09:49 |
| noonedeadpunk | I'd say it totally would be needed if we were changing the default backend to vault, for instance | 09:50 |
| noonedeadpunk | but yeah | 09:50 |
| noonedeadpunk | jrosser: I still kinda feel that either we are talking about different things or there is some misconception of what this condition is. as it does not substitute nor relate to the need of filtering on PKI side | 09:51 |
| noonedeadpunk | as it's really jsut to be smart about backend selection and have precedence over global openstack_pki_backend when user cert is defined as a var | 09:52 |
| jrosser | sorry /o\ | 09:52 |
| noonedeadpunk | Or I am completely missing smth else | 09:53 |
| jrosser | https://review.opendev.org/c/openstack/openstack-ansible-os_glance/+/954269/9/defaults/main.yml | 09:57 |
| jrosser | i made those vars always defined but empty | 09:57 |
| jrosser | anyway i think we're all talking about slightly different things and it's not particularly productive | 09:58 |
| jrosser | imho having the external interface to the pki role not be tangled up with a specific implementation is most important | 09:58 |
| noonedeadpunk | right | 09:59 |
| noonedeadpunk | I totally agree here | 09:59 |
| noonedeadpunk | though I guess src even in glance patch is somehow tangled with standalone, unless we do some "user" backend which will just install certs? | 10:00 |
| noonedeadpunk | and then we can pass path to the cert as a name kinda... | 10:01 |
| noonedeadpunk | and don't have src as a unique parameter | 10:01 |
| noonedeadpunk | or, document directory structure for user-provided backend | 10:01 |
| jrosser | so my whole idea was to make it so that defining a value for the user defined cert rather than '' made it get installed from that path | 10:02 |
| jrosser | the user didnt have to understand how | 10:02 |
| jrosser | but also at the same time not put logic about specific backends into the service role defaults | 10:02 |
| noonedeadpunk | ok, sure ,we can do that as well | 10:02 |
| noonedeadpunk | I just don't see a logic in https://review.opendev.org/c/openstack/openstack-ansible-os_neutron/+/949420/4/defaults/main.yml except `neutron_pki_backend` which I think we've addressed? | 10:04 |
| noonedeadpunk | as these patches are super simmilar to me, excpet this var | 10:04 |
| noonedeadpunk | (and san and passing backend explicitly) | 10:04 |
| noonedeadpunk | san we agreed that is good, right? | 10:05 |
| noonedeadpunk | and passing backend I find necessary as group_vars might not be enough as it's not always scoping things good enough | 10:06 |
| noonedeadpunk | so I think we are pretty much aligned then, no? | 10:08 |
| damiandabrowski | 1. passing `neutron_pki_backend` may not be necessary if we accept the fact that overriding backend for a single role would require overriding *_pki_certificates and *_pki_install_certificates. | 10:15 |
| damiandabrowski | I think we may accept it considering the fact that doing this(changing backend for a single role) is quite rare thing to do | 10:16 |
| noonedeadpunk | not for octavia, right? | 10:16 |
| noonedeadpunk | and then we would be back to user certs kinda | 10:17 |
| damiandabrowski | but for amphora certs we don't rely on octavia_pki_backend | 10:17 |
| damiandabrowski | we just statically define: backend: standalone | 10:17 |
| damiandabrowski | https://review.opendev.org/c/openstack/openstack-ansible-os_octavia/+/949419 | 10:17 |
| damiandabrowski | so it's fine | 10:18 |
| noonedeadpunk | imo, <service>_pki_backend is reasonable to have | 10:19 |
| noonedeadpunk | it's still a common interface thing for the role. | 10:19 |
| damiandabrowski | I don't have a strong opinion here, we can keep it | 10:19 |
| noonedeadpunk | and again - user certs | 10:21 |
| noonedeadpunk | so to have aa user cert - they need to override <service>pki_install_certificates | 10:21 |
| noonedeadpunk | which can be problematic for upgrades | 10:22 |
| damiandabrowski | what is "aa user cert"? | 10:22 |
| noonedeadpunk | `neutron_user_ssl_cert ` | 10:23 |
| noonedeadpunk | or well, you filter in role, if src is defined - then use standalone? | 10:24 |
| noonedeadpunk | This is what jrosser you meant? ^ | 10:24 |
| damiandabrowski | ah yes, keeping `<service>_pki_backend` would help to avoid overriding the whole *_pki_install_certificates | 10:24 |
| damiandabrowski | choosing pki backend based on src won't work for hashi_vault | 10:25 |
| damiandabrowski | because src is only specified in *_pki_install_certificates, while neutron_pki_backend is used also in *_pki_certificates | 10:25 |
| damiandabrowski | https://review.opendev.org/c/openstack/openstack-ansible-os_neutron/+/949420/comment/870da622_215b554f/ | 10:25 |
| noonedeadpunk | yeah, ok, right | 10:26 |
| noonedeadpunk | now I understand this convo more | 10:26 |
| noonedeadpunk | so I don't see why having <service>_pki_backend is really problematic | 10:26 |
| noonedeadpunk | as group_vars are not fine-grained control of this var and might have very unexpected behavior | 10:27 |
| noonedeadpunk | when service partially would be served by diferent backends | 10:27 |
| noonedeadpunk | damiandabrowski: we had a variable somewhere to disable cert generation, right? | 10:29 |
| noonedeadpunk | https://review.opendev.org/c/openstack/openstack-ansible-os_glance/+/954269/9/tasks/main.yml | 10:29 |
| noonedeadpunk | so, if we refactor this condition a bit, then choosing pki backend based on src will work for hashi_vault | 10:30 |
| noonedeadpunk | as we don't care about pki_certificates if pki_create_certificates is false | 10:31 |
| damiandabrowski | Didn't we just end up right where we started? :D | 10:38 |
| damiandabrowski | 1. Idea: move the logic from `neutron_pki_backend` to the pki role(to fallback to standalone backend when `src` is defined) | 10:39 |
| damiandabrowski | 2. Decision: it's okay to let users override `neutron_pki_backend` when they want to use user-defined certs | 10:39 |
| damiandabrowski | 3. Idea: Refactor other condition in service role logic, to dynamically fallback to standalone backend when user-defined cert is used | 10:39 |
| damiandabrowski | 3. contradicts with the original idea 1. of fully moving the logic to pki role | 10:39 |
| noonedeadpunk | I was just saying that chosing backend on src is possible :) | 10:41 |
| noonedeadpunk | and we still need to have pki_create_certificates logic in place | 10:42 |
| noonedeadpunk | But, I kinda in favor of explicitly supplying backend per role instead of detecting in inside of pki based on data | 10:42 |
| noonedeadpunk | damiandabrowski: one thing though, is that potentially we don't need to supply `backend` in pki_install_certificates or whenever - we can pass it to the pki role directly in all cases except octavia? | 10:45 |
| noonedeadpunk | but then anyway backend defined for cert is having precedence over generic one I guess | 10:45 |
| noonedeadpunk | damiandabrowski: moreover, you already do that: https://review.opendev.org/c/openstack/openstack-ansible-os_neutron/+/949420/4/tasks/main.yml | 10:50 |
| noonedeadpunk | so what's the purpose of `backend: "{{ neutron_pki_backend }}"` in _pki_certificates ? | 10:51 |
| noonedeadpunk | so I think this: | 10:54 |
| damiandabrowski | hmm, let's clarify if we're talking about _pki_certificates or _pki_install_certificates :D | 10:55 |
| noonedeadpunk | both | 10:55 |
| noonedeadpunk | 1. drop complexity for neutron_pki_backend | 10:55 |
| noonedeadpunk | 2. drop backend in certs stanza when it's absolutely needed (Octavia) in favor of include-wide var (as it's same anyway) | 10:56 |
| noonedeadpunk | 3. almost profit? | 10:56 |
| damiandabrowski | maybe you're right and defining backend in _certificates and _install_certificates is reasonable only when its value is different than pki_backend | 10:58 |
| damiandabrowski | I'll test it today | 10:58 |
| noonedeadpunk | I would expect it to be the case, otherwise it could be a bug somewhere | 11:19 |
| noonedeadpunk | 3. adjust pki_create_certificates condition to respect _user_ssl_cert being in defaults | 11:20 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/openstack-ansible master: [doc] Use ${REINSTALLED_HOST} env var for OS re-deployment guide https://review.opendev.org/c/openstack/openstack-ansible/+/955811 | 16:59 |
| opendevreview | Damian Dąbrowski proposed openstack/openstack-ansible-os_neutron master: Add hashi_vault pki backend support https://review.opendev.org/c/openstack/openstack-ansible-os_neutron/+/949420 | 18:47 |
| opendevreview | Damian Dąbrowski proposed openstack/ansible-role-pki master: Add hashi_vault backend https://review.opendev.org/c/openstack/ansible-role-pki/+/948881 | 18:50 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!