design:Add component certificate identification for karmada components #6269

tiansuo114 · 2025-04-07T05:28:33Z

What type of PR is this?
/kind design

What this PR does / why we need it:
This pr is about #6091

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

Special notes for your reviewer:
@XiShanYongYe-Chang
@zhzhuang-zju
Does this PR introduce a user-facing change?:

codecov-commenter · 2025-04-07T05:42:53Z

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.34%. Comparing base (ba5ffba) to head (e67ae00).
Report is 110 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6269      +/-   ##
==========================================
+ Coverage   47.95%   49.34%   +1.38%     
==========================================
  Files         676      678       +2     
  Lines       55964    55122     -842     
==========================================
+ Hits        26837    27199     +362     
+ Misses      27355    26154    -1201     
+ Partials     1772     1769       -3

Flag	Coverage Δ
unittests	`49.34% <ø> (+1.38%)`	⬆️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zhzhuang-zju · 2025-05-10T08:45:12Z

/assign

zhzhuang-zju · 2025-05-10T09:14:45Z

hack/deploy-karmada.sh

    for component in "${components[@]}"
    do
      generate_config_secret ${component} ${karmada_ca} ${CLIENT_CRT} ${CLIENT_KEY}
    done

+


Suggested change

Redundant blank lines

zhzhuang-zju · 2025-05-10T09:17:04Z

hack/deploy-karmada.sh

+
+

Suggested change

ditto

zhzhuang-zju · 2025-05-13T02:33:40Z

hack/deploy-karmada.sh

+util::create_certkey "" "${CERT_DIR}" "ca" karmada-webhook "system:karmada:karmada-webhook" "" "${karmada_webhook_alt_names[@]}"
+util::create_certkey "" "${CERT_DIR}" "ca" karmada-search "system:karmada:karmada-search" "" "${karmada_search_alt_names[@]}"
+util::create_certkey "" "${CERT_DIR}" "ca" karmada-metrics-adapter "system:karmada:karmada-metrics-adapter" "" "${karmada_metrics_adapter_alt_names[@]}"
+util::create_certkey "" "${CERT_DIR}" "ca" karmada-scheduler-estimator "system:karmada:karmada-scheduler-estimator" "" "${karmadaAltNames[@]}"


Why is the karmada-scheduler-estimator component still using karmadaAltNames?

The reason the karmada-scheduler-estimator component still uses karmadaAltNames (i.e., uses wildcards to specify DNS names in the certificate) is that during Karmada cluster deployment, the karmada-scheduler-estimator component's Deployment name might be something like karmada-scheduler-estimator-member1. However, in x509 certificates, wildcards can only appear in the leftmost complete label and cannot be used in formats like karmada-scheduler-estimator-*.karmada-system.svc.cluster.local. Therefore, to meet runtime requirements, we use karmadaAltNames to assign the DNS field of the certificate for the karmada-scheduler-estimator component.

Thanks for your explanation. That makes sense. However, I suggest creating a new variable, karmada_scheduler_estimator_alt_names, whose value can be set as needed.

zhzhuang-zju · 2025-05-13T02:40:53Z

hack/deploy-karmada.sh

@@ -187,13 +196,53 @@ util::cmd_must_exist "openssl"
 util::cmd_must_exist_cfssl ${CFSSL_VERSION}
 # create CA signers
 util::create_signing_certkey "" "${CERT_DIR}" ca karmada '"client auth","server auth"'
-# signs a certificate
+
+
 karmadaAltNames=("*.karmada-system.svc.cluster.local" "*.karmada-system.svc" "localhost" "127.0.0.1" $(util::get_apiserver_ip_from_kubeconfig "${HOST_CLUSTER_NAME}") "${interpreter_webhook_example_service_external_ip_address}")


It seems that ${interpreter_webhook_example_service_external_ip_address} has not been included in the SAN of the new certificates. cc @XiShanYongYe-Chang Do you know whether this SAN entry is necessary, and which component relies on it?

