Implement cluster source fields with external secrets #1178
Conversation
andrewstucki
requested review from
RafalKorepta,
chrisseto and
gene-redpanda
as code owners
|
Note that we currently have no acceptance tests that validate the work here. I can add one maybe after this general shape lands using something like localstack, but don't want to complicate an already extremely large diff. |
andrewstucki
changed the title
| Brokers: []string{"kafka:9092"}, | ||
| SASL: &ir.KafkaSASL{ | ||
| Username: "user", | ||
| Password: &ir.ValueSource{}, |
There was a problem hiding this comment.
I think this change is wrong as the test case is named nil values handling. Could you change this to Password: nil?
| // Should the value be contained in a k8s secret rather than configmap, we can refer | ||
| // to it here. |
There was a problem hiding this comment.
NIT: For consistency the documentation should be exactly the same as for ConfigMapKeyRef or it the comment should clearly state please see ConfigMapKeyRef.
| if source := o.GetClusterSource(); source != nil { | ||
| if spec := source.GetKafkaAPISpec(); spec != nil { | ||
| return spec | ||
| return redpandav1alpha2.ConvertKafkaAPISpecToIR(obj.GetNamespace(), spec) |
There was a problem hiding this comment.
| return redpandav1alpha2.ConvertKafkaAPISpecToIR(obj.GetNamespace(), spec) | |
| return redpandav1alpha2.ConvertKafkaAPISpecToIR(o.GetNamespace(), spec) |
I think it better match after type check. Never the less it would be good to be consistent with using obj.GetNamespace() or o.GetNamespace().
| if source := o.GetRemoteClusterSource(); source != nil { | ||
| if spec := source.GetKafkaAPISpec(); spec != nil { | ||
| return spec | ||
| return redpandav1alpha2.ConvertKafkaAPISpecToIR(obj.GetNamespace(), spec) |
operator/internal/controller/redpanda/shadow_link_controller.go
Outdated
Show resolved
Hide resolved
| return nil, err | ||
| } | ||
| return commonTLS.Config(ctx, k8sClient) | ||
| return commonTLS.Config(ctx, k8sClient, nil) |
There was a problem hiding this comment.
Code comment could help future me why we don't need external secret expander.
| return nil, err | ||
| } | ||
| return commonTLS.Load(ctx, k8sClient) | ||
| return commonTLS.Load(ctx, k8sClient, nil) |
There was a problem hiding this comment.
Code comment could help future me why we don't need external secret expander.
There was a problem hiding this comment.
yeah, I'm not entirely sure where we want to comment that, but the main thing is that the commonTLS stuff here is synthetically constructed via looking at the v1 cluster, it doesn't actually come from a StaticConfiguration, so there's literally no way for it to be an external secret. It's always going to be a SecretKeyRef if commonTLS exists at all based on the returns of this function:
|
@RafalKorepta I'll circle back on some of the NIT comments after this lands just so we can unblock some folks. |
This implements external secret-configurable fields any place where cluster sources may be specified. Under cluster source, the affected fields:
Deprecated:
Kafka:
clusterSource.staticConfiguration.kafka.tls.caCertSecretKeyRefclusterSource.staticConfiguration.kafka.tls.certSecretRefclusterSource.staticConfiguration.kafka.tls.keySecretRefclusterSource.staticConfiguration.kafka.sasl.passwordSecretRefclusterSource.staticConfiguration.kafka.sasl.oauth.tokenSecretRefclusterSource.staticConfiguration.kafka.sasl.gssapi.passwordSecretRefclusterSource.staticConfiguration.kafka.sasl.awsMskIam.secretKeySecretRefclusterSource.staticConfiguration.kafka.sasl.awsMskIam.sessionTokenSecretRefAdmin:
clusterSource.staticConfiguration.admin.tls.caCertSecretKeyRefclusterSource.staticConfiguration.admin.tls.certSecretRefclusterSource.staticConfiguration.admin.tls.keySecretRefclusterSource.staticConfiguration.admin.sasl.passwordSecretRefclusterSource.staticConfiguration.admin.sasl.tokenSchemaRegistry:
clusterSource.staticConfiguration.schemaRegistry.tls.caCertSecretKeyRefclusterSource.staticConfiguration.schemaRegistry.tls.certSecretRefclusterSource.staticConfiguration.schemaRegistry.tls.keySecretRefclusterSource.staticConfiguration.schemaRegistry.sasl.passwordSecretRefclusterSource.staticConfiguration.schemaRegistry.sasl.tokenAdded:
For all of the above a corresponding
ValueSourcefield with the following structure was added:inline(stringpointer)configMapKeyRef(corev1.ConfigMapKeySelectorpointer)secretKeyRef(corev1.SecretKeySelectorpointer)externalSecretRef(ExternalSecretKeySelectorpointer -- this struct just has anamereference for now)The general naming convetion is just drop the
SecretRefsuffix from the above deprecated fields with the exception oftokenwhich was renamedauthToken. Thus the following fields were added:Kafka:
clusterSource.staticConfiguration.kafka.tls.caCertclusterSource.staticConfiguration.kafka.tls.certclusterSource.staticConfiguration.kafka.tls.keyclusterSource.staticConfiguration.kafka.sasl.passwordclusterSource.staticConfiguration.kafka.sasl.oauth.tokenclusterSource.staticConfiguration.kafka.sasl.gssapi.passwordclusterSource.staticConfiguration.kafka.sasl.awsMskIam.secretKeyclusterSource.staticConfiguration.kafka.sasl.awsMskIam.sessionTokenAdmin:
clusterSource.staticConfiguration.admin.tls.caCertclusterSource.staticConfiguration.admin.tls.certclusterSource.staticConfiguration.admin.tls.keyclusterSource.staticConfiguration.admin.sasl.passwordclusterSource.staticConfiguration.admin.sasl.authTokenSchemaRegistry:
clusterSource.staticConfiguration.schemaRegistry.tls.caCertclusterSource.staticConfiguration.schemaRegistry.tls.certclusterSource.staticConfiguration.schemaRegistry.tls.keyclusterSource.staticConfiguration.schemaRegistry.sasl.passwordclusterSource.staticConfiguration.schemaRegistry.sasl.authTokenFor all of the above fields, we handle them by prioritizing the old, deprecated fields if specified, but if they are not, the new fields are used.