[release-4.9] Bug 2060111: Set Upgradeable=False if default cert has no SAN #711

Miciah · 2022-03-02T17:34:18Z

This is a manual cherry-pick of #708 with an additional fix for an issue introduced in #709 (see #709 (comment)).

status: Use `GetPlatformStatus`

Don't use infraConfig.Status.PlatformStatus directly; infraConfig.Status could be nil.

Follow-up to #709.

pkg/operator/controller/ingress/status.go: Use GetPlatformStatus.

Set `Upgradeable=False` if default cert has no SAN

If an ingresscontroller's default certificate has a Common Name (CN) for the ingress domain but has no Subject Alternative Name (SAN) for the same, report that the cluster cannot be upgraded by setting the Upgradeable=False status condition on the ingresscontroller and clusteroperator.

Clients built using Go 1.17 reject certificates without SANs. OpenShift 4.10 is built using Go 1.17, which means that various operators that connect to routes that use the default certificate would reject the certificate and fail to complete the TLS handshake after upgrading to OpenShift 4.10 if the ingress operator didn't block the upgrade on a cluster with a problematic certificate.

pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Get the default certificate secret and pass it to computeIngressUpgradeableCondition.
(computeIngressUpgradeableCondition): Add a parameter for the default certificate secret. Use the argument value to call the new checkDefaultCertificate function to check for problematic certificates.
(checkDefaultCertificate): New function. Return a non-nil error value if the default certificate in the provided secret has a CN for the ingress domain and no SAN for the same.
pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): Verify that computeIngressUpgradeableCondition reports Upgradeable=False if the default certificate has a CN and no SAN for the ingress domain and reports Upgradeable=True otherwise.

Don't use infraConfig.Status.PlatformStatus directly; infraConfig.Status could be nil. Follow-up to commit 9d4a41b. * pkg/operator/controller/ingress/status.go: Use GetPlatformStatus.

openshift-ci · 2022-03-02T17:34:28Z

@Miciah: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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.

Miciah · 2022-03-03T02:29:48Z

TestConfigurableRouteNoConsumingUserNoRBAC failed; the same test failed on #707, so it looks like the test may be flaky.

/test e2e-aws-operator

melvinjoseph86 · 2022-03-03T06:07:24Z

Verified via pre-merge verification workflow, more references related to the test can be found in:
https://bugzilla.redhat.com/show_bug.cgi?id=2060111#c3
/label qe-approved

melvinjoseph86 · 2022-03-03T06:08:26Z

/test e2e-gcp-serial
/test e2e-aws-operator

melvinjoseph86 · 2022-03-03T08:33:41Z

/test e2e-aws-operator

melvinjoseph86 · 2022-03-03T14:22:17Z

/test e2e-aws-operator

Miciah · 2022-03-03T16:46:48Z

Must-gather failed.
/test e2e-aws-operator

Miciah · 2022-03-03T17:10:23Z

/assign @candita

Miciah · 2022-03-03T18:14:31Z

Installer failed:

