Ensure external certs have a hash #3535
Conversation
…ject. That ensures that the rootCA is also in the bundle.
… them. There can be a case that a user uses the same certs for two different components. We want to avoid that components get re-rendered due to having different hashes per reconciliation loop.
rene-dekker
force-pushed
the
ensure-external-certs-have-a-hash
branch
from
5aab7db
to
db2bdf7
Compare
| @@ -360,7 +360,7 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re | |||
| r.status.SetDegraded(operatorv1.ResourceReadError, "Failed to get certificate", err, reqLogger) | |||
| return reconcile.Result{}, err | |||
| } else if prometheusCertificate != nil { | |||
| trustedBundle = certificatemanagement.CreateTrustedBundle(prometheusCertificate) | |||
| trustedBundle = certificateManager.CreateTrustedBundle(prometheusCertificate) | |||
There was a problem hiding this comment.
Changed for consistency, to ensure that every bundle will contain the ca as well.
| @@ -329,7 +329,7 @@ func (cm *certificateManager) GetOrCreateKeyPair(cli client.Client, secretName, | |||
| cm.log.V(3).Info("secret %s has invalid DNS names, the expected names are: %v", secretName, dnsNames) | |||
| return keyPair, nil | |||
| } | |||
| } else if keyPair == nil { | |||
| } else { | |||
There was a problem hiding this comment.
The condition if keyPair == nil is always true here, so I removed it.
| @@ -252,7 +253,7 @@ func getSystemCertificates() ([]byte, error) { | |||
| if err == nil { | |||
| return data, nil | |||
| } | |||
| if err != nil && !os.IsNotExist(err) { | |||
| if !os.IsNotExist(err) { | |||
There was a problem hiding this comment.
The condition err != nil is always true here, so I removed it.
There was a problem hiding this comment.
Why are we not checking if err != nil before err == nil ? It is easier to read that this. If we have an error and it is not because the file is missing we return the error.
There was a problem hiding this comment.
Added, but doubtful how much of an improvement it really is.
rene-dekker
force-pushed
the
ensure-external-certs-have-a-hash
branch
from
db2bdf7
to
3270939
Compare
| for _, cert := range t.certificates { | ||
| certs = append(certs, cert) | ||
| } | ||
| var certs []CertificateInterface |
There was a problem hiding this comment.
Changed because it failed static checks.
| } | ||
| var certs []CertificateInterface | ||
| certs = append(certs, t.certificates...) | ||
|
|
||
| sort.Slice(certs, func(i, j int) bool { |
There was a problem hiding this comment.
Is this sorting still needed ? If so, why can't we move this sorting after adding all certificates so that it is available for everybody ?
There was a problem hiding this comment.
Good point, I think that there should not be a need for it, as the order in which things are added to the bundle (which is now backed by a slice, rather than a map) is deterministic.
There was a problem hiding this comment.
We decided to keep the sorting regardless. (See last commit message).
| @@ -252,7 +253,7 @@ func getSystemCertificates() ([]byte, error) { | |||
| if err == nil { | |||
| return data, nil | |||
| } | |||
| if err != nil && !os.IsNotExist(err) { | |||
| if !os.IsNotExist(err) { | |||
There was a problem hiding this comment.
Why are we not checking if err != nil before err == nil ? It is easier to read that this. If we have an error and it is not because the file is missing we return the error.
| @@ -249,12 +238,13 @@ var certFiles = []string{ | |||
| func getSystemCertificates() ([]byte, error) { | |||
There was a problem hiding this comment.
Removed redundancy which failed the static-checks.
| if !os.IsNotExist(err) { | ||
| return nil, fmt.Errorf(fmt.Sprintf("error occurred when loading system root certificate with name %s", filename), err) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I think it is needed, if we don't have the else then we'd do the return when os.IsNotExist(err) is true.
Although we may strictly not need the sorting, we conclude that sorting may still be desirable. It has the potential to reduce the number of changes and if any variability in insertion error could exist, sorting would prevent a potential bug. No need to change this.
Related to #3503 and #3532
Users may use the same cert for both Kibana and ES. This causes pod restarts, because deployments may get a hash for Kibana OR ES and not both. This change will render both hashes, thus avoiding the restarts.
In addition fixed a small issue where the bundle was not created from the certificateManager. This is the only instance in the non-test code where this is the case.
I was able to demonstrate the fix in a cluster:
The following annotations were on the linseed deploy: