Skip to content

Conversation

@bysnupy
Copy link
Member

@bysnupy bysnupy commented Mar 3, 2019

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 3, 2019
@openshift-ci-robot
Copy link

Hi @bysnupy. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@vikram-redhat
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2019
@vikram-redhat
Copy link
Contributor

@stuartchuan - can you PTAL?

@huffmanca - FYI. If QE passes it, we should be ok to merge and CP.

Copy link

Choose a reason for hiding this comment

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

... can be triggered to be recreated by removing annotations to the router service, deleting the secret and re-adding annotations to the router service, I think the secret should be deleted after removing the annotation, otherwise it will be recreated immediately.

Copy link

Choose a reason for hiding this comment

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

Line 708: service.alpha.openshift.io/serving-cert-signed-by- this annotation should not be removed.

Copy link

Choose a reason for hiding this comment

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

others LGTM, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your review and kind correction @lihongan,

You're right. I changed the sequence of the steps as you mentioned. (removing secret step is after removing annotation step)

And I think it had better to remove service.alpha.openshift.io/serving-cert-signed-by annotation at same time. The annotation also added automatically with the current OpenShift CA signer value, when it is added service.alpha.openshift.io/serving-cert-secret-name again. For example, if new CA is deployed before the router certs deploying work, it can cause not matching CA issue with new CA, because the service.alpha.openshift.io/serving-cert-signed-by's value is old or different with new CA signer value.

Copy link

Choose a reason for hiding this comment

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

That make sense. Thank you @bysnupy

@lihongan
Copy link

lihongan commented Mar 4, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2019
@lihongan
Copy link

/assign vikram-redhat

@vikram-redhat
Copy link
Contributor

@stuartchuan - can you confirm if this is ok from QE's point of view?

@xltian

@stuartchuan
Copy link

@vikram-redhat ,
@lihongan is the QE owner of this part, seems he had reviewed the changes and added lgtm label already.

@vikram-redhat
Copy link
Contributor

Thanks @stuartchuan!

@huffmanca - can you please do a peer review and followup if needed?

@bysnupy
Copy link
Member Author

bysnupy commented Apr 3, 2019

Could you PTAL @lihongan ? "redeploy-router-certificates.yml" makes changes to wrong "service serving certificate secrets" annotation status has already been changed to VERIFIED, if some tasks are required, let me know that. thanks.

Copy link
Contributor

@huffmanca huffmanca left a comment

Choose a reason for hiding this comment

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

I sincerely apologize for the delays in submitting this review. Overall it looks good, just need to tweak two lines so that the numbering isn't broken from the text.

Once these changes are made I will merge and cherry-pick.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2019
"router-metrics-tls" secret should be service serving certificates secret. "router-certs" secret is using as wild card certificates and not service serving certificates secret.

- Fix: https://bugzilla.redhat.com/show_bug.cgi?id=1672011
@bysnupy
Copy link
Member Author

bysnupy commented Apr 11, 2019

@huffmanca I've complete to fix it as mentioned from you. Could you PTAL again ?

@huffmanca
Copy link
Contributor

@bysnupy,

Thanks for making these changes. It looks good to me!

@huffmanca huffmanca added the peer-review-done Signifies that the peer review team has reviewed this PR label Apr 11, 2019
@huffmanca huffmanca merged commit 4b15329 into openshift:master Apr 11, 2019
@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.11

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.10

@openshift-cherrypick-robot

@huffmanca: new pull request created: #14419

Details

In response to this:

/cherrypick enterprise-3.11

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.

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.9

@openshift-cherrypick-robot

@huffmanca: new pull request created: #14420

Details

In response to this:

/cherrypick enterprise-3.10

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.

@openshift-cherrypick-robot

@huffmanca: new pull request created: #14421

Details

In response to this:

/cherrypick enterprise-3.9

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.

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.7

@openshift-cherrypick-robot

@huffmanca: new pull request created: #14422

Details

In response to this:

/cherrypick enterprise-3.7

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.

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.6

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.5

@openshift-cherrypick-robot

@huffmanca: new pull request created: #14423

Details

In response to this:

/cherrypick enterprise-3.6

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.

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.4

@openshift-cherrypick-robot

@huffmanca: #13941 failed to apply on top of branch "enterprise-3.5":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	install_config/redeploying_certificates.adoc
Falling back to patching base and 3-way merge...
Auto-merging install_config/redeploying_certificates.adoc
CONFLICT (content): Merge conflict in install_config/redeploying_certificates.adoc
Patch failed at 0001 Correct the steps for redeploying router certificates

Details

In response to this:

/cherrypick enterprise-3.5

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.

@huffmanca
Copy link
Contributor

/cherrypick enterprise-3.3

@openshift-cherrypick-robot

@huffmanca: #13941 failed to apply on top of branch "enterprise-3.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	install_config/redeploying_certificates.adoc
Falling back to patching base and 3-way merge...
Auto-merging install_config/redeploying_certificates.adoc
CONFLICT (content): Merge conflict in install_config/redeploying_certificates.adoc
Patch failed at 0001 Correct the steps for redeploying router certificates

Details

In response to this:

/cherrypick enterprise-3.4

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.

@openshift-cherrypick-robot

@huffmanca: #13941 failed to apply on top of branch "enterprise-3.3":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	install_config/redeploying_certificates.adoc
Falling back to patching base and 3-way merge...
Auto-merging install_config/redeploying_certificates.adoc
CONFLICT (content): Merge conflict in install_config/redeploying_certificates.adoc
Patch failed at 0001 Correct the steps for redeploying router certificates

Details

In response to this:

/cherrypick enterprise-3.3

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.

@huffmanca
Copy link
Contributor

As the 3.5 through 3.3 cherry-picks failed, these have been manually included under:

#14430
#14431
#14432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-3.3 branch/enterprise-3.4 branch/enterprise-3.5 branch/enterprise-3.6 branch/enterprise-3.7 branch/enterprise-3.9 branch/enterprise-3.10 branch/enterprise-3.11 ok-to-test Indicates a non-member PR verified by an org member that is safe to test. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants