feat: karmadactl add ca-cert-path and ca-key-path opts #5127

guozheng-shen · 2024-07-03T13:09:15Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
issue: #5103

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmadactl`: Introduced `--ca-cert-file` and `--ca-key-file` flags to `init` command to specify the root CA which will be used to issue the certificate for components.

codecov-commenter · 2024-07-03T13:23:17Z

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

Codecov Report

Attention: Patch coverage is 59.09091% with 9 lines in your changes missing coverage. Please review.

Project coverage is 28.27%. Comparing base (c4a81bf) to head (43bb7c7).
Report is 30 commits behind head on master.

Files	Patch %	Lines
pkg/karmadactl/cmdinit/cert/cert.go	64.70%	3 Missing and 3 partials ⚠️
pkg/karmadactl/cmdinit/kubernetes/deploy.go	0.00%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5127      +/-   ##
==========================================
+ Coverage   28.21%   28.27%   +0.05%     
==========================================
  Files         632      632              
  Lines       43568    43608      +40     
==========================================
+ Hits        12294    12329      +35     
+ Misses      30378    30377       -1     
- Partials      896      902       +6

Flag	Coverage Δ
unittests	`28.27% <59.09%> (+0.05%)`	⬆️

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.

RainbowMango · 2024-07-04T01:27:02Z

/assign @chaosi-zju

@guozheng-shen You might need to rebase to get rid of the merge commits. #5127 (comment).

chaosi-zju

Hello, thank you for your important commit! Review is ongoing, and my viewpoint just for reference only.

pkg/karmadactl/cmdinit/cert/cert.go

chaosi-zju · 2024-07-05T01:48:10Z

The CI of this PR failed due to it wasn't signed off, usually please use git commit -s -m 'your message ' or git commit -m ' Signed-off-by: AuthorName <authoremail@example.com> \n <other message> ' to pass DCO.

Detail guideline can refer to: https://probot.github.io/apps/dco/

chaosi-zju · 2024-07-05T01:52:26Z

generally looks good, thanks for your contribution!

By the way, have you tested?

chaosi-zju · 2024-07-05T02:07:40Z

pkg/karmadactl/cmdinit/cert/cert_test.go

@@ -101,7 +101,7 @@ func TestGenCerts(_ *testing.T) {
 	apiserverCertCfg := NewCertConfig("karmada-apiserver", []string{""}, karmadaAltNames, &notAfter)
 	frontProxyClientCertCfg := NewCertConfig("front-proxy-client", []string{}, certutil.AltNames{}, &notAfter)

-	if err := GenCerts(TestCertsTmp, etcdServerCertConfig, etcdClientCertCfg, karmadaCertCfg, apiserverCertCfg, frontProxyClientCertCfg); err != nil {
+	if err := GenCerts(TestCertsTmp, "", "", etcdServerCertConfig, etcdClientCertCfg, karmadaCertCfg, apiserverCertCfg, frontProxyClientCertCfg); err != nil {


It would be even better if you could supplement the test coverage for this case GenCerts(TestCertsTmp, caCertPath, caKeyPath, etcdServerCertConfig, etcdClientCertCfg, karmadaCertCfg, apiserverCertCfg, frontProxyClientCertCfg) ٩(๑❛ᴗ❛๑)۶ .

pkg/karmadactl/cmdinit/cert/cert_test.go

chaosi-zju · 2024-07-10T02:55:46Z

/lgtm

cc @RainbowMango

pkg/karmadactl/cmdinit/cert/cert_test.go

tiansuo114 · 2024-07-11T01:33:44Z

Hello @guozheng-shen , I am a student participating in the karmada community open source activity. According to Tutor @liangyuanpeng suggestion, we think we can make some modifications to cert_test, adding the function of checking whether certificate files exist in the corresponding directory according to parameter passing to the end of the original unit test file. I am currently working on this aspect Would you like to ask what your opinion is on this modification, thank you for your trouble

您好，我是一名参加karmada社区开源活动的学生。根据导师@liangyuanpeng的建议，我们认为可以对cert_test进行一些修改，在原来的单元测试文件末尾添加根据参数传递来检查相应目录中是否存在证书文件的功能。我目前正在做这方面的工作,对此,我想请问您对这次修改有何意见,麻烦您了

pkg/karmadactl/cmdinit/cert/cert_test.go

RainbowMango

/assign

RainbowMango

Looks great! Thanks @guozheng-shen for doing this.
Just some nits, otherwise looks good to me.

pkg/karmadactl/cmdinit/cmdinit.go

pkg/karmadactl/cmdinit/kubernetes/deploy.go

pkg/karmadactl/cmdinit/cert/cert.go

RainbowMango · 2024-07-11T08:32:11Z

pkg/karmadactl/cmdinit/cert/cert.go

@@ -261,11 +262,12 @@ func NewCertConfig(cn string, org []string, altNames certutil.AltNames, notAfter
 }

 // GenCerts Create CA certificate and sign etcd karmada certificate.
-func GenCerts(pkiPath string, etcdServerCertCfg, etcdClientCertCfg, karmadaCertCfg, apiserverCertCfg, frontProxyClientCertCfg *CertsConfig) error {
-	caCert, caKey, err := NewCACertAndKey("karmada")
+func GenCerts(pkiPath, caCertPath, caKeyPath string, etcdServerCertCfg, etcdClientCertCfg, karmadaCertCfg, apiserverCertCfg, frontProxyClientCertCfg *CertsConfig) error {


Note: Too many parameters for this function.

Not required to fix it in this PR. But it is concerning.

Signed-off-by: guozheng-shen <179826407@qq.com>

RainbowMango

/lgtm

@guozheng-shen Have you tested this latest patch on your side? Does it work as expected?

@chaosi-zju @liangyuanpeng Do you have any further comments?

liangyuanpeng · 2024-07-12T02:50:46Z

Have you tested this latest patch on your side? Does it work as expected?

wating for the answer for test successed,otherwise LGTM

chaosi-zju · 2024-07-12T06:21:59Z

looks good

CC @RainbowMango

RainbowMango

OK thanks, let's do it.
/approve

karmada-bot · 2024-07-12T06:41:00Z

[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:

~~pkg/karmadactl/cmdinit/OWNERS~~ [RainbowMango]

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

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

karmada-bot requested review from chaosi-zju and prodanlabs July 3, 2024 13:09

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 3, 2024

guozheng-shen force-pushed the feat-ca branch from 2cc9fa4 to 42978d7 Compare July 3, 2024 13:11

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

karmada-bot assigned chaosi-zju Jul 4, 2024

chaosi-zju reviewed Jul 4, 2024

View reviewed changes

pkg/karmadactl/cmdinit/cert/cert.go Outdated Show resolved Hide resolved

pkg/karmadactl/cmdinit/cert/cert.go Show resolved Hide resolved

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 Jul 4, 2024

guozheng-shen force-pushed the feat-ca branch from 4d81b00 to fcfe392 Compare July 4, 2024 13:55

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 4, 2024

chaosi-zju reviewed Jul 5, 2024

View reviewed changes

guozheng-shen force-pushed the feat-ca branch from d009e37 to 84b6497 Compare July 5, 2024 05:39

chaosi-zju reviewed Jul 9, 2024

View reviewed changes

pkg/karmadactl/cmdinit/cert/cert_test.go Outdated Show resolved Hide resolved

pkg/karmadactl/cmdinit/cert/cert_test.go Outdated Show resolved Hide resolved

guozheng-shen force-pushed the feat-ca branch from 84b6497 to 22a8c15 Compare July 9, 2024 11:53

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

liangyuanpeng reviewed Jul 10, 2024

View reviewed changes

pkg/karmadactl/cmdinit/cert/cert_test.go Outdated Show resolved Hide resolved

guozheng-shen force-pushed the feat-ca branch from 22a8c15 to 09f9600 Compare July 10, 2024 15:28

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2024

liangyuanpeng reviewed Jul 10, 2024

View reviewed changes

pkg/karmadactl/cmdinit/cert/cert_test.go Outdated Show resolved Hide resolved

tiansuo114 reviewed Jul 11, 2024

View reviewed changes

pkg/karmadactl/cmdinit/cert/cert_test.go Show resolved Hide resolved

RainbowMango reviewed Jul 11, 2024

View reviewed changes

karmada-bot assigned RainbowMango Jul 11, 2024

RainbowMango reviewed Jul 11, 2024

View reviewed changes

feat: karmadactl add ca-cert-path and ca-key-path opts

43bb7c7

Signed-off-by: guozheng-shen <179826407@qq.com>

guozheng-shen force-pushed the feat-ca branch from 338e548 to 43bb7c7 Compare July 11, 2024 13:30

RainbowMango reviewed Jul 12, 2024

View reviewed changes

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

liangyuanpeng mentioned this pull request Jul 12, 2024

improve karmadactl unit test: check the cert file exist #5175

Merged

RainbowMango approved these changes Jul 12, 2024

View reviewed changes

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

karmada-bot merged commit fdc29c7 into karmada-io:master Jul 12, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: karmadactl add ca-cert-path and ca-key-path opts #5127

feat: karmadactl add ca-cert-path and ca-key-path opts #5127

guozheng-shen commented Jul 3, 2024 •

edited by RainbowMango

Loading

codecov-commenter commented Jul 3, 2024 •

edited

Loading

RainbowMango commented Jul 4, 2024

chaosi-zju left a comment

chaosi-zju commented Jul 5, 2024

chaosi-zju commented Jul 5, 2024 •

edited

Loading

chaosi-zju Jul 5, 2024

guozheng-shen Jul 5, 2024

chaosi-zju commented Jul 10, 2024

tiansuo114 commented Jul 11, 2024

RainbowMango left a comment

RainbowMango left a comment

RainbowMango Jul 11, 2024

RainbowMango left a comment

liangyuanpeng commented Jul 12, 2024

chaosi-zju commented Jul 12, 2024

RainbowMango left a comment

karmada-bot commented Jul 12, 2024

feat: karmadactl add ca-cert-path and ca-key-path opts #5127

feat: karmadactl add ca-cert-path and ca-key-path opts #5127

Conversation

guozheng-shen commented Jul 3, 2024 • edited by RainbowMango Loading

codecov-commenter commented Jul 3, 2024 • edited Loading

Codecov Report

RainbowMango commented Jul 4, 2024

chaosi-zju left a comment

Choose a reason for hiding this comment

chaosi-zju commented Jul 5, 2024

chaosi-zju commented Jul 5, 2024 • edited Loading

chaosi-zju Jul 5, 2024

Choose a reason for hiding this comment

guozheng-shen Jul 5, 2024

Choose a reason for hiding this comment

chaosi-zju commented Jul 10, 2024

tiansuo114 commented Jul 11, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango Jul 11, 2024

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

liangyuanpeng commented Jul 12, 2024

chaosi-zju commented Jul 12, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jul 12, 2024

guozheng-shen commented Jul 3, 2024 •

edited by RainbowMango

Loading

codecov-commenter commented Jul 3, 2024 •

edited

Loading

chaosi-zju commented Jul 5, 2024 •

edited

Loading