[OCPBUGS-25341]: perform operator apiService certificate validity checks directly #3217

ankitathomas · 2024-05-03T19:50:59Z

Cert updates can occasionally fail silently, updating only the timestamps on the CSV without any changes to the underlying cert secret. This PR uses the cert expiry times directly to retry the refresh.

test/e2e/webhook_e2e_test.go

tmshort · 2024-05-21T15:33:02Z

pkg/controller/operators/olm/apiservices.go

-		if !certs.Active(ca) {
-			logger.Warnf("CA cert not active")
-			errs = append(errs, fmt.Errorf("found the CA cert is not active"))
-			continue
-		}


We're no longer checking the certs (here and below)?

I've combined the cert checks with freshness immediately after the APIService checks for successful/failed CSVs. We don't necessarily need to repeat the checks here again.

dtfranz · 2024-05-22T20:08:28Z

pkg/controller/install/certresources.go

+func (i *StrategyDeploymentInstaller) ShouldRotateCerts(s Strategy) (bool, error) {
+	strategy, ok := s.(*v1alpha1.StrategyDetailsDeployment)
+	if !ok {
+		return false, fmt.Errorf("attempted to install %s strategy with deployment installer", strategy.GetStrategyName())


Could we make this error a little more descriptive? To me it's just saying "we attempted an install" rather than "install failed because: ..." From what I can tell, looks like a failure due to a missing strategy? But I'm not super familiar with this code.

dtfranz · 2024-05-22T20:11:00Z

pkg/controller/install/certresources.go

+		return false, fmt.Errorf("attempted to install %s strategy with deployment installer", strategy.GetStrategyName())
+	}
+
+	hasCerts := map[string]struct{}{}


nitpick and not required: we're using k8s.io/apimachinery/pkg/util/sets in v1 and inconsistently in v0. Might make things a little cleaner but it's not a big deal to me.

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

tmshort

Just a few questions right now.
However, and I realize this is pre-existing code, I don't like creating a certificate before checking it. The process should really be:

Does the cert exist? No -> create the cert
Is the cert need to be rotated? Yes -> rotate/create the cert

Right now, a cert is created every time the rotate check is to be done, and that is a BIG waste of CPU.

tmshort · 2024-06-12T15:15:45Z

pkg/controller/install/certresources.go

+func HostnamesForService(serviceName, namespace string) []string {
+	return []string{
+		fmt.Sprintf("%s.%s", serviceName, namespace),
+		fmt.Sprintf("%s.%s.svc", serviceName, namespace),


not adding .cluster nor .cluster.local?

These should resolve even without the cluster domain specified, though there shouldn't be any harm in adding the fqdns for the default cluster domains. We could possibly also lookup the actual domain similar to pkg/package-server/client/util.go, though that may not be necessary

tmshort · 2024-06-12T15:22:35Z

pkg/controller/install/certresources.go

+			} else if apierrors.IsNotFound(err) {
+				return true, nil
+			} else {
+				return false, err


If there's some other error, what happens? This does not rotate the certificate.

I assume it means we enter exponential backoff (at least until we hit the 5m install plan timeout)?

Any other error is likely to be either temporary (in which case it's hopefully resolved next time around), or not (in which case we would likely fail in a similar way trying to rotate the cert).

The only scenario that is coming to mind where Get secret fails, but a subsequent Create or Update would succeed would be if we lack read permission but have write permission.

tmshort · 2024-06-12T15:34:22Z

pkg/controller/operators/olm/operator.go

-			// Re-sync if kube-apiserver was unavailable
-			if apierrors.IsServiceUnavailable(installErr) {
-				logger.WithError(installErr).Info("could not update install status")
-				syncError = installErr
-				return
-			}


Why was this moved lower? If there's an error, shouldn't we check it first?

I had a similar thought. Maybe we shouldn't timeout the install plan if the reason for things taking so long is that KAS is unavailable.

joelanford

I did a quick pass and I think this looks good overall, but to be honest I'm a bit lost in the depths of this code.

The one question that I had that came to mind: how do we force ourselves to check if certs need to be rotated? If nothing else in the system is changing, are we doing any of:

re-reconcile if the cert secret changes
re-reconcile at a time in the future base on the current secrets' expiration time.

It could totally be that we already handle that part correctly, but it seems like an important detail to understand about this bug.

pkg/controller/install/certresources.go

joelanford · 2024-06-12T15:48:01Z

pkg/controller/install/certresources.go

+		return false, fmt.Errorf("failed to install %s strategy with deployment installer: unsupported deployment install strategy", strategy.GetStrategyName())
+	}
+
+	hasCerts := sets.NewString()


I believe this is deprecated. The sets package now supports/prefers the generic type.

Suggested change

hasCerts := sets.NewString()

hasCerts := sets.New[string]()

timeouts after API availability checks, add service FQDN to list of hostnames. Signed-off-by: Ankita Thomas <ankithom@redhat.com>

ankitathomas · 2024-06-13T15:57:59Z

* re-reconcile if the cert secret changes

We already do this for all olm managed secrets

* re-reconcile at a time in the future base on the current secrets' expiration time.

With each reconcile, we check all certs for anything that expires in a day or less and rotate all of those, including the ones that are already expired. The problem was that we were checking the cert freshness timestamps on the CSV to make those checks, and those were sometimes being incorrectly updated when the cert rotate hadn't really succeeded.

joelanford · 2024-06-13T17:51:36Z

With each reconcile, we check all certs for anything that expires in a day or less and rotate all of those, including the ones that are already expired.

But I'm wondering if there's a scenario where:

I install an operator that needs a cert
OLM creates the secrets, sets expiration to now + N
Everything reconciles, steady state is achieved, no further changes are made to the CSV.
N time passes, and the certs expire. Still no changes, so still no cert renewal?

Is there something that forces a re-reconcile inside the time window where:

The certs are not yet expired, but
They are close enough to expiration that we'll rotate them.

tmshort

/lgtm

Although we may want to look at the order of operations when a cert is to be rotated. As, this could lead to CPU spikes.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2024

openshift-ci bot requested review from akihikokuroda and joelanford May 3, 2024 19:51

ankitathomas force-pushed the cert-rotate branch from 94005cb to bf96d68 Compare May 7, 2024 19:08

ankitathomas force-pushed the cert-rotate branch from bf96d68 to bdd559e Compare May 17, 2024 21:37

ankitathomas changed the title ~~WIP: [OCPBUGS-25341]: perform operator apiService certificate validity checks directly~~ [OCPBUGS-25341]: perform operator apiService certificate validity checks directly May 17, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2024

ankitathomas force-pushed the cert-rotate branch 2 times, most recently from 5373cc0 to 1f59431 Compare May 21, 2024 13:39

ankitathomas requested review from dtfranz and tmshort May 21, 2024 13:49

tmshort reviewed May 21, 2024

View reviewed changes

test/e2e/webhook_e2e_test.go Outdated Show resolved Hide resolved

tmshort reviewed May 21, 2024

View reviewed changes

ankitathomas force-pushed the cert-rotate branch 2 times, most recently from 129df3a to 1fc69f8 Compare May 21, 2024 22:23

dtfranz reviewed May 22, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2024

ankitathomas force-pushed the cert-rotate branch from 83a83a6 to 1b113fd Compare June 12, 2024 14:46

perform operator apiService certificate validity checks directly

8083e10

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

ankitathomas force-pushed the cert-rotate branch from 1b113fd to d6c60b3 Compare June 12, 2024 14:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2024

tmshort reviewed Jun 12, 2024

View reviewed changes

joelanford reviewed Jun 12, 2024

View reviewed changes

pkg/controller/install/certresources.go Show resolved Hide resolved

joelanford reviewed Jun 12, 2024

View reviewed changes

use sets to track certs to install, revert to checking for installPlan

5d5112f

timeouts after API availability checks, add service FQDN to list of hostnames. Signed-off-by: Ankita Thomas <ankithom@redhat.com>

ankitathomas force-pushed the cert-rotate branch from d6c60b3 to 5d5112f Compare June 12, 2024 22:02

ankitathomas requested a review from tmshort June 13, 2024 17:26

ankitathomas requested a review from joelanford June 13, 2024 17:26

tmshort approved these changes Jun 18, 2024

View reviewed changes

openshift-ci bot assigned tmshort Jun 18, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2024

tmshort added this pull request to the merge queue Jun 18, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 18, 2024

kevinrizza added this pull request to the merge queue Jun 20, 2024

Merged via the queue into operator-framework:master with commit 908da0c Jun 20, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPBUGS-25341]: perform operator apiService certificate validity checks directly #3217

[OCPBUGS-25341]: perform operator apiService certificate validity checks directly #3217

ankitathomas commented May 3, 2024

tmshort May 21, 2024

ankitathomas May 21, 2024 •

edited

Loading

dtfranz May 22, 2024

dtfranz May 22, 2024

tmshort left a comment

tmshort Jun 12, 2024

ankitathomas Jun 12, 2024 •

edited

Loading

tmshort Jun 12, 2024

joelanford Jun 12, 2024

tmshort Jun 12, 2024

joelanford Jun 12, 2024

joelanford left a comment

joelanford Jun 12, 2024

ankitathomas commented Jun 13, 2024

joelanford commented Jun 13, 2024

tmshort left a comment

[OCPBUGS-25341]: perform operator apiService certificate validity checks directly #3217

[OCPBUGS-25341]: perform operator apiService certificate validity checks directly #3217

Conversation

ankitathomas commented May 3, 2024

Choose a reason for hiding this comment

ankitathomas May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitathomas Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitathomas commented Jun 13, 2024

joelanford commented Jun 13, 2024

tmshort left a comment

Choose a reason for hiding this comment

ankitathomas May 21, 2024 •

edited

Loading

ankitathomas Jun 12, 2024 •

edited

Loading