In the discussion, my understanding of the karmada-interpreter-webhook-example component was that it was still in a demo state and might undergo further changes, so I didn’t modify its certificate configuration at that time.

zhzhuang-zju · 2025-05-13T02:56:09Z

hack/deploy-karmada.sh


    # 3. generate secret with ca cert or sa key
    generate_ca_cert_secret kube-controller-manager ${karmada_ca} ${karmada_ca_key}
    generate_key_pair_secret kube-controller-manager ${SA_PUB} ${SA_KEY}
    generate_key_pair_secret karmada-apiserver ${SA_PUB} ${SA_KEY}

-    # 4. generate secret with karmada config
-    components=(karmada-aggregated-apiserver karmada-controller-manager kube-controller-manager karmada-scheduler karmada-descheduler karmada-metrics-adapter karmada-search karmada-webhook karmada-interpreter-webhook-example)
+    # 5. generate secret with karmada config for each component using their specific client certs


Suggested change

# 5. generate secret with karmada config for each component using their specific client certs

# 4. generate secret with karmada config for each component using their specific client certs

zhzhuang-zju · 2025-05-13T03:02:13Z

hack/deploy-karmada.sh

 SA_PUB=$(base64 < "${CERT_DIR}/sa.pub" | tr -d '\r\n')
 SA_KEY=$(base64 < "${CERT_DIR}/sa.key" | tr -d '\r\n')
+


Suggested change

The issue with extra spaces has been completely fixed. How does the file look now?

Thanks~ looks well

zhzhuang-zju · 2025-05-13T07:08:47Z

@tiansuo114 You can merge the commits. Others LGTM

zhzhuang-zju · 2025-05-13T07:48:44Z

ask @XiShanYongYe-Chang for another look

XiShanYongYe-Chang

Thanks all~
/approve

I have an idea that is not related to the current pr. The current file looks a little long and a little hard to read. There is no clear code block organization and step order. Do you have any ideas to improve it?

karmada-bot · 2025-05-13T09:00:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [XiShanYongYe-Chang]

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

…pt deploy Signed-off-by: tiansuo <zhaoyi_114@outlook.com>

tiansuo114 · 2025-05-13T12:20:43Z

Thanks all~ /approve

I have an idea that is not related to the current pr. The current file looks a little long and a little hard to read. There is no clear code block organization and step order. Do you have any ideas to improve it?

I think we can improve the readability of this file by adding some functions — for example, integrating all certificate generation logic into a single generate_all_certificates function. However, regarding the overall file length, I'm not sure if it's a good idea to put functions that are only used in deploy-karmada.sh into util.sh. I’d appreciate it if you could share your thoughts on this.

XiShanYongYe-Chang · 2025-05-13T12:26:21Z

Thanks @tiansuo114~

I haven't really thought about it, I just feel like the file is a little big and not very easy to read. (Of course, it has nothing to do with the current pr, which is the result of a long-term update iteration.) If you want and have time, you can try it.

zhzhuang-zju · 2025-05-13T12:32:31Z

I think we can improve the readability of this file by adding some functions — for example, integrating all certificate generation logic into a single generate_all_certificates function. However, regarding the overall file length, I'm not sure if it's a good idea to put functions that are only used in deploy-karmada.sh into util.sh. I’d appreciate it if you could share your thoughts on this.

Rather than adding more functions, I think the current issue affecting readability is the lack of a clear execution flow. It's not easy to understand what the script does exactly and in what order.

tiansuo114 · 2025-05-13T12:55:57Z

I think we can improve the readability of this file by adding some functions — for example, integrating all certificate generation logic into a single generate_all_certificates function. However, regarding the overall file length, I'm not sure if it's a good idea to put functions that are only used in deploy-karmada.sh into util.sh. I’d appreciate it if you could share your thoughts on this.

Rather than adding more functions, I think the current issue affecting readability is the lack of a clear execution flow. It's not easy to understand what the script does exactly and in what order.

