[OCPBUGS-25341]: perform operator apiService certificate validity checks directly #3217
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
ankitathomas
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
ankitathomas
force-pushed
the
cert-rotate
branch
2 times, most recently
from
5373cc0
to
1f59431
Compare
| if !certs.Active(ca) { | ||
| logger.Warnf("CA cert not active") | ||
| errs = append(errs, fmt.Errorf("found the CA cert is not active")) | ||
| continue | ||
| } |
There was a problem hiding this comment.
We're no longer checking the certs (here and below)?
There was a problem hiding this comment.
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.
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
| 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()) |
There was a problem hiding this comment.
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.
| return false, fmt.Errorf("attempted to install %s strategy with deployment installer", strategy.GetStrategyName()) | ||
| } | ||
|
|
||
| hasCerts := map[string]struct{}{} |
There was a problem hiding this comment.
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.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
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.