-
-
Notifications
You must be signed in to change notification settings - Fork 9
Added proposal for Strimzi integration with operator #86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
|
|
||
| ## Current situation | ||
|
|
||
| Currently, with the integration of automatic discovery of bootstrap address of a Strimzi Kafka cluster [feature](https://github.com/kroxylicious/kroxylicious/pull/2693), we are able to retrieve the bootstrap address of the plain listeners by using `strimziKafkaRef` section in the KafkaService, but currently it doesn't support `tls`. When dealing with tls protected Strimzi Kafka cluster, the user will still need to use the old approach of manually setting the bootstrap address and then using the `trustAnchorRef` field to configure trust for the Kafka cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make it clear here that it is only the case where Strimzi is signing the broker certificates itself using its own CA that is problematic.
| The current process is manual and any errors while configuring TLS in the Kroxylicious operator can result in the operator not working properly. | ||
|
|
||
| Adding this feature help us get rid of the manual configuration and would also increase the integration of the Strimzi ecosystem with Kroxylicious. | ||
| It will also open doors for closer integration with other operators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| It will also open doors for closer integration with other operators. |
I think the sentence reflects the general aspiration but this proposal does nothing concrete in that direction. I'd just remove the sentence.
|
|
||
| #### Process for looking up for certificate and key. | ||
|
|
||
| It is possible even when using Strimzi cluster, the users can provide certificate signed by real CA (Verisign etc), then we (as a client) don't need a trust anchor. In these case we should be using system trust anchor. Taking the trust anchor from Strimzi in this situation is harmful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd use the phrase 'public CA' rather than real CA.
why am I saying this
Often corporations create their own in-house CA and use it to sign certificates used by their internal systems. This is every bit a 'real CA', it is just a private in scope rather than public..
When designing software systems using TLS we need to keep in mind three use-cases
- the public CA
- the private/corporate CA
- and the self-signed use-case (for development, demos etc)
we already support 1 and 2. This proposal is supporting the 3rd.
|
|
||
| It is possible even when using Strimzi cluster, the users can provide certificate signed by real CA (Verisign etc), then we (as a client) don't need a trust anchor. In these case we should be using system trust anchor. Taking the trust anchor from Strimzi in this situation is harmful. | ||
|
|
||
| To tackle this issue, we can create a `trustStrimziCaCertificate` flag which will control whether we want to retrieve the trust anchor from Strimzi or we want the user to configure the trust anchor themselves. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add the trustStrimziCaCertificate flag to the API design you set out in point 1 and state is default value (false). The motivation for default false being a secure by default ethos.
|
|
||
| We will be making use of the below algorithm to discover the cert and key for the trust. | ||
|
|
||
| ```text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest using a table to set out the different scenarios. Enumerate the different permutations:
trustStrimziCaCertificatetrue|false,- spec.tls.trustAnchorRef provided or not
and set out what will result in the status.trustAnchorRef field in each case.
|
|
||
| #### What happens if the secret/key doesn't exist | ||
|
|
||
| In case the operator is not able to find the expected secret, then the operator would throw `ReferencedResourceNotReconciled` exception in the status of the `KafkaService` CR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally the operator will indeed use exceptions, but that's just implementation detail. What's important is the effect on the public API (the status section of the CR). So rephrase to talk about the conditions that will be added.
| In case the operator is not able to find the expected secret, then the operator would throw `ReferencedResourceNotReconciled` exception in the status of the `KafkaService` CR. | ||
|
|
||
| In case the key doesn't exist, then the operator would throw `InvalidReferencedResource` exception in the status of the `KafkaService` CR. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are missing a part saying that KafkaProxy reconciler needs to refer to the status.trustAnchorRef rather than the spec.
|
Thanks @ShubhamRwt, I've left a few comments. I'd suggest asking @katheris for a review too. |
|
@katheris Hi, Can you have a look at this proposal whenever you get some time? Thanks |
This proposal aims to extend the automatic discovery of bootstrap address feature to handle TLS listeners.