Yes, what I mean is to refactor some of the current repeatable processes into separate functions, and reorganize the file into a structure like: variable declarations → logic execution → function definitions. This might make the file more readable.

zhzhuang-zju · 2025-05-14T01:18:49Z

/lgtm

karmada-bot added the kind/design Categorizes issue or PR as related to design. label Apr 7, 2025

karmada-bot requested review from lfbear and Poor12 April 7, 2025 05:28

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 7, 2025

tiansuo114 mentioned this pull request Apr 7, 2025

[lfx-mentorship-2025-March-May-Tracking-Issue] Karmada Self-Signed Certificate Content Standardization #6264

Open

tiansuo114 force-pushed the server_cert branch 2 times, most recently from 2144a6f to 0d996d1 Compare April 9, 2025 13:57

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 9, 2025

tiansuo114 force-pushed the server_cert branch from d5848d1 to b4ebfcf Compare April 10, 2025 01:15

tiansuo114 changed the title ~~design:Add component certificate identification for server components~~ design:Add component certificate identification for karmada components Apr 10, 2025

karmada-bot assigned zhzhuang-zju May 10, 2025

zhzhuang-zju reviewed May 13, 2025

View reviewed changes

XiShanYongYe-Chang approved these changes May 13, 2025

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2025

design:Add component certificate identification for karmada hack scri…

e67ae00

…pt deploy Signed-off-by: tiansuo <zhaoyi_114@outlook.com>

tiansuo114 force-pushed the server_cert branch from 2ef9fa7 to e67ae00 Compare May 13, 2025 11:57

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2025

karmada-bot merged commit a34c412 into karmada-io:master May 14, 2025
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

design:Add component certificate identification for karmada components #6269

design:Add component certificate identification for karmada components #6269

tiansuo114 commented Apr 7, 2025 •

edited

Loading

codecov-commenter commented Apr 7, 2025 •

edited

Loading

zhzhuang-zju commented May 10, 2025

zhzhuang-zju May 10, 2025

zhzhuang-zju May 10, 2025

zhzhuang-zju May 13, 2025

tiansuo114 May 13, 2025

zhzhuang-zju May 13, 2025

zhzhuang-zju May 13, 2025

tiansuo114 May 13, 2025

zhzhuang-zju May 13, 2025

zhzhuang-zju May 13, 2025

zhzhuang-zju May 13, 2025

tiansuo114 May 13, 2025

zhzhuang-zju May 13, 2025

zhzhuang-zju commented May 13, 2025

zhzhuang-zju commented May 13, 2025

XiShanYongYe-Chang left a comment

karmada-bot commented May 13, 2025

tiansuo114 commented May 13, 2025

XiShanYongYe-Chang commented May 13, 2025

zhzhuang-zju commented May 13, 2025

tiansuo114 commented May 13, 2025

zhzhuang-zju commented May 14, 2025

	# 5. generate secret with karmada config for each component using their specific client certs
	# 4. generate secret with karmada config for each component using their specific client certs

		SA_PUB=$(base64 < "${CERT_DIR}/sa.pub" \| tr -d '\r\n')
		SA_KEY=$(base64 < "${CERT_DIR}/sa.key" \| tr -d '\r\n')

design:Add component certificate identification for karmada components #6269

design:Add component certificate identification for karmada components #6269

Conversation

tiansuo114 commented Apr 7, 2025 • edited Loading

codecov-commenter commented Apr 7, 2025 • edited Loading

Codecov Report

zhzhuang-zju commented May 10, 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

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

zhzhuang-zju commented May 13, 2025

zhzhuang-zju commented May 13, 2025

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

karmada-bot commented May 13, 2025

tiansuo114 commented May 13, 2025

XiShanYongYe-Chang commented May 13, 2025

zhzhuang-zju commented May 13, 2025

tiansuo114 commented May 13, 2025

zhzhuang-zju commented May 14, 2025

tiansuo114 commented Apr 7, 2025 •

edited

Loading

codecov-commenter commented Apr 7, 2025 •

edited

Loading