Skip to content

Conversation

@cybertron
Copy link
Contributor

We only want to generate these cert files when necessary, either on
a new deployment of the registry or when the cert files change.
The previous check for existence meant the latter case wasn't handled
correctly. Even if we fixed that somehow, the md5 check later wasn't
effective either because the directory is bind mounted into the
container so we're essentially checking that the md5 of the file
matches itself.

To address these issues, I've added a version number to the cert and
key files for the registry. If/when we make further changes to the
cert configuration for the registry we just need to bump that version
to force an update of the registry. The registry restart is also
explicitly requested any time the certs are re-generated so we don't
have to try to figure out which cert is in use.

It's possible we could try to inspect the existing cert for differences
with the desired cert, but that would be more complicated and I'm not
sure it would be less error-prone than this approach.

We only want to generate these cert files when necessary, either on
a new deployment of the registry or when the cert files change.
The previous check for existence meant the latter case wasn't handled
correctly. Even if we fixed that somehow, the md5 check later wasn't
effective either because the directory is bind mounted into the
container so we're essentially checking that the md5 of the file
matches itself.

To address these issues, I've added a version number to the cert and
key files for the registry. If/when we make further changes to the
cert configuration for the registry we just need to bump that version
to force an update of the registry. The registry restart is also
explicitly requested any time the certs are re-generated so we don't
have to try to figure out which cert is in use.

It's possible we could try to inspect the existing cert for differences
with the desired cert, but that would be more complicated and I'm not
sure it would be less error-prone than this approach.
@hardys hardys added the CI check this PR with CI label Feb 7, 2020
@hardys hardys self-requested a review February 7, 2020 08:45
@hardys
Copy link

hardys commented Feb 7, 2020

Thanks for fixing this, it's something I missed in #902

I was thinking we'd inspect the existing certs, but this approach seems fine as an alternative - I'll test locally but lgtm!

@metal3ci
Copy link

metal3ci commented Feb 7, 2020

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1494/

@hardys hardys merged commit 4bd9971 into openshift-metal3:master Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI check this PR with CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants