Conversation
Signed-off-by: CG <cgpoh76@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for customizing the metadata (annotations and labels) of Secrets generated by the KafkaAccess operator through a new template field in the KafkaAccess spec. This enables users to add custom annotations for tools like secret replicators and to apply custom labels for organization and filtering purposes.
Key changes:
- Introduces a new template API structure (
KafkaAccessTemplate,SecretTemplate,MetadataTemplate) for specifying custom Secret metadata - Updates the reconciler to merge template-specified annotations/labels with existing Secret metadata during updates
- Provides example configurations demonstrating the use of annotations for secret replication tools
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packaging/install/040-Crd-kafkaaccess.yaml |
Adds CRD schema definition for the new template field supporting secret metadata customization |
packaging/helm-charts/helm3/strimzi-access-operator/crds/040-Crd-kafkaaccess.yaml |
Adds CRD schema definition for Helm chart installation |
packaging/examples/kafka-access-with-user.yaml |
Adds example demonstrating custom annotation and label usage |
operator/src/main/java/io/strimzi/kafka/access/server/HealthServlet.java |
Adds explicit constructor and updates class documentation |
operator/src/main/java/io/strimzi/kafka/access/internal/KafkaParser.java |
Adds private constructor to prevent instantiation and improves documentation |
operator/src/main/java/io/strimzi/kafka/access/internal/KafkaAccessMapper.java |
Fixes spelling in javadoc ("Kuberentes" → "Kubernetes") and adds private constructor |
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java |
Implements template handling logic for extracting and applying annotations/labels to Secrets |
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessOperator.java |
Adds explicit constructor and enhances class documentation |
install/040-Crd-kafkaaccess.yaml |
Adds CRD schema definition for the template field |
helm-charts/helm3/strimzi-access-operator/crds/040-Crd-kafkaaccess.yaml |
Adds CRD schema definition for Helm chart |
examples/kafka-access-with-user.yaml |
Adds example usage of template field with custom metadata |
examples/kafka-access-with-reflector.yaml |
New example file showing secret replication tool integration patterns |
api/src/main/java/io/strimzi/kafka/access/model/SecretTemplate.java |
New model class for Secret template configuration |
api/src/main/java/io/strimzi/kafka/access/model/MetadataTemplate.java |
New model class for metadata (annotations/labels) template configuration |
api/src/main/java/io/strimzi/kafka/access/model/KafkaAccessTemplate.java |
New model class for top-level template configuration |
api/src/main/java/io/strimzi/kafka/access/model/KafkaAccessSpec.java |
Adds template field to KafkaAccess spec with getter/setter methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final Map<String, String> currentAnnotations = Optional.ofNullable(secret.getMetadata().getAnnotations()).orElse(new HashMap<>()); | ||
| final Map<String, String> currentLabels = Optional.ofNullable(secret.getMetadata().getLabels()).orElse(new HashMap<>()); | ||
|
|
||
| // Merge template annotations/labels with existing ones (template takes precedence) |
There was a problem hiding this comment.
The comment "Merge template annotations/labels with existing ones (template takes precedence)" is slightly misleading. The merge behavior actually preserves ALL existing annotations/labels and adds/overwrites only the template-specified ones. This means:
- Manually added annotations/labels are preserved across reconciliations
- Removing an annotation/label from the template will NOT remove it from an existing secret
- Template values take precedence only for keys that exist in both maps
Consider clarifying the comment to explicitly state this behavior, as it has important implications for users who might expect the secret metadata to exactly match the template specification:
// Merge template annotations/labels with existing ones.
// Template values override matching keys, but all other existing metadata is preserved.
// Note: Removing a key from the template will not remove it from existing secrets.| // Merge template annotations/labels with existing ones (template takes precedence) | |
| // Merge template annotations/labels with existing ones. | |
| // Template values override matching keys, but all other existing metadata is preserved. | |
| // Note: Removing a key from the template will not remove it from existing secrets. |
| /** | ||
| * Creates a new HealthServlet. | ||
| * This explicit constructor documents the default servlet instantiation. | ||
| */ | ||
| public HealthServlet() { | ||
| super(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] This explicit no-argument constructor that only calls super() is unnecessary and can be removed. Java automatically provides a default no-argument constructor that calls the superclass constructor when no constructors are explicitly defined. The Javadoc plugin warning (if any) should be configured to ignore classes that don't need explicit constructor documentation, rather than adding unnecessary constructors.
| /** | |
| * Creates a new HealthServlet. | |
| * This explicit constructor documents the default servlet instantiation. | |
| */ | |
| public HealthServlet() { | |
| super(); | |
| } |
| /** | ||
| * Creates a new KafkaAccessOperator instance. | ||
| * Explicit constructor added to satisfy Javadoc plugin warning on default constructor. | ||
| */ | ||
| public KafkaAccessOperator() { | ||
| // Intentionally empty. | ||
| } |
There was a problem hiding this comment.
[nitpick] This explicit no-argument constructor is unnecessary and can be removed. Java automatically provides a default no-argument constructor when no constructors are explicitly defined. Since this class only contains static methods, the constructor is never called directly. The Javadoc plugin warning (if any) should be configured to ignore classes that don't need explicit constructor documentation, rather than adding unnecessary constructors.
| /** | |
| * Creates a new KafkaAccessOperator instance. | |
| * Explicit constructor added to satisfy Javadoc plugin warning on default constructor. | |
| */ | |
| public KafkaAccessOperator() { | |
| // Intentionally empty. | |
| } |
# Conflicts: # operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
scholzj
left a comment
scholzj
left a comment
There was a problem hiding this comment.
I think having some description explaining what this is, what it does, and why this is needed would be a good start. You should also not change anything in install, examples and helm-chart folders. Only their versions in packaging.
Thanks @scholzj , I removed the changes in those folders and added description of the PR. |
Can you please elaborate on this? The whole purpose of the Access Operator is that it copies the credentials where needed without any need for additional tooling. If you want to use secret replication tools, you can set the labels/annotations at the secrets at the source (In Strimzi Cluster and User Operators) any skip the whole Access Operator. |
Instead of replicating and referring to 2 secrets (Kafka cluster CA cert secret and Kafka User secret), I just need to replicate 1 secret generated by Access operator and I can connect to my Kafka cluster with the correct credentials. Therefore, I find it very convenient if I can replicate the secret generated by Access operator via annotation. |
I'm not really sure if that is worth the operating effort and running costs TBH. Nor the maintenance effort required of the feature. One Secret or two, seems to make a little difference. Let's see what the others think. |
|
I think it's reasonable for users to be able to customise the labels and annotations on the Secret created by the Access Operator, in the same way that they can for the other Strimzi created resources. I don't understand the usecase here for being able to replicate, but generally I can see users wanting to have set labels or annotations across their Strimzi resources. I'll add this as a discussion to the next Community call to see what others think and get consensus. |
This PR adds support for customizing the metadata (annotations and labels) of Secrets generated by the Kafka Access operator. We are using Mittwald secret replicator to replicate secrets to other namespaces and requires annotation in order for it to work.