Fix #626 panic on invalid tls config in secret #627
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tdeverdiere The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
size/S
…f they are nil
tdeverdiere
force-pushed
the
fix-626-panic-on-invalid-tls-config-in-secret
branch
from
75fa8da to
3e4a61a
Compare
knative-prow
bot
added
the
do-not-merge/work-in-progress
knative-prow
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
I left a couple nits regarding the code, otherwise these changes seem reasonable to me. @tdeverdiere have you tested these changes in a k8s cluster both with and without TLS?
| } else { | ||
| // For now just override the previous config. | ||
| r.numConsumers = redisConfig.NumConsumers |
There was a problem hiding this comment.
nit: to make the code a bit cleaner here, instead of using an else, could you return in the if err != nil block?
| } | ||
|
|
||
| func (r *Reconciler) updateTLSSecret(ctx context.Context, secret *corev1.Secret) { | ||
| tlsSecret, err := GetTLSSecret(secret.Data) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Errorw("Error reading TLS configuration", zap.Error(err)) | ||
| } else { |
There was a problem hiding this comment.
nit: to make the code a bit cleaner here, instead of using an else, could you return in the if err != nil block?
| @@ -95,7 +95,8 @@ func (r *Reconciler) updateTLSSecret(ctx context.Context, secret *corev1.Secret) | |||
| tlsSecret, err := GetTLSSecret(secret.Data) | |||
| if err != nil { | |||
| logging.FromContext(ctx).Errorw("Error reading TLS configuration", zap.Error(err)) | |||
| } else { | |||
There was a problem hiding this comment.
nit: to make the code a bit cleaner here, instead of using an else, could you return in the if err != nil block?
Fixes #626
Proposed Changes
Release Note
Docs
#626