External `etcd` Support for Karmada Operator #5536

jabellard · 2024-09-12T22:11:45Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
This is an implementation of this proposal for adding support to the Karmada operator for external etcd cluster connections.

Which issue(s) this PR fixes:
Fixes #5242

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The new `SecretRef` field added as part of the configuration for connecting to an external etcd cluster can be 
used to reference a secret that contains credentials for connecting to an external etcd cluster.

karmada-bot · 2024-09-12T22:11:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign poor12, rainbowmango for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jabellard · 2024-09-12T22:21:01Z

Hey @RainbowMango . This is still a work in progress, but I'd really appreciate some early feedback from you and the team to ensure we're well aligned on the way forward for the implementation of this feature.

codecov-commenter · 2024-09-12T22:27:25Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 124 lines in your changes missing coverage. Please review.

Project coverage is 34.80%. Comparing base (4c8bcd4) to head (b36b3d8).
Report is 40 commits behind head on master.

Files with missing lines	Patch %	Lines
operator/pkg/controlplane/etcd/util.go	0.00%	45 Missing ⚠️
operator/pkg/controlplane/apiserver/apiserver.go	0.00%	30 Missing ⚠️
operator/pkg/tasks/deinit/component.go	0.00%	17 Missing ⚠️
operator/pkg/controlplane/search/search.go	0.00%	14 Missing ⚠️
operator/pkg/tasks/deinit/cert.go	0.00%	8 Missing ⚠️
operator/pkg/init.go	0.00%	6 Missing ⚠️
operator/pkg/deinit.go	0.00%	3 Missing ⚠️
operator/pkg/tasks/init/component.go	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5536      +/-   ##
==========================================
+ Coverage   34.14%   34.80%   +0.66%     
==========================================
  Files         643      646       +3     
  Lines       44524    44915     +391     
==========================================
+ Hits        15203    15634     +431     
+ Misses      28165    28074      -91     
- Partials     1156     1207      +51

Flag	Coverage Δ
unittests	`34.80% <0.00%> (+0.66%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

operator/pkg/controlplane/etcd/util.go

Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]> Onwards Signed-off-by: Joe Nathan Abellard <[email protected]>

Signed-off-by: Joe Nathan Abellard <[email protected]>

operator/pkg/apis/operator/v1alpha1/type.go

Signed-off-by: Joe Nathan Abellard <[email protected]>

jabellard · 2024-10-08T13:23:57Z

@RainbowMango , any thoughts?

Signed-off-by: Joe Nathan Abellard <[email protected]>

RainbowMango · 2024-10-15T09:24:46Z

operator/pkg/controller/karmada/controller.go

+// validateKarmada ensures the Karmada resource adheres to validation rules
+func (ctrl *Controller) validateKarmada(karmada *operatorv1alpha1.Karmada) error {
+	if karmada.Spec.Components.Etcd != nil && karmada.Spec.Components.Etcd.External != nil {
+		expectedSecretName := fmt.Sprintf("%s-%s", karmada.Name, constants.KarmadaOperatorEtcdClientCertNameSuffix)


Can you remind me of the benefit of using the karmada instance name(karma.name) as the prefix?

In my opinion, each karmada instance would run in a separate namespace, so it does not necessarily have the prefix.

Explore further.

RainbowMango · 2024-10-15T09:42:02Z

operator/pkg/controller/karmada/controller.go

+			newCondition := metav1.Condition{
+				Type:               string(operatorv1alpha1.Ready),
+				Status:             metav1.ConditionFalse,
+				Reason:             InvalidExternalEtcdClientSecretName,
+				Message:            errorMessage,
+				LastTransitionTime: metav1.Now(),
+			}
+			meta.SetStatusCondition(&karmada.Status.Conditions, newCondition)


I'd hesitate to introduce a condition for each concrete unexpected situation. My concern is we probably will have a lot of validations, we can't introduce a new condition for each validation.

I didn't expect to have condition from this PR, but thanks for me :) Maybe we can have a condition to represent the overall validation result, and we can put the error message to the Message if validation fails.

Will create just one generic validation reason and use the message to describe the error.

RainbowMango · 2024-10-15T09:58:43Z

operator/pkg/constants/constants.go

+	// EtcClientCredentialsVolumeName defines the name of the volume for the etcd client credentials
+	EtcClientCredentialsVolumeName = "etcd-client-credentials" // #nosec G101
+	// EtcClientCredentialsMountPath defines the mount path for the etcd client credentials data
+	EtcClientCredentialsMountPath = "/etc/etcd/pki" // #nosec G101


Suggested change

// EtcClientCredentialsVolumeName defines the name of the volume for the etcd client credentials

EtcClientCredentialsVolumeName = "etcd-client-credentials" // #nosec G101

// EtcClientCredentialsMountPath defines the mount path for the etcd client credentials data

EtcClientCredentialsMountPath = "/etc/etcd/pki" // #nosec G101

// EtcClientCredentialsVolumeName defines the name of the volume for the etcd client credentials

EtcClientCredentialsVolumeName = "etcd-client-cert" // #nosec G101

// EtcClientCredentialsMountPath defines the mount path for the etcd client credentials data

EtcClientCredentialsMountPath = "/etc/karmada/pki/etcd-client" // #nosec G101

As discussed with @chaosi-zju, we are going to standardize the volume name and mount path like this:

Volume: <server>-client-cert // e.g. etcd-client-cert

Mount path: /etc/karmada/pki/<server> // e.g. /etc/karmada/pki/etcd-client

File in container:

/etc/karmada/pki/<server>-client/ca.crt // e.g. /etc/karmada/pki/etcd-client/ca.crt

/etc/karmada/pki/<server>-client/tls.crt // e.g. /etc/karmada/pki/etcd-client/tls.crt

/etc/karmada/pki/<server>-client/tls.key // e.g. /etc/karmada/pki/etcd-client/tls.key

What do you think?

Will make the changes.

RainbowMango · 2024-10-18T07:56:43Z

Hi @jabellard, As discussed at the community meeting, we will have an alpha release for feature #5478.
And here it is: https://github.com/karmada-io/karmada/releases/tag/v1.12.0-alpha.1

jabellard · 2024-10-18T12:43:56Z

And here it is: https://github.com/karmada-io/karmada/releases/tag/v1.12.0-alpha.1

Thank you!

RainbowMango · 2024-10-24T01:24:08Z

@jabellard, please confirm whether we still need this PR.

jabellard · 2024-10-24T10:51:38Z

@jabellard, please confirm whether we still need this PR.

No longer needed. The other PRs covered the work initially introduced here.

karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 12, 2024

karmada-bot requested review from calvin0327, chaosi-zju and lonelyCZ September 12, 2024 22:11

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 12, 2024

jabellard changed the title ~~External etcd Support for Karmada Operator~~ External etcd Support for Karmada Operator Sep 12, 2024

chaosi-zju reviewed Sep 13, 2024

View reviewed changes

operator/pkg/controlplane/etcd/util.go Outdated Show resolved Hide resolved

jabellard force-pushed the external_etcd_support2 branch from d3ad4fa to 51b869b Compare September 13, 2024 20:40

jabellard marked this pull request as ready for review September 13, 2024 20:46

karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2024

karmada-bot requested review from a7i and chaosi-zju September 13, 2024 20:46

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Sep 14, 2024

jabellard added 4 commits September 14, 2024 10:06

Onwards

803ba24

Signed-off-by: Joe Nathan Abellard <[email protected]>

Onwards

280beaf

Signed-off-by: Joe Nathan Abellard <[email protected]>

Onwards

001b081

Signed-off-by: Joe Nathan Abellard <[email protected]>

jabellard force-pushed the external_etcd_support2 branch from f36f9a9 to 001b081 Compare September 14, 2024 14:07

karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Sep 14, 2024

RainbowMango reviewed Sep 23, 2024

View reviewed changes

operator/pkg/apis/operator/v1alpha1/type.go Outdated Show resolved Hide resolved

RainbowMango reviewed Sep 23, 2024

View reviewed changes

operator/pkg/apis/operator/v1alpha1/type.go Outdated Show resolved Hide resolved

operator/pkg/apis/operator/v1alpha1/type.go Show resolved Hide resolved

jabellard added 2 commits September 24, 2024 10:40

Address comments

b002ce6

Signed-off-by: Joe Nathan Abellard <[email protected]>

Address comments

b36b3d8

Signed-off-by: Joe Nathan Abellard <[email protected]>

RainbowMango mentioned this pull request Oct 10, 2024

Fix typo in operator file name #5658

Closed

Add validaton for Karmada CR

63245c7

Signed-off-by: Joe Nathan Abellard <[email protected]>

RainbowMango reviewed Oct 15, 2024

View reviewed changes

jabellard mentioned this pull request Oct 15, 2024

External etcd Support for Karmada Operator - Part 1 #5699

Merged

RainbowMango mentioned this pull request Oct 21, 2024

Add The Ability to Retrieve External Etcd Client Credentials From Secret #5242

Closed

jabellard closed this Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External `etcd` Support for Karmada Operator #5536

External `etcd` Support for Karmada Operator #5536

jabellard commented Sep 12, 2024

karmada-bot commented Sep 12, 2024

jabellard commented Sep 12, 2024

codecov-commenter commented Sep 12, 2024 •

edited

Loading

jabellard commented Oct 8, 2024

RainbowMango Oct 15, 2024

jabellard Oct 15, 2024

RainbowMango Oct 15, 2024

jabellard Oct 15, 2024

RainbowMango Oct 15, 2024

jabellard Oct 15, 2024

RainbowMango commented Oct 18, 2024

jabellard commented Oct 18, 2024

RainbowMango commented Oct 24, 2024

jabellard commented Oct 24, 2024

External etcd Support for Karmada Operator #5536

External etcd Support for Karmada Operator #5536

Conversation

jabellard commented Sep 12, 2024

karmada-bot commented Sep 12, 2024

jabellard commented Sep 12, 2024

codecov-commenter commented Sep 12, 2024 • edited Loading

Codecov Report

jabellard commented Oct 8, 2024

RainbowMango Oct 15, 2024

Choose a reason for hiding this comment

jabellard Oct 15, 2024

Choose a reason for hiding this comment

RainbowMango Oct 15, 2024

Choose a reason for hiding this comment

jabellard Oct 15, 2024

Choose a reason for hiding this comment

RainbowMango Oct 15, 2024

Choose a reason for hiding this comment

jabellard Oct 15, 2024

Choose a reason for hiding this comment

RainbowMango commented Oct 18, 2024

jabellard commented Oct 18, 2024

RainbowMango commented Oct 24, 2024

jabellard commented Oct 24, 2024

External `etcd` Support for Karmada Operator #5536

External `etcd` Support for Karmada Operator #5536

codecov-commenter commented Sep 12, 2024 •

edited

Loading