-
Notifications
You must be signed in to change notification settings - Fork 261
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
packaging: fix ovirt ovn provider for keycloak #379
Conversation
5fc596f
to
57ddce4
Compare
Adding the description I prepared offline for Eitan. Perhaps it could be useful for other reviewers. "The motivation behind this change is the scenario when we have existing ovirt engine installation ie. 4.4 or 4.5 without keycloak. Now a user wants to enable keycloak because is ready to migrate it's user base backend (db or ldap or something totally different ie. integrate with google or github sso) to keycloak. The initial version of ovirt keyloak support assumed that keycloak can be only 'activated' on new (clean) installations (first setup). |
57ddce4
to
8862e56
Compare
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.
LGTM
oengcommcons.KeycloakEnv.KEYCLOAK_ENABLED | ||
) | ||
|
||
if ovnprovider_installed and keycloak_enabled: |
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.
With no further condition? Consider this:
- Run engine-setup without keycloak
- Run engine-setup enabling keycloak the first time - this is the run for which you are writing the current patch
- Run again engine-setup, e.g. for an upgrade
It seems like you'll ask for credentials also on (3.).
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.
It is only asking for confirmation whether to use the keycloak sso credentials:
Use default credentials (admin@ovirt@internalsso) for ovirt-provider-ovn (Yes, No) [Yes]:
After some consideration, I agree it does not make any sense because, in order for ovn provider work correctly, it needs the keycloak sso credentials. I will make an update to include this requirement and skip user question (3).
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.
It will also ask for the credentials themselves, because on upgrades we do not have the admin password.
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.
So, do you say that on upgrade, the 'postinstall' file is not used? In ovirt-engine-keycloak there this code snippet:
@osetupattrs(
answerfile=True,
postinstallfile=True,
is_secret=True,
)
def ADMIN_PASSWORD(self):
return 'OVESETUP_KEYCLOAK_CONFIG/adminPassword'
I had an impression that postinstallfile annotation would handle this 'upgrade' case so that no new keycloak password is required, or was I totally wrong?
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.
No, you got it exactly right. My fault. For the internal engine admin password, we deliberately do not save it, because the user can change it later. Isn't it so also for keycloak?
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.
Unfortunately, I have not considered this scenario at all. You're right - it is possible to change admin@ovirt & admin (keycloak admin console) passwords.
keycloak_enabled = self.environment.get( | ||
oengcommcons.KeycloakEnv.KEYCLOAK_ENABLED | ||
) | ||
if keycloak_enabled: |
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.
Same
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.
In this case, when subsequent engine-setup is executed on environment with already enabled keycloak, the ovirt provider ovn config /etc/ovirt-provider-ovn/conf.d/10-setup-ovirt-provider-ovn.conf
will be simply rewritten. This is not the prettiest approach, but it is harmless and I think I saw similar cases elsewhere in setup code. No additional credentials related questions are supposed to be asked.
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.
You are right that there are many other such cases, as I noted in the dwh patch. It will not even be rewritten, because filetransaction won't rewrite it if it already has the content we want. And the secret key is saved in postinstall, and AFAICT with this the content indeed will be the same. Didn't test.
However, if you do accept my suggestion to add a var CONFIGURED, perhaps better use it here too. Or not, if you think it's better to overwrite if needed. In the original place we added CONFIGURED, we actually eventually had to partially ignore it, because we wanted to make further changes to the ssl conf file (and later also had to revert that, due to wanting to rely on crypto-policies, but that's a different story...).
after=( | ||
oenginecons.Stages.OVN_PROVIDER_OVN_DB, | ||
), | ||
condition=lambda self: self._provider_installed, |
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.
Again. Not terribly bad, as I hope it will be harmless, but perhaps better check if we need to update.
Not sure how to best do this, exactly. If you do indeed add a 'KeycloakEnv.CONFIGURED' as discussed in the dwh patch, perhaps you can condition on this - update ovn only if keycloak is ENABLEd but not yet CONFIGURED.
And, to clarify (as I didn't write this in the dwh patch, sorry): I think they should both be in your current KeycloakEnv in engine_common/constants.py and used only from there everywhere - not duplicate ENABLE/D or CONFIGURED in dwh or ovirt-engine-keycloak.
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.
That makes sense. Originally I planned to make that unification after those patches/fixes, but I can see the benefits of having it much earlier. I am going to give it a try now, when the patches are, more or less, doing what they are supposed to do.
8862e56
to
c0ed01f
Compare
This patch fixes ovirt ovn credentials after keycloak is re-enabled. The original issue is caused by the fact that setup code responsible for updating ovn provider credentials (os config and ovirt engine db) is only triggered on first 'new' ovn provider installation. The result was that user had to: 1. manually update: /etc/ovirt-provider-ovn/conf.d/10-setup-ovirt-provider-ovn.conf to add additional line under 'Ovirt' section: ovirt-admin-user-name=admin@ovirt@internalsso 2. in Administration Panel update Providers -> OVN provider -> username&password to match the above As a side effect, this patch also contains all general keycloak variables needed for determining when to activate or not activate it. Previously, these variables used to be scattered across ovirt-engine, ovirt-dwh and ovirt-engine-keycloak packages.
c0ed01f
to
23ba829
Compare
Just to update you about the current state.
Now, I was able to successfully verify all flows I had in mind. Please note, that it is not possible to 'activate' keycloak on already existing environments with just: The reasoning is explained at https://github.com/oVirt/ovirt-engine-keycloak/pull/38/files#diff-214f54e42c9224e0585091f1e23b595d84ed4a044c3d0f35a19aab57fc853cbbR56 |
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.
+1
This patch fixes ovirt ovn credentials after keycloak is re-enabled.
The original issue is caused by the fact that setup code responsible for
updating ovn provider credentials (os configuration and ovirt engine db) is only
triggered on first 'new' ovn provider installation. The result is that
user had to
manually update:
/etc/ovirt-provider-ovn/conf.d/10-setup-ovirt-provider-ovn.conf
to add additional line under 'Ovirt' section:
ovirt-admin-user-name=admin@ovirt@internalsso
in Administration Panel update Providers -> OVN provider ->
username&password to match the above
This patch depends on:
and resolves: