Skip to content
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

[3.7] Redeploy etcd certificates during upgrade when etcd hostname not present in etcd serving cert SAN. #7914

Merged
merged 6 commits into from
Apr 16, 2018

Conversation

vrutkovs
Copy link
Member

@vrutkovs vrutkovs commented Apr 11, 2018

Cherry-pick of #6859 and #6926 on 3.7 branch

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565762

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 11, 2018
@vrutkovs vrutkovs force-pushed the 3.7-bz1536217 branch 2 times, most recently from bd7ddc1 to b07b897 Compare April 11, 2018 15:28
@sdodson
Copy link
Member

sdodson commented Apr 11, 2018

I had to patch in the new filter and a few other things. I'm also afraid right now that my cert is not always getting replaced. I'll push patches to your branch when i get it working. I have a decent way to iterate on it right now.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 11, 2018
when:
- true in hostvars | oo_select_keys(groups['oo_etcd_to_config']) | oo_collect('__etcd_cert_lacks_hostname') | default([false])
vars:
etcd_certificates_redeploy: True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this it wasn't re-deploying the certs. It was properly determining that the cert lacked proper SAN but not updating them.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2018
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2018
Copy link
Member

@sdodson sdodson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved w/ the additional commit i just added

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 13, 2018

@vrutkovs: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/system-containers 9f90bc5 link /test system-containers
ci/openshift-jenkins/logging 9f90bc5 link /test logging
ci/openshift-jenkins/gcp 9f90bc5 link /test gcp
ci/openshift-jenkins/install 9f90bc5 link /test install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

""" Parses SubjectAlternativeNames from a PEM certificate.
Ex: certificate = '''-----BEGIN CERTIFICATE-----
MIIEcjCCAlqgAwIBAgIBAzANBgkqhkiG9w0BAQsFADAhMR8wHQYDVQQDDBZldGNk
LXNpZ25lckAxNTE2ODIwNTg1MB4XDTE4MDEyNDE5MDMzM1oXDTIzMDEyMzE5MDMz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put a '...' to abbreviate? I'd prefer not to have this giant wall of text, let's keep the cert data to 2 or 3 lines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from lib_utils_oo_parse_certificate_san vs oo_parse_certificate_san this is just a straight backport from master. I'm not sure this is worth fixing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't notice the branch. :)

Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

""" Parses SubjectAlternativeNames from a PEM certificate.
Ex: certificate = '''-----BEGIN CERTIFICATE-----
MIIEcjCCAlqgAwIBAgIBAzANBgkqhkiG9w0BAQsFADAhMR8wHQYDVQQDDBZldGNk
LXNpZ25lckAxNTE2ODIwNTg1MB4XDTE4MDEyNDE5MDMzM1oXDTIzMDEyMzE5MDMz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't notice the branch. :)

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2018
@abutcher
Copy link
Member

/lgtm

@sdodson sdodson merged commit c2b36cc into openshift:release-3.7 Apr 16, 2018
@vrutkovs
Copy link
Member Author

/cherrypick release-3.7-hotfix

@openshift-cherrypick-robot

@vrutkovs: new pull request created: #8154

In response to this:

/cherrypick release-3.7-hotfix

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants