-
Notifications
You must be signed in to change notification settings - Fork 24
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
Issue certifcates for etcd-operator #9
base: main
Are you sure you want to change the base?
Conversation
@ArkaSaha30 please rebase this PR. I just merged #8 |
23af956
to
b177198
Compare
A couple of high level thoughts:
|
cc @hakman |
b177198
to
5e15ec4
Compare
3685a85
to
f5456d0
Compare
Sure, thank you! |
Please read #10 |
f5456d0
to
b5e5e38
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ArkaSaha30 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Updating the current progress here, making changes to this PR to accommodate certificates prefixed with etcd cluster names as per mentioned in the issue. |
@ArkaSaha30 : Please let me know if you need help in any tasks. Just to avoid overlap, I'll wait for you to confirm if I can pick up some of the tasks. Thanks. |
Sure, thank you! |
Sure, once you complete this, I will add that the code for secrets mounting in the same PR. |
b5e5e38
to
b5e719e
Compare
b5e719e
to
b59f6a1
Compare
Updated the PR to align with the certificate management design,
etcd-operator-controller-manager logs:
|
b59f6a1
to
699b561
Compare
It seems that you only create one secret (certificate & key pair) for each etcd cluster? We need to different certificate & key for each member. |
Also have you tested this PR in a K8s cluster? |
3be61a1
to
3f3fd00
Compare
Yes, I have been testing this on a kind cluster. The logs mentioned in comment are from a kind cluster. |
Yes, currently I am creating client, server and peer certificates for each etcd cluster. |
Different members should have different certificates. Specifically, each member should have at least 2 certificates, one used for etcdserver to validate client connections, the others also used for etcdserver to validate peer's connection. The etcd-operator just needs one certificate to connect to etcd PODs. |
Got it! At what stage does it make sense to create the member specific certificates? Once the members are ready or once the member creation request is submitted? |
We need to ensure the certificate is ready before creating each etcd member/POD. |
Just to clarify, the client and peer cert names should have the member/POD name prefixed/suffixed to it? |
3f3fd00
to
54eff56
Compare
As per the discussion in etcd-operator meeting, we need to create a certificate package to manage the certificates so that it is generic and future changes can be isolated to the package itself. |
5b15932
to
4ae9757
Compare
|
4ae9757
to
7404fea
Compare
0917113
to
0b38aa8
Compare
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.
Can you also add an at least an e2e test case?
4cb1b25
to
f3324d9
Compare
Signed-off-by: ArkaSaha30 <[email protected]>
f3324d9
to
80351c7
Compare
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 think we also need to add e2e test.
@@ -19,7 +20,7 @@ func NewProvider(pt ProviderType) (certInterface.Provider, error) { | |||
case Auto: | |||
return nil, nil // change me later | |||
case CertManager: | |||
return nil, nil // change me later | |||
return &certManager.CertManagerProvider{}, nil // change me later |
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.
Is there anything that can't be finalized yet?
return &certManager.CertManagerProvider{}, nil // change me later | |
return certManager.New(...), nil |
}, | ||
} | ||
|
||
createErr := cm.Create(ctx, certificateResource) |
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 seems that you always create it directly, what if the secret/certificate has already been created in previous invocation?
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 am checking if the certificate with the same name exists earlier
foundCert := &certmanagerv1.Certificate{}
foundErr := cm.Get(ctx, client.ObjectKey{Name: secretName, Namespace: namespace}, foundCert)
I should check for secret as well before invoking the create func, is that correct?
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.
Note the previous invocation may fail at any step, you need to follow the same pattern as the controller's reconciliation to reconcile it.
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.
Yes agree, I will update it
return false, fmt.Errorf("failed to get secret: %v", err) | ||
} | ||
|
||
certificateData, exists := secret.Data["tls.crt"] |
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 only check the certificate file, should we check the private key file as well?
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.
Yes, I agree. I will add that. Thank you!
This PR will add a basic capability to issue self-signed certificates for etcd-operator.
Prerequisite: cert-manager needs to be installed before deploying the etcd-operator
kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.16.2/cert-manager.yaml
Once etcd-operator is deployed it requests 3 certificates from an issuer in the
etcd-operator-system
namespace