Standardize the naming of karmada config in Karmada Operator #6082

seanlaii · 2025-01-24T05:09:27Z

What type of PR is this?
/kind feature
/kind cleanup

What this PR does / why we need it:
In karmada, here are two important secrets, which is mount by most karmada components. One is karmada-cert, which contains a series of cert files like ca.crt, apiserver.crt and so on; another is karmada-kubeconfig, which contains a kubeconfig of karmada-apiserver.

However, in different installation methods, we used inconsistent secret naming or file path naming, which can potentially cause some unnecessary problems, detail refer to #5363.

This PR aims to standardize the naming of karmada config in Karmada Operator installation method.

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-operator`: standardize the naming of karmada config in karmada-operator installation method

codecov-commenter · 2025-01-24T05:23:54Z

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

Codecov Report

Attention: Patch coverage is 40.78947% with 45 lines in your changes missing coverage. Please review.

Project coverage is 48.33%. Comparing base (d24b2b9) to head (852a022).

Files with missing lines	Patch %	Lines
operator/pkg/tasks/init/upload.go	0.00%	40 Missing ⚠️
operator/pkg/tasks/deinit/kubeconfig.go	88.46%	2 Missing and 1 partial ⚠️
operator/pkg/util/naming.go	0.00%	2 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6082      +/-   ##
==========================================
- Coverage   48.35%   48.33%   -0.02%     
==========================================
  Files         666      666              
  Lines       54880    54928      +48     
==========================================
+ Hits        26537    26552      +15     
- Misses      26618    26650      +32     
- Partials     1725     1726       +1

Flag	Coverage Δ
unittests	`48.33% <40.78%> (-0.02%)`	⬇️

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.

seanlaii · 2025-01-24T06:30:56Z

Hi @chaosi-zju , I found that in karmada-operator, all the components reference the same secret.
Do we plan to change this behavior to create secrets for each component?
If that's the case, maybe I could create multiple secrets in https://github.com/karmada-io/karmada/blob/master/operator/pkg/tasks/init/upload.go#L108-L115 by checking if the components exist from data.Components()?

chaosi-zju · 2025-01-24T06:54:47Z

Do we plan to change this behavior to create secrets for each component?

Yes, that is also our current purpose, we aims to create different secret for each component in format of ${karmada_instance_name}-${component}-config

just like:

karmada-aggregated-apiserver-config
karmada-controller-manager-config
karmada-scheduler-config
karmada-descheduler-config
...

besides, sorry I missed a point, the content of it should be like:

apiVersion: v1
kind: Secret
metadata:
  name: ${karmada_instance_name}-${component}-config
  namespace: karmada-system
stringData:
  karmada.config: |-
    xxxx.....

pay attention to the sub field karmada.config (In the past, the subfields varied across different installation methods, and we hope to standardize them.)

Signed-off-by: wei-chenglai <[email protected]>

karmada-bot · 2025-01-25T23:00:59Z

[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 liangyuanpeng, poor12 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:

hack/OWNERS
operator/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

seanlaii · 2025-01-25T23:19:48Z

Hi @chaosi-zju , please help review the PR when you are available. Thank you!

seanlaii · 2025-01-25T23:20:17Z

operator/pkg/tasks/init/upload.go

+			Name:      name,
+			Labels:    constants.KarmadaOperatorLabel,
+		},
+		StringData: map[string]string{"karmada.config": configString},


Change to StringData from Data.

seanlaii · 2025-01-25T23:20:49Z

operator/pkg/tasks/init/upload.go

+func generateComponentKubeconfigSecrets(data InitData, configString string) []*corev1.Secret {
+	var secrets []*corev1.Secret
+
+	secrets = append(secrets, generateKubeconfigSecret(util.AdminKubeconfigSecretName(data.GetName()), data.GetNamespace(), configString))


Still preserve admin-config for https://github.com/karmada-io/karmada/blob/master/operator/pkg/controller/karmada/planner.go#L162-L172.

Let me understand your intention:

Unlike local up or karmadactl, which allow for direct export of Karmada config to the file system of host machine for user access, the Karmada config generated by the operator can only be stored as secret in cluster, which requires users to manually export it.

As a result, it is essential to retain this admin config for user access?

it is reasonable

BTW, in that case, our related website doc which teach users how to export karmada config may also need updation

seanlaii · 2025-01-25T23:22:07Z

operator/pkg/tasks/deinit/kubeconfig.go

+func generateComponentKubeconfigSecretNames(data DeInitData) []string {
+	secretNames := []string{
+		util.AdminKubeconfigSecretName(data.GetName()),
+		util.ComponentKubeconfigSecretName(util.KarmadaAggregatedAPIServerName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KarmadaControllerManagerName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KubeControllerManagerName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KarmadaSchedulerName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KarmadaDeschedulerName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KarmadaMetricsAdapterName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KarmadaSearchName(data.GetName())),
+		util.ComponentKubeconfigSecretName(util.KarmadaWebhookName(data.GetName())),
+	}
+
+	return secretNames
+}


Return all potential secrets that need to be cleaned up. DeleteSecretIfHasLabels would ignore the secret if not found.

chaosi-zju · 2025-01-26T09:07:18Z

operator/pkg/controlplane/apiserver/manifests.go

+        - mountPath: /etc/karmada/config
+          name: karmada-config


just nits, it is better like this:

Suggested change

- mountPath: /etc/karmada/config

name: karmada-config

- name: karmada-config

mountPath: /etc/karmada/config

chaosi-zju · 2025-01-26T09:16:57Z

operator/pkg/tasks/deinit/kubeconfig.go

@@ -44,15 +44,35 @@ func runCleanupKubeconfig(r workflow.RunData) error {

 	klog.V(4).InfoS("[cleanup-kubeconfig] Running cleanup-kubeconfig task", "karmada", klog.KObj(data))


Suggested change

klog.V(4).InfoS("[cleanup-kubeconfig] Running cleanup-kubeconfig task", "karmada", klog.KObj(data))

klog.V(4).InfoS("[cleanup-karmada-config] Running cleanup-karmada-config task", "karmada", klog.KObj(data))

the above line and function name may also need a modification

chaosi-zju · 2025-01-26T09:23:06Z

operator/pkg/tasks/deinit/kubeconfig.go

+			constants.KarmadaOperatorLabel,
+		)
+		if err != nil {
+			return fmt.Errorf("failed to cleanup karmada kubeconfig secret '%s', err: %w", secretName, err)


according to RainbowMango, we prefer using 'karmada config' instead of 'karmada kubeconfig'
it is better to eliminate the term kubeconfig when using karmada config.

chaosi-zju · 2025-01-26T09:47:20Z

operator/pkg/util/naming.go

+// ComponentKubeconfigSecretName returns secret name of karmada component kubeconfig
+func ComponentKubeconfigSecretName(karmadaComponent string) string {
+	return generateResourceName(karmadaComponent, "config")
+}


shall we directly name it as ${karmadaComponent}-config?

Other installation methods do not have logic inner generateResourceName

In your case, like util.ComponentKubeconfigSecretName(util.KarmadaSearchName(data.GetName())), the parameter karmadaComponent actually always has prefix karmada, the logic inner generateResourceName is redundant

karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jan 24, 2025

karmada-bot requested review from chaosi-zju and Poor12 January 24, 2025 05:09

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 24, 2025

seanlaii changed the title ~~Standardize the naming of karmada config in Karmada Operator~~ [WIP] Standardize the naming of karmada config in Karmada Operator Jan 24, 2025

karmada-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2025

chaosi-zju mentioned this pull request Jan 24, 2025

[Umbrella] [Karmada config && certificates] secret and path naming convention #6051

Open

12 tasks

seanlaii force-pushed the operator branch from d74cbad to 9f32abd Compare January 25, 2025 19:26

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 25, 2025

seanlaii force-pushed the operator branch from 9f32abd to 8acb959 Compare January 25, 2025 19:33

Standardize the naming of karmada config in Karmada Operator

852a022

Signed-off-by: wei-chenglai <[email protected]>

seanlaii force-pushed the operator branch from 8acb959 to 852a022 Compare January 25, 2025 23:00

seanlaii changed the title ~~[WIP] Standardize the naming of karmada config in Karmada Operator~~ Standardize the naming of karmada config in Karmada Operator Jan 25, 2025

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 Jan 25, 2025

seanlaii commented Jan 25, 2025

View reviewed changes

chaosi-zju reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize the naming of karmada config in Karmada Operator #6082

Standardize the naming of karmada config in Karmada Operator #6082

seanlaii commented Jan 24, 2025

codecov-commenter commented Jan 24, 2025 •

edited

Loading

seanlaii commented Jan 24, 2025

chaosi-zju commented Jan 24, 2025 •

edited

Loading

karmada-bot commented Jan 25, 2025

seanlaii commented Jan 25, 2025

seanlaii Jan 25, 2025

seanlaii Jan 25, 2025

chaosi-zju Jan 26, 2025

chaosi-zju Jan 26, 2025

seanlaii Jan 25, 2025

chaosi-zju Jan 26, 2025

chaosi-zju Jan 26, 2025

chaosi-zju Jan 26, 2025

chaosi-zju Jan 26, 2025 •

edited

Loading

chaosi-zju Jan 26, 2025

		@@ -44,15 +44,35 @@ func runCleanupKubeconfig(r workflow.RunData) error {

		klog.V(4).InfoS("[cleanup-kubeconfig] Running cleanup-kubeconfig task", "karmada", klog.KObj(data))

	klog.V(4).InfoS("[cleanup-kubeconfig] Running cleanup-kubeconfig task", "karmada", klog.KObj(data))
	klog.V(4).InfoS("[cleanup-karmada-config] Running cleanup-karmada-config task", "karmada", klog.KObj(data))

Standardize the naming of karmada config in Karmada Operator #6082

Are you sure you want to change the base?

Standardize the naming of karmada config in Karmada Operator #6082

Conversation

seanlaii commented Jan 24, 2025

codecov-commenter commented Jan 24, 2025 • edited Loading

Codecov Report

seanlaii commented Jan 24, 2025

chaosi-zju commented Jan 24, 2025 • edited Loading

karmada-bot commented Jan 25, 2025

seanlaii commented Jan 25, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaosi-zju Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 24, 2025 •

edited

Loading

chaosi-zju commented Jan 24, 2025 •

edited

Loading

chaosi-zju Jan 26, 2025 •

edited

Loading