Support Custom API Server CA Certificate for Karmada Instance in Operator #5842

jabellard · 2024-11-20T00:43:23Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
This is an implementation of this proposal to add support for custom API server CA certificates in the Karmada operator.

Which issue(s) this PR fixes:
Fixes #5784
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-operator`: The new `CustomCertificate.ApiServerCACert` field added as part of the Karmada spec can be used to specify the reference to a secret that contains a custom CA certificate for the Karmada API Server.

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

RainbowMango · 2024-11-20T01:53:28Z

/retest
the failing unit test is not related but an known issue.

codecov-commenter · 2024-11-20T02:00:58Z

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

Codecov Report

Attention: Patch coverage is 25.71429% with 52 lines in your changes missing coverage. Please review.

Project coverage is 45.99%. Comparing base (6795dba) to head (620638d).

Files with missing lines	Patch %	Lines
operator/pkg/tasks/init/cert.go	3.03%	31 Missing and 1 partial ⚠️
operator/pkg/init.go	0.00%	17 Missing and 1 partial ⚠️
operator/pkg/certs/certs.go	83.33%	2 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5842      +/-   ##
==========================================
- Coverage   46.02%   45.99%   -0.04%     
==========================================
  Files         660      660              
  Lines       53987    54028      +41     
==========================================
+ Hits        24848    24850       +2     
- Misses      27521    27558      +37     
- Partials     1618     1620       +2

Flag	Coverage Δ
unittests	`45.99% <25.71%> (-0.04%)`	⬇️

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.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

zhzhuang-zju · 2024-11-20T02:11:55Z

operator/pkg/certs/certs.go

-	caName   string
-	cert     []byte
-	key      []byte
+	PairName string


Can we avoid exposing internal fields by adding a new function to generate KarmadaCert, thereby minimizing the scope of changes?

@zhzhuang-zju So, what's your suggestion?
Do you mean to introduce some set methods?

How about

func NewKarmadaCert(pairName,caName string , cert, key []byte) *KarmadaCert{ return &KarmadaCert{ pairName: pairName, caName: caName, cert: cert, key: key, } }

@zhzhuang-zju , thanks for the suggestion.

zhzhuang-zju · 2024-11-20T02:35:09Z

operator/pkg/init.go

+	Kubeconfig              *rest.Config
+	KarmadaVersion          string
+	CRDTarball              operatorv1alpha1.CRDTarball
+	CustomCertificateConfig operatorv1alpha1.CustomCertificate


We can consider not adding a new field CustomCertificateConfig since Karmada already contains the configuration for customcertificates.

That's a good point. Thanks for bringing it up. I considered that, but if you take a closer look, the root Karmada object is not part of this type. It's possible to add it and get rid of a lot of fields here, but that would take a refactor with very wide scope. I think it's fine for now, as this object is meant to carry the only the info needed in downstream functions.

the root Karmada object is not part of this type

Sorry, I didn't get it. Could you please explain it more?

We can add the entire Karmada object to this field and get rid of some of the other fields, but that will introduce a refactor of a very wide scope with a lot of changes to the code. Can we avoid doing that for now?

karmada/operator/pkg/init.go

Lines 44 to 52 in 9416c08

type InitOptions struct {

Name string

Namespace string

Kubeconfig *rest.Config

KarmadaVersion string

CRDTarball operatorv1alpha1.CRDTarball

KarmadaDataDir string

Karmada *operatorv1alpha1.Karmada

}

But the current struct InitOptions already have the entire Karmada object. There's no need to remove other fields; it's just that no new fields need to be added.

Ok. Now I see where the confusion is coming from. I could have it explained it a bit better, so it's my fault. This is the struct I meant to refer to it. It's what's used to carry information to the downstream functions, not InitOptions. Notice how the root Karmada type is not in there.

That struct is then abstracted behind this interface which is what is used directly in those functions.

@zhzhuang-zju , just let me know if have any other comments/questions.

hah, Thank you for the clarification. I understand that CustomCertificateConfig is required for initdata, but is it possible to obtain it through initoptions.Karmada.Spec.CustomCertificate instead of adding it directly to initoptions? I actually don't want initoptions to become too large.

We can add the entire Karmada object to this field and get rid of some of the other fields, but that will introduce a refactor of a very wide scope with a lot of changes to the code. Can we avoid doing that for now?

Yeah, I agree that.

zhzhuang-zju · 2024-11-20T02:50:56Z

operator/pkg/tasks/init/cert.go

+		if kc.Name == constants.CaCertAndKeyName && customCertConfig.APIServerCACert != nil {
+			secretRef := customCertConfig.APIServerCACert
+			klog.V(4).InfoS("[certs] Loading custom CA certificate", "secret", secretRef.Name, "namespace", secretRef.Namespace)
+
+			certData, keyData, err := loadCACertFromSecret(data.RemoteClient(), secretRef)
+			if err != nil {
+				return fmt.Errorf("failed to load custom CA certificate: %w", err)
+			}
+
+			klog.V(2).InfoS("[certs] Successfully loaded custom CA certificate", "secret", secretRef.Name)
+
+			customKarmadaCert := &certs.KarmadaCert{
+				PairName: kc.Name,
+				CAName:   kc.CAName,
+				Cert:     certData,
+				Key:      keyData,
+			}
+
+			data.AddCert(customKarmadaCert)
+			klog.V(2).InfoS("[certs] Successfully added custom CA certificate to cert store", "certName", kc.Name)
+			return nil
+		}


How about extracting two functions: one to determine whether it is a custom CA, and another to load the karmadaCert using secretRef. This may enhance the extensibility of the code.

I think this is fine. It's a one liner that's very specific to the API Server CA. It's not general.

I think this is fine. It's a one liner that's very specific to the API Server CA. It's not general.

Get it~
Could we abstract the process of loading the karmadaCert by secretRef into a function? This way, if there's a need for custom other certificates in the future, this function could be used universally.

Not sure what you mean. That's actually already done in the loadCACertFromSecret function.

I mean L111-L125 can be extracted into a function. However, this is not important either; we can consider it when there is a real need.

Yeah. I agree with you on not abstracting the logic prematurely.

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

jabellard · 2024-11-20T12:59:24Z

@zhzhuang-zju , thanks for comments and suggestions. Left some notes and just pushed some changes.

zhzhuang-zju · 2024-11-22T01:24:25Z

thanks, others LGTM

RainbowMango

/lgtm
/approve

karmada-bot · 2024-11-22T08:01:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~charts/OWNERS~~ [RainbowMango]
~~operator/OWNERS~~ [RainbowMango]
~~operator/pkg/apis/OWNERS~~ [RainbowMango]

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

RainbowMango · 2024-11-22T08:01:58Z

/hold

RainbowMango · 2024-11-22T08:03:38Z

Oh... I meant to squash the commits...

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 20, 2024

karmada-bot requested review from calvin0327, lonelyCZ and Poor12 November 20, 2024 00:43

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

Add support for custom API Server CA certificate

91322e2

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

jabellard force-pushed the external_ca_cert branch from d257c38 to 91322e2 Compare November 20, 2024 00:45

jabellard mentioned this pull request Nov 20, 2024

Proposal: Support Custom CA Certificate for Karmada Instance in Operator #5794

Merged

Fix lint errors

620638d

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

zhzhuang-zju reviewed Nov 20, 2024

View reviewed changes

jabellard added 2 commits November 20, 2024 07:41

Address comments

4ff99eb

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

Address comments

3a09ac9

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

RainbowMango approved these changes Nov 22, 2024

View reviewed changes

karmada-bot assigned RainbowMango Nov 22, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2024

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2024

karmada-bot merged commit 4dbcfaf into karmada-io:master Nov 22, 2024
19 checks passed

karmada-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Custom API Server CA Certificate for Karmada Instance in Operator #5842

Support Custom API Server CA Certificate for Karmada Instance in Operator #5842

jabellard commented Nov 20, 2024 •

edited by RainbowMango

Loading

RainbowMango commented Nov 20, 2024

codecov-commenter commented Nov 20, 2024

zhzhuang-zju Nov 20, 2024

RainbowMango Nov 20, 2024

zhzhuang-zju Nov 20, 2024

jabellard Nov 20, 2024

zhzhuang-zju Nov 20, 2024

jabellard Nov 20, 2024

zhzhuang-zju Nov 21, 2024

jabellard Nov 21, 2024

zhzhuang-zju Nov 21, 2024

jabellard Nov 21, 2024

jabellard Nov 21, 2024

jabellard Nov 21, 2024

zhzhuang-zju Nov 21, 2024

RainbowMango Nov 22, 2024

zhzhuang-zju Nov 20, 2024

jabellard Nov 20, 2024

zhzhuang-zju Nov 21, 2024

jabellard Nov 21, 2024

zhzhuang-zju Nov 21, 2024

jabellard Nov 21, 2024

jabellard commented Nov 20, 2024

zhzhuang-zju commented Nov 22, 2024

RainbowMango left a comment

karmada-bot commented Nov 22, 2024

RainbowMango commented Nov 22, 2024

RainbowMango commented Nov 22, 2024

	type InitOptions struct {
	Name string
	Namespace string
	Kubeconfig *rest.Config
	KarmadaVersion string
	CRDTarball operatorv1alpha1.CRDTarball
	KarmadaDataDir string
	Karmada *operatorv1alpha1.Karmada
	}

Support Custom API Server CA Certificate for Karmada Instance in Operator #5842

Support Custom API Server CA Certificate for Karmada Instance in Operator #5842

Conversation

jabellard commented Nov 20, 2024 • edited by RainbowMango Loading

RainbowMango commented Nov 20, 2024

codecov-commenter commented Nov 20, 2024

Codecov Report

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

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

jabellard commented Nov 20, 2024

zhzhuang-zju commented Nov 22, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Nov 22, 2024

RainbowMango commented Nov 22, 2024

RainbowMango commented Nov 22, 2024

jabellard commented Nov 20, 2024 •

edited by RainbowMango

Loading