Adding test cases for kafka user issuerRef group #967

shubhamcoc · 2023-05-08T18:34:51Z

Description

Adding test cases for kafka user issuerRef group and rename of related functions

Type of Change

New Feature (Supporting external issuers with custom API groups for cert-manager dependency #911 )

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
All new and existing tests pass

panyuenlau

LGTM in general, it'd be great add the missing test cases to the unit tests

panyuenlau · 2023-05-09T20:33:23Z

pkg/util/pki/pki_common_test.go

+			Kind:  "testKind",
+			Group: "testGroup",
+		},
+		PKIBackend: v1beta1.PKIBackendCertManager,


I think we should also test the case when PKIBackend is empty

panyuenlau · 2023-05-09T20:33:45Z

pkg/util/pki/pki_common_test.go

+			Kind:  "testKind",
+			Group: "testGroup",
+		},
+		PKIBackend: v1beta1.PKIBackendCertManager,


Same as above, we should add a test case when PKIBackend is empty

shubhamcoc · 2023-05-10T07:02:29Z

pkg/pki/certmanagerpki/certmanager_pki.go

@@ -121,9 +121,9 @@ func userProvidedIssuerPKI(cluster *v1beta1.KafkaCluster, extListenerStatuses ma
 	// No need to generate self-signed certs and issuers because the issuer is provided by user
 	return []runtime.Object{
 		// Broker "user"
-		pkicommon.BrokerUserForCluster(cluster, extListenerStatuses),
+		pkicommon.BrokerUserForClusterWithPKIBackendSpec(cluster, extListenerStatuses),


I realized, these changes are not required, as clusterCertificateForUser() func will take care if the issuer is provided in cluster.Spec.ListenersConfig.SSLSecrets.IssuerRef. Can @panyuenlau, @pregnor confirm it?

The two functions that you are modifying here are to create KafkaUser CRs for the broker and the operator (these are fake "users" used within the Koperator to ensure its functionality when the Kafka cluster is configured to enable ssl).

When you create a KafkaUser CR that is used by an actual Kafka client, the Koperator would first check if the Certificate for all the KafkaUser (including the those two "fake" KafkaUser) exist, and it only calls clusterCertificateForUser() when the corresponding Certificate are not present, therefore those Certificate for the "fake" KafkaUser are not going to be reconciled

Therefore, the clusterCertificateForUser() is not going to help us take care of the issuerRef information

bartam1 · 2023-05-10T14:08:50Z

pkg/util/pki/common.go

+			PKIBackendSpec: &v1alpha1.PKIBackendSpec{
+				// Not checking IssuerRef for nil as it is checked in caller function kafkapki
+				IssuerRef:  sslConfig.IssuerRef,
+				PKIBackend: pkiBackend,


Currently this can be cert-manager only so this modification useless.
We need to extend the k8s api validation here: https://github.com/shubhamcoc/koperator/blob/2c2621944be96aa162024aab581807d4b3f56967/api/v1beta1/kafkacluster_types.go#L491
It need to be "k8s-csr,cert-manager"
Do we really want to this?
If yes then we have to test that case when pkiBackend is k8s-csr in the kafkacluster.spec

We should rename userProvidedPKI function (

koperator/pkg/pki/certmanagerpki/certmanager_pki.go

Line 117 in 84c4643

return userProvidedPKI(ctx, c.client, c.cluster, extListenerStatuses)

) to userProvidedCAforPKICertManager

We should rename the userProvidedIssuerPKI -> userProvidedPKIBackend

We should rename the fullPKI -> generatedCAForPKICertManager

I can do the test after the fixes.

I did the changes, but since we are changing in the api pkg, we might need to update it in go.mod to use local api.

We need to create a separate PR for the API change and we need a new tag for the API when it is merged to the master branch. This new tag for the api should be used in this PR in go.mod.

You can use Go workspaces for local development before putting up the final PRs for the different modules.

updated the go.mod file

If my understanding is correct (see here), believe the original request is to ask for support to customize the group field with cert-manager, instead of asking support for k8s-csr - which is good to have tho but I don't think supporting k8s-csr is what this PR is aiming for

Yes @panyuenlau you are right, but if the issuerRef is present with create option as true, do we create kafka-server or kafka-client certificates??

pregnor · 2023-05-12T10:57:42Z

Pushed the api/v0.27.0 tag that contains #970.

bartam1 · 2023-05-15T10:11:00Z

I looked into deeper today and this will be not good. To support the K8S-CSR will be a bigger change and we should not do this in this PR.
For k8s-csr support we need to do:

Refactor a bit the current SSLSecrets's properties. We will have k8s-csr so it will more complex. (TODO comment:

koperator/api/v1beta1/kafkacluster_types.go

Line 495 in 84c4643

// TODO (tinyzimmer): The above are all optional now in one way or another.

)
(We should rename the SSLSecret property to SSLConfiguration but this would be breaking change. Probably we should create separate property for cert manager configuration like: SSLConfiguration.CertManager and SSLConfiguration.K8SCSR )
We need to introduce the SSLSecret.signerName property for k8s-csr (IssuerRef is only for cert-manager)
We need to put the kafkaUser generation here:

koperator/pkg/pki/k8scsrpki/k8scsr_pki.go

Line 27 in 84c4643

logger.Info("k8sCSR PKI reconcile is skipped since it is not supported yet for server certs")

as for cert-manager (https://github.com/shubhamcoc/koperator/blob/3a6979a6cedf666e05cfcf8be6c629b52dd6b3c1/pkg/pki/certmanagerpki/certmanager_pki.go#L95). This is necessary because when k8s-csr is selected then that reconcilePKI will be called from here:

koperator/pkg/resources/kafka/kafka.go

Line 231 in 84c4643

if err := pki.GetPKIManager(r.Client, r.KafkaCluster, v1beta1.PKIBackendProvided).ReconcilePKI(ctx, extListenerStatuses); err != nil {
We need to create test cases for k8s-csr use-case.
We need to create documentation about this change into the API and into https://github.com/banzaicloud/koperator-docs
We need to add KafkaCluster samples for using k8s-csr into https://github.com/banzaicloud/koperator/tree/master/config/samples

We should support the usage of the Kubernetes CertificateSigningRequests when creating server certificates but we should do this with more caution and planning.

Dear @shubhamcoc!

The community would be happy for a PR to support k8s-csr. In case when you creating this PR we will help you.

Regarding the current PR please:

please revert the API to v0.26.0
rename userProvidedPKIBackend -> userProvidedIssuerforPKICertManager
remove BrokerUserForClusterWithPKIBackendSpec and ControllerUserForClusterWithPKIBackendSpec

back to your question:
"I realized, these changes are not required, as clusterCertificateForUser() func will take care if the issuer is provided in cluster.Spec.ListenersConfig.SSLSecrets.IssuerRef. Can @panyuenlau, @pregnor confirm it?"
Answer: It is true they are not required. Here we can see (

koperator/pkg/pki/certmanagerpki/certmanager_user.go

Line 191 in 84c4643

issuerRef = c.cluster.Spec.ListenersConfig.SSLSecrets.IssuerRef

) koperator uses the issuerRef from the kafkacluster.spec.sslsecrets for the brokerUser (

koperator/pkg/util/pki/common.go

Line 192 in 84c4643

return &v1alpha1.KafkaUser{

) because the user.Spec.PKIBackendSpec == nil in this case.

shubhamcoc · 2023-05-16T06:54:48Z

@bartam1 I have modified the code as per your comments. Kindly review it.

bartam1

LGTM! Thank you!

pregnor

LGTM

pregnor · 2023-05-19T08:40:56Z

Mergable is stuck a bit, I'm trying to get it unstuck so I could merge the PR.

Edit: now it did the trick...

…herrypick-from-banzai-master * banzai/master: Add new CruiseControlTaskOperation to represent Status Cruise Control Operation (banzaicloud#975) Use permanent Slack link in README (banzaicloud#985) update cert-manager dependency libraries to 1.11.2 (banzaicloud#981) Adding test cases for kafka user issuerRef group (banzaicloud#967) fix(cc): re-creation of CC metrics topic (banzaicloud#976) Revert "Enabling k8s-csr for PKIbackend in kafka cluster spec (banzaicloud#970)" (banzaicloud#972) Using gomock to mock sigs.k8s.io/controller-runtime/pkg/client If (banzaicloud#973) # Conflicts: # go.mod # go.sum # pkg/resources/kafka/kafka_test.go

* Adding Affinity for Cruise Control * removing pkg/sdk * Adding api changes for tag * Adding Affinity in Cruise Control implementation * Adding IT for CC affinity * fixing IT for CC affinity * Adding value comparision in CC IT * Adding option to specify group name in kafka cluster * Fixing PKIBackend validation in kafka cluster * using api 0.27.0 tag * fixing review comments --------- Co-authored-by: Darren Lau <panyuenlau@Gmail.com> Co-authored-by: Patrik Egyed <8093632+pregnor@users.noreply.github.com>

shubhamcoc added 10 commits April 20, 2023 20:52

Adding Affinity for Cruise Control

7f86c5e

removing pkg/sdk

9303dcc

Adding api changes for tag

5376464

Merge branch 'banzaicloud:master' into master

a129df0

Adding Affinity in Cruise Control implementation

7d3fa91

Adding IT for CC affinity

f8e87d9

fixing IT for CC affinity

320dbf9

Adding value comparision in CC IT

209085e

Merge branch 'banzaicloud:master' into master

ec7df12

Merge branch 'banzaicloud:master' into master

7cd44ac

shubhamcoc requested a review from a team as a code owner May 8, 2023 18:34

shubhamcoc force-pushed the kafka-cluster-custom-issuer branch from 7bdcffe to 2c26219 Compare May 8, 2023 18:43

panyuenlau reviewed May 9, 2023

View reviewed changes

shubhamcoc commented May 10, 2023

View reviewed changes

bartam1 requested changes May 10, 2023

View reviewed changes

shubhamcoc added 4 commits May 12, 2023 18:10

Merge branch 'banzaicloud:master' into master

2a9dbda

Adding option to specify group name in kafka cluster

0a7e7ef

Fixing PKIBackend validation in kafka cluster

b9c7dd7

using api 0.27.0 tag

3a6979a

shubhamcoc force-pushed the kafka-cluster-custom-issuer branch from 9666c07 to 3a6979a Compare May 12, 2023 12:45

panyuenlau mentioned this pull request May 12, 2023

Supporting external issuers with custom API groups for cert-manager dependency #911

Closed

fixing review comments

96e235d

shubhamcoc changed the title ~~Kafka cluster custom issuer group name~~ Adding test cases for kafka user issuerRef group May 16, 2023

panyuenlau approved these changes May 16, 2023

View reviewed changes

Merge branch 'master' into kafka-cluster-custom-issuer

31976ca

bartam1 approved these changes May 17, 2023

View reviewed changes

pregnor approved these changes May 19, 2023

View reviewed changes

Merge branch 'master' into kafka-cluster-custom-issuer

3af72f4

Merge branch 'master' into kafka-cluster-custom-issuer

f658564

pregnor merged commit a64633a into banzaicloud:master May 19, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding test cases for kafka user issuerRef group #967

Adding test cases for kafka user issuerRef group #967

shubhamcoc commented May 8, 2023 •

edited by pregnor

panyuenlau left a comment

panyuenlau May 9, 2023

panyuenlau May 9, 2023

shubhamcoc May 10, 2023 •

edited

panyuenlau May 12, 2023

bartam1 May 10, 2023 •

edited

bartam1 May 10, 2023

shubhamcoc May 10, 2023

bartam1 May 11, 2023 •

edited

pregnor May 12, 2023

shubhamcoc May 12, 2023

panyuenlau May 12, 2023

shubhamcoc May 15, 2023

pregnor commented May 12, 2023

bartam1 commented May 15, 2023 •

edited

shubhamcoc commented May 16, 2023

bartam1 left a comment

pregnor left a comment

pregnor commented May 19, 2023 •

edited

Adding test cases for kafka user issuerRef group #967

Adding test cases for kafka user issuerRef group #967

Conversation

shubhamcoc commented May 8, 2023 • edited by pregnor

Description

Type of Change

Checklist

panyuenlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhamcoc May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartam1 May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartam1 May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pregnor commented May 12, 2023

bartam1 commented May 15, 2023 • edited

shubhamcoc commented May 16, 2023

bartam1 left a comment

Choose a reason for hiding this comment

pregnor left a comment

Choose a reason for hiding this comment

pregnor commented May 19, 2023 • edited

shubhamcoc commented May 8, 2023 •

edited by pregnor

shubhamcoc May 10, 2023 •

edited

bartam1 May 10, 2023 •

edited

bartam1 May 11, 2023 •

edited

bartam1 commented May 15, 2023 •

edited

pregnor commented May 19, 2023 •

edited