Add reconciler for client, peer, server certificate #173
+392
−17
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[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 |
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
from
bea3c32 to
c980e01
Compare
|
/test pull-etcd-operator-test-e2e |
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
2 times, most recently
from
dd9729a to
fe533db
Compare
|
/test pull-etcd-operator-test-e2e |
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
2 times, most recently
from
3f702a6 to
eba6759
Compare
|
ahrtr
reviewed
Jul 7, 2025
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
from
e37260a to
5e902b6
Compare
ahrtr
reviewed
Jul 14, 2025
ahrtr
reviewed
Jul 14, 2025
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
from
1b79a53 to
6192aa2
Compare
ahrtr
reviewed
Jul 23, 2025
| } | ||
|
|
||
| func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client.Client, certName string) error { | ||
| cert, certErr := certificate.NewProvider(certificate.ProviderType(ec.Spec.TLS.Provider), c) |
There was a problem hiding this comment.
ensure provider matches the providerConfig.
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
2 times, most recently
from
829112a to
84bb9d8
Compare
ArkaSaha30
changed the title
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
5 times, most recently
from
3027592 to
1d32631
Compare
|
Hi @ArkaSaha30, we discussed this in our community meeting. We would want to have the e2e tests in this pull request too, before we consider it mergeable. Thanks. |
Yes, I'm working on the e2e tests. Will update the PR by EOW. |
This commit will add reconciler logic for creating - client certificate to communicate with the etcdCluster - server, peer certificate for each of the etcd member communication Also, mount certificate secrets to member pods Signed-off-by: ArkaSaha30 <[email protected]>
Signed-off-by: ArkaSaha30 <[email protected]>
ArkaSaha30
force-pushed
the
cert-manager-reconciler
branch
from
565b1a7 to
47b635b
Compare
|
Client, server, peer certificates and their respective secrets: Server and peer secret are mounted to each of the members: |
ahrtr
reviewed
Aug 14, 2025
| @@ -84,6 +91,17 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
| etcdCluster.Spec.ImageRegistry = r.ImageRegistry | |||
| } | |||
|
|
|||
| // Create Client Certificate for etcd-operator to communicate with the etcdCluster | |||
| if etcdCluster.Spec.TLS != nil { | |||
| clientCertErr := createClientCertificate(etcdCluster, ctx, r.Client) | |||
There was a problem hiding this comment.
minor comment: usually context is the first parameter for a function or method
Comment on lines
+217
to
+218
| serverCertName := fmt.Sprintf("%s-%s-tls", ec.Name, "server") | ||
| peerCertName := fmt.Sprintf("%s-%s-tls", ec.Name, "peer") |
There was a problem hiding this comment.
Please create two functions to generate/get the serverCertName and peerCertName, so that we have a centralized place to maintain the names.
Comment on lines
+585
to
+588
| ExtraConfig: map[string]any{ | ||
| "issuerName": cmConfig.IssuerName, | ||
| "issuerKind": cmConfig.IssuerKind, | ||
| }, |
There was a problem hiding this comment.
Two solutions:
- refer to the
IssuerNameKeyandIssuerNameKey - get the details of populating the
ExtraConfigencapsulated in the certificate package.
For simplicity, let's follow solution 1 for now, and revisit 2 in future if needed.
| } | ||
|
|
||
| func createClientCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client.Client) error { | ||
| certName := fmt.Sprintf("%s-%s-tls", ec.Name, "client") |
There was a problem hiding this comment.
create a function to get the secret for client
| return ctx | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Please also verify that we can access the etcdcluster via client certificate.
- write a key/value pair and read it back
ivanvc
reviewed
Aug 20, 2025
| @@ -54,6 +56,11 @@ type EtcdClusterReconciler struct { | |||
| // +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch;create;update;patch;delete | |||
| // +kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;list;watch;create;update;patch;delete | |||
| // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch;get;list;update | |||
| // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;patch;update;delete | |||
| // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch | |||
There was a problem hiding this comment.
Quick question: I see that we're querying the STS, and checking the podSpec from there, but I'm not sure why we would need to query the pod directly. Am I missing something? Or, is there a particular reason why you added this permission?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will implement the reconciler for client, peer and server certificate creation for the etcdCluster object and its respective members.
Fixes: #10