-
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: Make sure OVN certificates are renewed when needed #562
Conversation
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. OST can tell you if it works:)
/ost |
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
/ost |
failed clean install |
Hopefully fixed. Unless more setup plugin dependencies pop up. Let's see. |
/ost |
/ost he-basic-suite-master rhel8 |
/ost basic-suite-master rhel8 |
OST failure unrelated. |
unrelated indeed, but that's because we can't test engine on rhel unfortunately. I'll run that manually |
/ost he-basic-suite-master |
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.
Looks good, but some comments inside.
@@ -906,6 +906,7 @@ def _is_provider_installed(self): | |||
|
|||
@plugin.event( | |||
stage=plugin.Stages.STAGE_CUSTOMIZATION, | |||
name=oenginecons.Stages.OVN_PROVIDER_INIT, |
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.
I'd call it OVN_PROVIDER_CUSTOMIZATION, if you do want to add a name.
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.
Not needed anymore, dropped.
oenginecons.Stages.CERTIFICATE_CHECK, | ||
), | ||
after=( | ||
oenginecons.Stages.OVN_PROVIDER_INIT, | ||
), | ||
condition=lambda self: self._enabled or 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.
Perhaps we can get rid of this function altogether and just move the call to _generate_pki to inside the 'if' in line 933? And while at it, to lower confusion, perhaps rename _generate_pki to something that better describes what it does - which is only to affect future generation of pki, not actual generation (which should only happen at STAGE_MISC, not _CUSTOMIZATION). E.g. "_add_ovn_pki_entities" or something like that.
No harm in keeping the addition of CERTIFICATE_CHECK and adding it to _customization
's before=
, but it's not needed, as they are already ordered by the titles (product options running before pki). Generally speaking, the *_TITLE_*
names are somewhat misleading - controlling the ordering is an important part of their functionality at least as their emission of titles.
If you disagree, perhaps rename current function - _misc might hint that it's running in STAGE_MISC.
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.
Good ideas, thank you, done.
It has been working without it but with the dependency change in the followup commit the events are reordered, the missing dependency manifests and the initial engine-setup may fail.
Due to incorrect staging and missing dependencies, OVN certificates are not renewed if there are no other certificate renewals needed. This patch fixes it by changing when OVN certificate info to PKI entities is added. It must be done: - In STAGE_CUSTOMIZATION rather than STAGE_MISC. STAGE_CUSTOMIZATION is where we ask for certificate renewal if needed. - After the OVN plugin attributes are initialized so that the event condition doesn’t crash on a missing attribute. - Before certificate renewal check is performed. All this can be achieved by moving the single line of _misc_pki method to _customization method. It must be also done after self.environment[oenginecons.PKIEnv.ENTITIES] is initialized but that happens in STAGE_INIT, which is run early so no change is needed here. Note that we cannot add the OVN certificate info there in STAGE_INIT because we don’t know yet whether OVN is used. Bug-Url: https://bugzilla.redhat.com/2097558 Bug-Url: https://bugzilla.redhat.com/2097560
It doesn’t generate any PKI, just adds info about the required certificates to the environment.
/ost |
/ost he-basic-suite-master |
Due to incorrect staging and missing dependencies, OVN certificates
are not renewed if there are no other certificate renewals needed.
This patch fixes it by changing when OVN certificate info to PKI
entities is added. It must be done:
In STAGE_CUSTOMIZATION rather than STAGE_MISC. STAGE_CUSTOMIZATION
is where we ask for certificate renewal if needed.
After the OVN plugin attributes are initialized so that the event
condition doesn’t crash on a missing attribute.
Before certificate renewal check is performed.
It must be also done after
self.environment[oenginecons.PKIEnv.ENTITIES] is initialized but that
happens in STAGE_INIT, which is run early so no change is needed
here. Note that we cannot add the OVN certificate info there in
STAGE_INIT because we don’t know yet whether OVN is used.
Bug-Url: https://bugzilla.redhat.com/2097558
Bug-Url: https://bugzilla.redhat.com/2097560