feature(karmadactl): support new cert generate in init command split model #6810
Conversation
Signed-off-by: tiansuo <[email protected]>
Signed-off-by: tiansuo <[email protected]>
Signed-off-by: tiansuo <[email protected]>
Signed-off-by: tiansuo <[email protected]>
Signed-off-by: tiansuo <[email protected]>
karmada-bot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
karmada-bot
added
the
size/XXL
Summary of ChangesHello @tiansuo114, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the certificate generation and secret management within Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for a new certificate generation model in the karmadactl init command using a "split" secret layout. The feature allows generating certificates with unique CommonNames and Subject Alternative Names for each Karmada component when using the --secret-layout=split flag.
- Adds a new "split" certificate layout mode that creates per-component TLS secrets instead of a single aggregated secret
- Introduces comprehensive CommonName constants and certificate naming conventions for all Karmada components
- Extends the certificate generation system to support component-specific certificates with proper CA selection
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/karmadactl/cmdinit/options/global.go | Adds certificate name constants and CommonName definitions for all components |
| pkg/karmadactl/cmdinit/kubernetes/deployments.go | Implements split-layout volume mounting and command generation for deployments |
| pkg/karmadactl/cmdinit/kubernetes/statefulset.go | Adds split-layout support for etcd StatefulSet configuration |
| pkg/karmadactl/cmdinit/kubernetes/deploy.go | Core implementation of split certificate generation and secret creation |
| pkg/karmadactl/cmdinit/cert/cert.go | Adds NewGenCerts function for component-specific certificate generation |
| pkg/cert/constants.go | New file defining TLS secret names and key constants |
| pkg/karmadactl/cmdinit/cmdinit.go | Adds --secret-layout CLI flag |
| Test files | Comprehensive test coverage for the new split certificate functionality |
| CI/Scripts | Adds CI workflow and script for testing split secret layout |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| return []corev1.VolumeMount{ | ||
| {Name: karmadaConfigVolumeName, ReadOnly: true, MountPath: karmadaConfigVolumeMountPath}, | ||
| {Name: certconst.SecretWebhook, ReadOnly: true, MountPath: webhookCertVolumeMountPath}, |
There was a problem hiding this comment.
Volume name should match the secret name pattern. This line uses the secret name as the volume name, but it should use a consistent volume naming pattern like other volumes (e.g., 'webhook-cert' instead of 'karmada-webhook-cert').
Copilot uses AI. Check for mistakes.
| } | ||
| return []corev1.Volume{ | ||
| {Name: karmadaConfigVolumeName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: util.KarmadaConfigName(names.KarmadaWebhookComponentName)}}}, | ||
| {Name: certconst.SecretWebhook, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretWebhook}}}, |
There was a problem hiding this comment.
Volume name inconsistency. This should use a volume name constant (like 'serverCertVolumeName') instead of using the secret name directly as the volume name, to maintain consistency with the split layout pattern.
| {Name: certconst.SecretWebhook, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretWebhook}}}, | |
| {Name: serverCertVolumeName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretWebhook}}}, |
Copilot uses AI. Check for mistakes.
| }, | ||
| }, | ||
| } | ||
| Volumes = append(Volumes, secretVolume) |
There was a problem hiding this comment.
[nitpick] The conditional logic creates different volume structures but both branches append to the same Volumes slice. Consider restructuring to make the two different volume creation paths more explicit and easier to understand.
Copilot uses AI. Check for mistakes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature: a new "split" secret layout for certificate management in karmadactl init. This improves security and maintainability by providing per-component certificates and secrets. The changes are extensive, covering certificate generation, secret creation, and deployment configurations for various Karmada components. The addition of a dedicated test script and new unit tests is also a great step towards ensuring the quality of this new feature. My review focuses on improving code clarity, reducing duplication, and enhancing robustness in the new logic for the split layout. I've identified several areas where refactoring could make the code more maintainable and easier to understand.
| if strings.ToLower(i.SecretLayout) != secretLayoutSplit { | ||
| return command | ||
| } | ||
|
|
||
| // Split layout: reuse legacy YAML structure and command style, | ||
| // only swap certificate paths to split-mounted locations. | ||
| command = []string{ | ||
| "sh", | ||
| "-c", | ||
| fmt.Sprintf( | ||
| `set -ex | ||
| cat <<EOF | tee %s/%s | ||
| name: ${%s} | ||
| client-transport-security: | ||
| client-cert-auth: true | ||
| trusted-ca-file: %s/ca.crt | ||
| key-file: %s/tls.key | ||
| cert-file: %s/tls.crt | ||
| peer-transport-security: | ||
| client-cert-auth: false | ||
| trusted-ca-file: %s/ca.crt | ||
| key-file: %s/tls.key | ||
| cert-file: %s/tls.crt | ||
| initial-cluster-state: new | ||
| initial-cluster-token: etcd-cluster | ||
| initial-cluster: %s | ||
| listen-peer-urls: http://${%s}:%v | ||
| listen-client-urls: https://${%s}:%v,http://127.0.0.1:%v | ||
| initial-advertise-peer-urls: http://${%s}:%v | ||
| advertise-client-urls: https://${%s}.%s.%s.svc.%s:%v | ||
| data-dir: %s | ||
| cipher-suites: %s | ||
| `, | ||
| etcdContainerConfigDataMountPath, etcdConfigName, | ||
| etcdEnvPodName, | ||
| etcdClientCertVolumeMountPath, | ||
| serverCertVolumeMountPath, | ||
| serverCertVolumeMountPath, | ||
| etcdClientCertVolumeMountPath, | ||
| serverCertVolumeMountPath, | ||
| serverCertVolumeMountPath, | ||
| strings.TrimRight(etcdClusterConfig, ","), | ||
| etcdEnvPodIP, etcdContainerServerPort, | ||
| etcdEnvPodIP, etcdContainerClientPort, etcdContainerClientPort, | ||
| etcdEnvPodIP, etcdContainerServerPort, | ||
| etcdEnvPodName, etcdStatefulSetAndServiceName, | ||
| i.Namespace, i.HostClusterDomain, | ||
| etcdContainerClientPort, | ||
| etcdContainerDataVolumeMountPath, | ||
| etcdCipherSuites, | ||
| ), | ||
| } | ||
| return command | ||
| } |
There was a problem hiding this comment.
The logic for generating the etcd init container command is duplicated for the 'legacy' and 'split' layouts, with only minor differences in certificate paths. This large block of duplicated code is hard to maintain. Please refactor it to remove the duplication, for example by determining the paths based on the layout and then using a single command template.
| cd charts/karmada/ | ||
| cp -r _crds crds | ||
| tar -zcvf ../../crds.tar.gz crds | ||
| cd - |
There was a problem hiding this comment.
Using cd in a script can be risky. If any command within the cd block fails, the script might continue executing in an unexpected directory. It's safer to run these commands in a subshell to isolate the directory change.
| cd charts/karmada/ | |
| cp -r _crds crds | |
| tar -zcvf ../../crds.tar.gz crds | |
| cd - | |
| (cd charts/karmada/ && cp -r _crds crds && tar -zcvf ../../crds.tar.gz crds) | |
| options.KarmadaAPIServerEtcdClientCertAndKeyName: func(option *CommandInitOption) (cert, key []byte, err error) { | ||
| if cert, err = os.ReadFile(option.ExternalEtcdClientCertPath); err != nil { | ||
| return | ||
| } | ||
| key, err = os.ReadFile(option.ExternalEtcdClientKeyPath) | ||
| return | ||
| }, | ||
| options.KarmadaAggregatedAPIServerEtcdClientCertAndKeyName: func(option *CommandInitOption) (cert, key []byte, err error) { | ||
| if cert, err = os.ReadFile(option.ExternalEtcdClientCertPath); err != nil { | ||
| return | ||
| } | ||
| key, err = os.ReadFile(option.ExternalEtcdClientKeyPath) | ||
| return | ||
| }, | ||
| options.KarmadaSearchEtcdClientCertAndKeyName: func(option *CommandInitOption) (cert, key []byte, err error) { | ||
| if cert, err = os.ReadFile(option.ExternalEtcdClientCertPath); err != nil { | ||
| return | ||
| } | ||
| key, err = os.ReadFile(option.ExternalEtcdClientKeyPath) | ||
| return | ||
| }, |
There was a problem hiding this comment.
There's significant code duplication in the function literals for handling external etcd client certificates. The logic for KarmadaAPIServerEtcdClientCertAndKeyName, KarmadaAggregatedAPIServerEtcdClientCertAndKeyName, and KarmadaSearchEtcdClientCertAndKeyName is identical to the one for EtcdClientCertAndKeyName. This can be refactored to improve maintainability.
| if i.SecretLayout != secretLayoutSplit { | ||
| i.SecretLayout = secretLayoutLegacy | ||
| } |
There was a problem hiding this comment.
The validation for SecretLayout could be more robust and user-friendly. Currently, any value other than "split" defaults to "legacy", which might be confusing for users if they make a typo (e.g., "Split"). It would be better to perform a case-insensitive comparison and return an error for invalid values.
i.SecretLayout = strings.ToLower(i.SecretLayout)
if i.SecretLayout != secretLayoutSplit && i.SecretLayout != secretLayoutLegacy {
return fmt.Errorf("invalid --secret-layout value %q. Must be either %q or %q", i.SecretLayout, secretLayoutLegacy, secretLayoutSplit)
}| caCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", globaloptions.CaCertAndKeyName)]) | ||
| caKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", globaloptions.CaCertAndKeyName)]) | ||
| apiserverCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.KarmadaAPIServerCertAndKeyName)]) | ||
| apiserverKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.KarmadaAPIServerCertAndKeyName)]) | ||
| aggregatedCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.KarmadaAggregatedAPIServerCertAndKeyName)]) | ||
| aggregatedKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.KarmadaAggregatedAPIServerCertAndKeyName)]) | ||
| webhookServerCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.KarmadaWebhookCertAndKeyName)]) | ||
| webhookServerKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.KarmadaWebhookCertAndKeyName)]) | ||
| frontProxyCaCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.FrontProxyCaCertAndKeyName)]) | ||
| frontProxyClientCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.FrontProxyClientCertAndKeyName)]) | ||
| frontProxyClientKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.FrontProxyClientCertAndKeyName)]) | ||
| etcdCaCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.EtcdCaCertAndKeyName)]) | ||
| etcdServerCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.EtcdServerCertAndKeyName)]) | ||
| etcdServerKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.EtcdServerCertAndKeyName)]) | ||
| // Component-specific etcd client certs | ||
| etcdClientCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.EtcdClientCertAndKeyName)]) | ||
| etcdClientKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.EtcdClientCertAndKeyName)]) | ||
| apiserverEtcdClientCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.KarmadaAPIServerEtcdClientCertAndKeyName)]) | ||
| apiserverEtcdClientKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.KarmadaAPIServerEtcdClientCertAndKeyName)]) | ||
| aggregatedEtcdClientCrt := string(i.CertAndKeyFileData[fmt.Sprintf("%s.crt", options.KarmadaAggregatedAPIServerEtcdClientCertAndKeyName)]) | ||
| aggregatedEtcdClientKey := string(i.CertAndKeyFileData[fmt.Sprintf("%s.key", options.KarmadaAggregatedAPIServerEtcdClientCertAndKeyName)]) |
There was a problem hiding this comment.
There is a lot of repetitive code for reading certificate and key data from CertAndKeyFileData. This makes the code harder to read and maintain. Consider creating a helper function to encapsulate this logic.
| if err := util.CreateOrUpdateSecret(i.KubeClientSet, kcmCA); err != nil { | ||
| return err | ||
| } | ||
| kcmSA := i.SecretFromSpec(certconst.SecretKubeControllerManagerSAKeys, corev1.SecretTypeOpaque, map[string]string{certconst.KeySAPrivate: string(saPriv), certconst.KeySAPublic: string(saPub)}) |
There was a problem hiding this comment.
The saPriv and saPub variables are already strings when passed to this function. The string() conversions here are redundant and can be removed for better clarity.
kcmSA := i.SecretFromSpec(certconst.SecretKubeControllerManagerSAKeys, corev1.SecretTypeOpaque, map[string]string{certconst.KeySAPrivate: saPriv, certconst.KeySAPublic: saPub})| VolumeMounts: func() []corev1.VolumeMount { | ||
| if strings.ToLower(i.SecretLayout) == secretLayoutSplit { | ||
| return []corev1.VolumeMount{ | ||
| {Name: serverCertVolumeName, ReadOnly: true, MountPath: serverCertVolumeMountPath}, | ||
| {Name: etcdClientCertVolumeName, ReadOnly: true, MountPath: etcdClientCertVolumeMountPath}, | ||
| {Name: frontProxyClientCertVolumeName, ReadOnly: true, MountPath: frontProxyClientCertVolumeMountPath}, | ||
| {Name: saKeyPairVolumeName, ReadOnly: true, MountPath: saKeyPairVolumeMountPath}, | ||
| } | ||
| } | ||
| return []corev1.VolumeMount{{Name: globaloptions.KarmadaCertsName, ReadOnly: true, MountPath: karmadaCertsVolumeMountPath}} | ||
| }(), | ||
| LivenessProbe: livenessProbe, | ||
| ReadinessProbe: readinessProbe, | ||
| }, | ||
| }, | ||
| Volumes: []corev1.Volume{ | ||
| { | ||
| Name: globaloptions.KarmadaCertsName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: globaloptions.KarmadaCertsName, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Volumes: func() []corev1.Volume { | ||
| if strings.ToLower(i.SecretLayout) == secretLayoutSplit { | ||
| return []corev1.Volume{ | ||
| {Name: serverCertVolumeName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretApiserverServer}}}, | ||
| {Name: etcdClientCertVolumeName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretApiserverEtcdClient}}}, | ||
| {Name: frontProxyClientCertVolumeName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretApiserverFrontProxyClient}}}, | ||
| {Name: saKeyPairVolumeName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: certconst.SecretApiserverServiceAccountKeys}}}, | ||
| } | ||
| } | ||
| return []corev1.Volume{{Name: globaloptions.KarmadaCertsName, VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: globaloptions.KarmadaCertsName}}}} | ||
| }(), |
There was a problem hiding this comment.
Using immediately-invoked function expressions (IIFEs) to conditionally define struct fields like VolumeMounts and Volumes makes the code harder to read and debug. It would be cleaner to extract this logic into separate helper methods on CommandInitOption.
|
/hold This pull request has a dependency on #6788. Kindly wait for the prerequisite PR to be merged before proceeding with the review and potential merge of this one. |
karmada-bot
added
the
do-not-merge/hold
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6810 +/- ##
==========================================
+ Coverage 45.77% 45.83% +0.05%
==========================================
Files 689 690 +1
Lines 57161 57693 +532
==========================================
+ Hits 26164 26442 +278
- Misses 29371 29559 +188
- Partials 1626 1692 +66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Part of #6670
Which issue(s) this PR fixes:
Fixes #6670
This commit allows karmadactl init to generate certificates with a new architecture (unique CN/SAN for each) when using the --secret-layout flag. This PR, combined with #6788 , completes the transition to the new self-signed certificate generation mechanism for karmadactl.
Special notes for your reviewer:
@chaosi-zju
@zhzhuang-zju
@XiShanYongYe-Chang
Does this PR introduce a user-facing change?: