Skip to content
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

Add option to provide API Group for customizable PKI #966

Merged
merged 11 commits into from
May 6, 2023

Conversation

shubhamcoc
Copy link
Contributor

@shubhamcoc shubhamcoc commented May 2, 2023

Description

Ability to add API group for the customizable PKI provided by the user.

Type of Change

Checklist

@shubhamcoc shubhamcoc requested a review from a team as a code owner May 2, 2023 17:48
@panyuenlau
Copy link
Member

Quick question @shubhamcoc - have you tested it with cert-manager in an actual k8s cluster?

Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM at a first glance, I will wait for manual test verification before merging

@shubhamcoc
Copy link
Contributor Author

Hi @panyuenlau, @pregnor Do you guys mean testing in kind cluster or we need some kind of customized cert manager for testing?

@panyuenlau
Copy link
Member

Hi @panyuenlau, @pregnor Do you guys mean testing in kind cluster or we need some kind of customized cert manager for testing?

What I mean by testing is to:

  1. Follow the quick start to bring up a running Kafka cluster, and you would need to build and use your customized image when you install the koeprator, for example:
helm install kafka-operator --repo https://kubernetes-charts.banzaicloud.com kafka-operator --namespace=kafka --create-namespace --set operator.image.repository=<path-to-your-repository>/<your-image-name> --set operator.image.tag=<your-image-tag>
  1. You would also need to install cert manager to the k8s cluster, one way of doing that is
kubectl apply \
--validate=false \
-f https://github.com/jetstack/cert-manager/releases/download/v1.11.0/cert-manager.crds.yaml
  1. And lastly you need to create a KafkaUser with the group info to verify the behavior that you updated in the PR:
apiVersion: kafka.banzaicloud.io/v1alpha1
kind: KafkaUser
metadata:
  name: example-kafkauser-with-pki
  namespace: kafka
spec:
  clusterRef:
    name: kafka
  secretName: example-kafkauser-secret
  pkiBackendSpec:
    pkiBackend: "cert-manager"
    issuerRef:
      name: "ca-issuer"
      kind: "ClusterIssuer"
      group: <supported-group-by-cert-manager>

A Certificate CR should be created successfully by cert-manager after this

@shubhamcoc
Copy link
Contributor Author

Hi @pregnor, @panyuenlau I have tested it and it was able to create the user certificate with name example-kafkauser-with-pki.

@panyuenlau
Copy link
Member

panyuenlau commented May 5, 2023

I also verified with

kubectl apply -f -<<EOF
apiVersion: kafka.banzaicloud.io/v1alpha1
kind: KafkaUser
metadata:
  name: example-kafkauser
  namespace: kafka
spec:
  clusterRef:
    name: kafka
  secretName: example-kafkauser-secret
  pkiBackendSpec:
    pkiBackend: "cert-manager"
    issuerRef:
      name: "kafka-kafka-issuer"
      kind: "ClusterIssuer"
      group: cert-manager.io
EOF

Where kafka-kafka-issuer being the cluster issuer:

kubectl get clusterissuer kafka-kafka-issuer -o yaml                                                                                                                     1 ↵
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  creationTimestamp: "2023-05-05T18:57:53Z"
  generation: 1
  labels:
    app: kafka
    kafka_issuer: kafka-kafka-issuer
  name: kafka-kafka-issuer
  resourceVersion: "41574"
  uid: 4335bbf3-0292-4519-bfc0-ad54eb5003e6
spec:
  ca:
    secretName: kafka-ca-certificate
status:
  conditions:
  - lastTransitionTime: "2023-05-05T19:18:58Z"
    message: Signing CA verified
    observedGeneration: 1
    reason: KeyPairVerified
    status: "True"
    type: Ready

The resulting Certificate is following:

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  creationTimestamp: "2023-05-05T19:27:35Z"
  generation: 1
  name: example-kafkauser
  namespace: kafka
  ownerReferences:
  - apiVersion: kafka.banzaicloud.io/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: KafkaUser
    name: example-kafkauser
    uid: 0f6ebfb6-7a53-4a64-9478-801bb5763530
  resourceVersion: "44159"
  uid: 9badeede-bf89-45f3-9dc4-34801c71bac3
spec:
  commonName: example-kafkauser
  issuerRef:
    group: cert-manager.io
    kind: ClusterIssuer
    name: kafka-kafka-issuer
  privateKey:
    encoding: PKCS8
  secretName: example-kafkauser-secret
  uris:
  - spiffe://cluster.local/ns/kafka/kafkauser/example-kafkauser
  usages:
  - client auth
  - server auth
status:
  conditions:
  - lastTransitionTime: "2023-05-05T19:27:35Z"
    message: Certificate is up to date and has not expired
    observedGeneration: 1
    reason: Ready
    status: "True"
    type: Ready
  notAfter: "2023-08-03T19:27:35Z"
  notBefore: "2023-05-05T19:27:35Z"
  renewalTime: "2023-07-04T19:27:35Z"
  revision: 1

Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I also see that improvements of the relevant tests can be made in

func TestReconcileUserCertificate(t *testing.T) {

But I think I will open another PR for improving the existing tests once I get a chance

@@ -182,7 +183,7 @@ func (c *certManager) clusterCertificateForUser(
}

// getCA returns the CA name/kind for the KafkaCluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should modify the description to name/kind/group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants