Roll pod when cert changed #5549
Conversation
There was a problem hiding this comment.
Few nits but LGTM otherwise.
Do we everywhere check that all the different secrets exist? If not, we might want to couple these two features together, so that we:
- Provide nice error messages (your code currently seems to expect they exist and if the don't will probably just throw some ugly exceptions)
- Get the secrets only once
systemtest/src/test/java/io/strimzi/systemtest/connect/ConnectST.java
Outdated
Show resolved
Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/connect/ConnectST.java
Outdated
Show resolved
Hide resolved
operator-common/src/main/java/io/strimzi/operator/common/operator/resource/SecretOperator.java
Outdated
Show resolved
Hide resolved
| public Future<String> getCertificateAsync(String namespace, CertSecretSource certSecretSource) { | ||
| return this.getAsync(namespace, certSecretSource.getSecretName()) | ||
| .compose(secret -> Future.succeededFuture(secret.getData().get(certSecretSource.getCertificate()))); | ||
| } | ||
|
|
||
| public Future<CertAndKey> getCertificateAndKeyAsync(String namespace, KafkaClientAuthenticationTls auth) { | ||
| return this.getAsync(namespace, auth.getCertificateAndKey().getSecretName()) | ||
| .compose(secret -> Future.succeededFuture(new CertAndKey(secret.getData().get(auth.getCertificateAndKey().getKey()).getBytes(), secret.getData().get(auth.getCertificateAndKey().getCertificate()).getBytes()))); | ||
| } | ||
|
|
||
| public Future<String> getPasswordAsync(String namespace, KafkaClientAuthentication auth) { | ||
| if (auth instanceof KafkaClientAuthenticationPlain) { | ||
| return this.getAsync(namespace, ((KafkaClientAuthenticationPlain) auth).getPasswordSecret().getSecretName()) | ||
| .compose(secret -> Future.succeededFuture(secret.getData().get(((KafkaClientAuthenticationPlain) auth).getPasswordSecret().getPassword()))); | ||
| } | ||
| if (auth instanceof KafkaClientAuthenticationScramSha512) { | ||
| return this.getAsync(namespace, ((KafkaClientAuthenticationScramSha512) auth).getPasswordSecret().getSecretName()) | ||
| .compose(secret -> Future.succeededFuture(secret.getData().get(((KafkaClientAuthenticationScramSha512) auth).getPasswordSecret().getPassword()))); | ||
| } else { | ||
| return Future.failedFuture("Auth type " + auth.getType() + " does not have a password property"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we really want to have these here? It introduces the api module into this quite heavily which was not the case before. They are also very specific where as the SecretOperator purpose is very generic.
There was a problem hiding this comment.
@tombentley Do we want to pass certificate, key, and password as a String parameter?
There was a problem hiding this comment.
The api is just part of it ... what is the value of having this here instead of in the Util class?
There was a problem hiding this comment.
Currently we've failed to really encapsulate and abstract our certificate handling. What I mean by that is everywhere we pass around a Secret which we happen to know contains a key store (broker certificate and key), or truststore (ca certificate). But that requires that the consumer of that Secret knows what type of secret is it, and how to decode the values within it into something it can use.
I think there is value is trying to better encapsulate these concepts, so rather than passing around Secrets we pass around something higher level. For example a AdminClientSupplier might accept some Strimzi Truststore class and a Strimzi Keystore class. Providing Resource Operators for those things (Truststore and Keystore) would be a logical thing to do. This would provide a path towards being able to abstract where certain credentials are stored (e.g. option to store keys in Vault), and should make the KAO slightly simpler to understand, because there are fewer generic Secrets floating around.
At least that was my motivation for suggesting this, and I'd be interested to know whether people think that's a desirable goal or not (and if they have specific reasons to think it wouldn't work out).
sknot-rh
force-pushed
the
roll-pod-when-cert-changed
branch
from
b46c409 to
a842284
Compare
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sknot-rh I'm fine with the code, but would like to find a consensus on the abstraction idea before we decide to merge this in its current form. |
Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Stanislav Knot <sknot@redhat.com>
sknot-rh
force-pushed
the
roll-pod-when-cert-changed
branch
from
2a399d7 to
48e22c1
Compare
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Type of change
Description
For now, Kafka does not support reloading the certificates dynamically. Thus, the rolling update is needed when the certificate is changed.
Fixes #5524
Checklist