K8SPSMDB-1387 certmanager --enable-certificate-owner-ref option causes no startup of any mongodb clusters #1850
+178
−117
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Demch1k
requested review from
egegunes,
gkech,
hors,
nmarukovich and
pooknull
as code owners
|
|
Demch1k
force-pushed
the
fix-certmanager-owner-ref
branch
from
e219161 to
227c0fe
Compare
egegunes
requested changes
Mar 10, 2025
Contributor
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
few comments.
also I wonder if we need to set this flag while deploying cert-manager in our tests
pkg/psmdb/tls/certmanager.go
Outdated
| return "", errors.Wrap(err, "set controller reference") | ||
| switch errors.Cause(err).(type) { | ||
| case *controllerutil.AlreadyOwnedError: | ||
| fmt.Sprintf("%s", err) |
Contributor
There was a problem hiding this comment.
should we return error here?
pkg/psmdb/tls/certmanager.go
Outdated
| return errors.Wrap(err, "set controller reference") | ||
| switch errors.Cause(err).(type) { | ||
| case *controllerutil.AlreadyOwnedError: | ||
| fmt.Sprintf("%s", err) |
Contributor
There was a problem hiding this comment.
should we return error here?
pkg/psmdb/tls/certmanager.go
Outdated
| } | ||
| if err = controllerutil.SetControllerReference(cr, secret, c.scheme); err != nil { | ||
| return errors.Wrap(err, "set controller reference") | ||
| switch errors.Cause(err).(type) { |
Contributor
There was a problem hiding this comment.
@gkech wdyt of this errors.Cause maybe we should check with errors.Is?
Contributor
There was a problem hiding this comment.
yes it is better @egegunes
@Demch1k let's use errors.Is and also, let's drop switch since it is not needed, so the following for all cases.
if err = controllerutil.SetControllerReference(cr, secret, c.scheme); err != nil {
if errors.Is(err, &controllerutil.AlreadyOwnedError{}) {
return errors.Wrap(err, "set owner reference")
}
return errors.Wrap(err, "set controller reference")
}
Contributor
|
seems like we will need to take this over, i'm moving this to next release |
gkech
changed the title
To fix the issue, we only need to modify the `WaitForCert` method by adding a check to see if the secret has a controller reference to a certificate
Author
egegunes
approved these changes
Nov 6, 2025
Author
|
@pooknull Could you approve this PR? |
Collaborator
commit: 4eb33c1 |
Contributor
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGE DESCRIPTION
https://perconadev.atlassian.net/browse/K8SPSMDB-1387
Problem:
We have enabled --enable-certificate-owner-ref for certmanager and after that mongodb operator can not startup any mongodb clusters.
Cause:
Mongodb operator return error when can't update owner references for certificates recources. But with --enable-certificate-owner-ref certmanager do it by itselfs.
Solution:
Catch error connected with already exists owner ref and jus print it out
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability