-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Requeue when TLS secret isn't found #537
Conversation
closes OSPRH-7090 Signed-off-by: Fabricio Aguiar <[email protected]>
@@ -243,6 +243,10 @@ func EnsureCert( | |||
// get cert secret | |||
certSecret, _, err := secret.GetSecret(ctx, helper, certSecretName, namespace) | |||
if err != nil { | |||
if k8s_errors.IsNotFound(err) && op == controllerutil.OperationResultCreated { | |||
helper.GetLogger().Info(fmt.Sprintf("Secret %s not found, reconcile in %s", certSecretName, cert.timeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good approach. This would hide that the cert is missing just as a log entry in the controller manager pod log. I think we should still pass it as an error to the upper level and either handle there the IsNotFound or just reflect it as an error and reconcile based on the updated event on the cert. In general related to the Jira I think its a valid information, especially if there is really an issue with cert-manager and the deployment won't progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requeueing when the cert was created makes sense (give more time to see if the secret gets created), in any other case we should raise an error. On another level, we can't know if the cert was recently created or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this will prevent getting the error when there is an issue and the cert secret creation won't progress. The error should be visible. Why is it an issue if the upper level reports for a short time that the cert secret is not there. Its a valid situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would prevent the first time, but after requeueing it would raise the error. In a similar situation, we had the feedback that displaying an error while it is just a failed attempt (that the operator would handle by itself) is confusing to the users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, sorry missed the op == controllerutil.OperationResultCreated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@@ -243,6 +243,10 @@ func EnsureCert( | |||
// get cert secret | |||
certSecret, _, err := secret.GetSecret(ctx, helper, certSecretName, namespace) | |||
if err != nil { | |||
if k8s_errors.IsNotFound(err) && op == controllerutil.OperationResultCreated { | |||
helper.GetLogger().Info(fmt.Sprintf("Secret %s not found, reconcile in %s", certSecretName, cert.timeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, sorry missed the op == controllerutil.OperationResultCreated
/cherrypick 18.0.0-proposed |
closes OSPRH-7090