level=error msg=Error: error updating LB Target Group (arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-9ddchgy8-265e5-qlmhd-aint/638dbe06869a2be6) tags: error tagging resource (arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-9ddchgy8-265e5-qlmhd-aint/638dbe06869a2be6): TargetGroupNotFound: Target groups 'arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-9ddchgy8-265e5-qlmhd-aint/638dbe06869a2be6' not found
level=error msg=	status code: 400, request id: ca03fac1-2eea-451c-9a63-3d90143670ac
level=error
level=error msg=  on ../tmp/openshift-install-cluster-393099372/vpc/master-elb.tf line 45, in resource "aws_lb_target_group" "api_internal":
level=error msg=  45: resource "aws_lb_target_group" "api_internal" {
level=error
level=error
level=fatal msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: failed to complete the change

/test e2e-aws-operator

candita · 2022-03-03T22:32:25Z

pkg/operator/controller/ingress/status.go

+				foundSAN = true
+			}
+		}
+		if cert.Subject.CommonName == domain && !foundSAN {


Is there always going to be a common name, even if there is a SAN?

No, but what we really care about is the situation where a serving certificate is accepted by older clients and rejected by newer clients. Checking that the CN matches the ingresscontroller domain and that no SAN does is the best approach I've come up with to warn when there is a problem without causing false positives. Consider these four cases:

The certificate has no SAN and no CN; such a certificate wouldn't work at all as a serving certificate even for older clients, so we should ignore it.

The certificate has no CN and does have a SAN; this is fine: if the SAN is valid, the certificate will be accepted by both old clients as well as new clients (and if the SAN is invalid, it won't be accepted by old or new clients), so we should ignore it.

The certificate has a CN that doesn't match the ingresscontroller domain; in this case, the certificate is presumably not the serving certificate but rather an intermediate, and anyway, as with the previous case, either it has a valid SAN, in which case both old as well as new clients will accept it, or it does not, in which case both old as well as new clients will reject it, so we should ignore it.

The certificate has a CN that does match the ingresscontroller domain; in this case, the certificate most likely is the serving certificate, and we need to make sure it has a SAN with the same domain in order to still be accepted by newer Go clients.

The last case is really the only one that matters for upgrades, so it is the only one where we set Upgradeable=True if we find a mismatch.

candita · 2022-03-03T22:43:33Z

pkg/operator/controller/ingress/status.go

+// <https://bugzilla.redhat.com/show_bug.cgi?id=2057762>.  This check can be
+// removed after OpenShift 4.9.


This is in 4.9, right?

Suggested change

// <https://bugzilla.redhat.com/show_bug.cgi?id=2057762>. This check can be

// removed after OpenShift 4.9.

// <https://bugzilla.redhat.com/show_bug.cgi?id=2057762>.

Yes, this is the 4.9 backport. Usually I keep the backport as close as possible to the original change (in this case, #708) to minimize risk of breaking something and to simplify future backports.

I was thinking that if someone saw "This check can be removed after OpenShift 4.9." that they might remove it in a later 4.9 version. Or it would at least cause some questions.

Hm, I guess someone could interpret "after OpenShift 4.9" as including newer z-stream releases of 4.9. Maybe I should have said "in OpenShift 4.10 or later" in the original commit. However, even if someone interpreted "after OpenShift 4.9" in that way, I think the risk that someone would remove this in a 4.9 z-stream is low since we have extra levels of review and scrutiny for backports.

I've amended the comment.

candita · 2022-03-04T18:32:48Z

/retest

If an ingresscontroller's default certificate has a Common Name (CN) for the ingress domain but has no Subject Alternative Name (SAN) for the same, report that the cluster cannot be upgraded by setting the Upgradeable=False status condition on the ingresscontroller and clusteroperator. Clients built using Go 1.17 reject certificates without SANs. OpenShift 4.10 is built using Go 1.17, which means that various operators that connect to routes that use the default certificate would reject the certificate and fail to complete the TLS handshake after upgrading to OpenShift 4.10 if the ingress operator didn't block the upgrade on a cluster with a problematic certificate. The commit is related to bug 2057762. https://bugzilla.redhat.com/show_bug.cgi?id=2057762 * pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Get the default certificate secret and pass it to computeIngressUpgradeableCondition. (computeIngressUpgradeableCondition): Add a parameter for the default certificate secret. Use the argument value to call the new checkDefaultCertificate function to check for problematic certificates. (checkDefaultCertificate): New function. Return a non-nil error value if the default certificate in the provided secret has a CN for the ingress domain and no SAN for the same. * pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): Verify that computeIngressUpgradeableCondition reports Upgradeable=False if the default certificate has a CN and no SAN for the ingress domain and reports Upgradeable=True otherwise.

candita · 2022-03-04T19:25:44Z

/lgtm

openshift-ci · 2022-03-04T19:26:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah,candita]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2022-03-04T22:10:28Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-05T00:58:28Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-05T01:00:39Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-bot · 2022-03-10T01:00:36Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-10T01:00:47Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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-bot · 2022-03-11T01:00:34Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-11T01:01:04Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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-bot · 2022-03-12T01:01:12Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-12T01:01:23Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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-bot · 2022-03-13T01:00:16Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-13T01:00:28Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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-bot · 2022-03-14T01:00:20Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-14T01:00:33Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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-bot · 2022-03-15T01:00:24Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-15T01:00:37Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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-bot · 2022-03-16T01:01:16Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2022-03-16T01:01:25Z

@openshift-bot: This pull request references Bugzilla bug 2060111, which is invalid:

expected dependent Bugzilla bug 2059210 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

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.

melvinjoseph86 · 2022-03-16T15:37:10Z

/label cherry-pick-approved

lihongan · 2022-03-17T00:55:33Z

/bugzilla refresh

openshift-ci · 2022-03-17T00:55:42Z

@lihongan: This pull request references Bugzilla bug 2060111, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 2059210 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 2059210 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0, 4.10.z
bug has dependents

Requesting review from QA contact:
/cc @melvinjoseph86

In response to this:

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-bot · 2022-03-17T03:01:11Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-17T04:58:11Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-17T05:11:09Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

melvinjoseph86 · 2022-03-17T06:18:36Z

/test e2e-aws-operator

melvinjoseph86 · 2022-03-17T07:28:12Z

/test e2e-aws-operator

openshift-bot · 2022-03-17T10:40:30Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-03-17T12:52:31Z

@Miciah: all tests passed!

Full PR test history. Your PR dashboard.

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.

openshift-ci · 2022-03-17T12:54:06Z

@Miciah: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#711

Bugzilla bug 2060111 has been moved to the MODIFIED state.

In response to this:

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.

status: Use GetPlatformStatus

654c1a9

Don't use infraConfig.Status.PlatformStatus directly; infraConfig.Status could be nil. Follow-up to commit 9d4a41b. * pkg/operator/controller/ingress/status.go: Use GetPlatformStatus.

openshift-ci bot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Mar 2, 2022

openshift-ci bot requested review from ironcladlou and rfredette March 2, 2022 17:35

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2022

Miciah force-pushed the cherry-pick-710-to-release-4.9 branch from f305946 to 082ddfb Compare March 2, 2022 22:09

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 3, 2022

openshift-ci bot assigned candita Mar 3, 2022

Miciah mentioned this pull request Mar 3, 2022

Bug 2079517: Use externalTrafficPolicy: Cluster with OVN #713

Merged

candita reviewed Mar 3, 2022

View reviewed changes

Miciah force-pushed the cherry-pick-710-to-release-4.9 branch from 082ddfb to ffef526 Compare March 4, 2022 18:36

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022

openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 16, 2022

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Mar 17, 2022

openshift-ci bot requested a review from melvinjoseph86 March 17, 2022 00:55

openshift-merge-robot merged commit 19c161a into openshift:release-4.9 Mar 17, 2022

		// <https://bugzilla.redhat.com/show_bug.cgi?id=2057762>. This check can be
		// removed after OpenShift 4.9.

[release-4.9] Bug 2060111: Set Upgradeable=False if default cert has no SAN #711

[release-4.9] Bug 2060111: Set Upgradeable=False if default cert has no SAN #711

Conversation

Miciah commented Mar 2, 2022

status: Use GetPlatformStatus

Set Upgradeable=False if default cert has no SAN

openshift-ci bot commented Mar 2, 2022

Miciah commented Mar 3, 2022

melvinjoseph86 commented Mar 3, 2022

melvinjoseph86 commented Mar 3, 2022

melvinjoseph86 commented Mar 3, 2022

melvinjoseph86 commented Mar 3, 2022

Miciah commented Mar 3, 2022

Miciah commented Mar 3, 2022

Miciah commented Mar 3, 2022

candita Mar 3, 2022

Choose a reason for hiding this comment

Miciah Mar 3, 2022

Choose a reason for hiding this comment

candita Mar 3, 2022

Choose a reason for hiding this comment

Miciah Mar 3, 2022

Choose a reason for hiding this comment

candita Mar 4, 2022

Choose a reason for hiding this comment

Miciah Mar 4, 2022

Choose a reason for hiding this comment

Miciah Mar 4, 2022

Choose a reason for hiding this comment

candita commented Mar 4, 2022

candita commented Mar 4, 2022

openshift-ci bot commented Mar 4, 2022

openshift-bot commented Mar 4, 2022

openshift-bot commented Mar 5, 2022

openshift-bot commented Mar 5, 2022

openshift-bot commented Mar 10, 2022

openshift-ci bot commented Mar 10, 2022

openshift-bot commented Mar 11, 2022

openshift-ci bot commented Mar 11, 2022

openshift-bot commented Mar 12, 2022

openshift-ci bot commented Mar 12, 2022

openshift-bot commented Mar 13, 2022

openshift-ci bot commented Mar 13, 2022

openshift-bot commented Mar 14, 2022

openshift-ci bot commented Mar 14, 2022

openshift-bot commented Mar 15, 2022

openshift-ci bot commented Mar 15, 2022

openshift-bot commented Mar 16, 2022

openshift-ci bot commented Mar 16, 2022

melvinjoseph86 commented Mar 16, 2022

lihongan commented Mar 17, 2022

openshift-ci bot commented Mar 17, 2022

openshift-bot commented Mar 17, 2022

openshift-bot commented Mar 17, 2022

openshift-bot commented Mar 17, 2022

melvinjoseph86 commented Mar 17, 2022

melvinjoseph86 commented Mar 17, 2022

openshift-bot commented Mar 17, 2022

openshift-ci bot commented Mar 17, 2022

openshift-ci bot commented Mar 17, 2022

status: Use `GetPlatformStatus`

Set `Upgradeable=False` if default cert has no SAN