Changes for cli command to renew certificate #912

pravinpushkar · 2022-03-04T14:20:29Z

Signed-off-by: Pravin Pushkar ppushkar@microsoft.com

Description

Adding new commands to renew expiring certificate.

# Generates new root and issuer certificates for kubernetest cluster
dapr mtls renew-certificate -k --valid-until <no of days> --restart

# Uses existing private root.key to generate new root and issuer certificates for kubernetes cluster
dapr mtls renew-certificate -k --certificate-password-file myprivatekey.key --valid-until <no of days>

# Rotates certificate of your kubernetes cluster with provided ca.cert, issuer.crt and issuer.key file path
dapr mtls renew-certificate -k --ca-root-certificate <ca.crt> --issuer-private-key <issuer.key> --issuer-public-certificate <issuer.crt> --restart

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #892

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

cmd/mtls.go

cmd/renew_certificate.go

shubham1172 · 2022-03-09T12:10:20Z

cmd/renew_certificate.go

+	Short: "Rotates Dapr root certificate of your kubernetes cluster",
+	Example: `
+# Generates new root and issuer certificates for kubernetest cluster
+dapr mtls renew-cert -k --valid-until <no of days> --restart true


Other mtls commands don't seem to support the -k flag. Is that expected?

dapr mtls expiry -k Error: unknown shorthand flag: 'k' in -k

this is expected in case we want to enhance it for standalone.
export and expiry does not make much sense for standalone mode imo.
In general, I think it should be with the specific subcommands whether -k flag is needed or not.

If mtls only supports k8s, then the subcommands should also enforce it ideally. Instead of silently failing, we should complain that it's a required flag and only k8s mode is supported. This can be tracked separately, but I think we should do it. Thoughts?

/cc @mukundansundar

I will be adding RenewCertificateCmd.MarkFlagRequired("kubernetes") in new commit.
We can enforce this flag through all subcommands but it will change the current behavior from dapr mtls expiry to of dapr mts expiry -k
@mukundansundar

codecov · 2022-03-09T13:31:54Z

Codecov Report

Merging #912 (038f841) into master (ce0af7c) will decrease coverage by 1.41%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
- Coverage   22.31%   20.90%   -1.42%     
==========================================
  Files          30       32       +2     
  Lines        1734     1851     +117     
==========================================
  Hits          387      387              
- Misses       1293     1410     +117     
  Partials       54       54

Impacted Files	Coverage Δ
pkg/kubernetes/common.go	`0.00% <0.00%> (ø)`
pkg/kubernetes/renew_certificate.go	`0.00% <0.00%> (ø)`
pkg/kubernetes/upgrade.go	`21.91% <0.00%> (+3.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce0af7c...038f841. Read the comment docs.

cmd/renew_certificate.go

README.md

tests/e2e/kubernetes/kubernetes_test.go

mukundansundar

few more changes please ...

cmd/renew_certificate.go

mukundansundar · 2022-03-22T14:27:10Z

cmd/renew_certificate.go

+						logErrorAndExit(err)
+					}
+				} else if privateKey != "" {
+					print.InfoStatusEvent(os.Stdout, "Using password file to generate root certificate")


Suggested change

print.InfoStatusEvent(os.Stdout, "Using password file to generate root certificate")

print.InfoStatusEvent(os.Stdout, "Using existing private key file to generate root certificate")

This change needs to be done?

mukundansundar · 2022-03-22T14:48:59Z

tests/e2e/common/common.go

+		assert.Contains(t, output, "Certificate rotation is successful!")
+
+		// remove cert directory created earlier.
+		os.RemoveAll("./certs")


not like to have a folder created by one test removed by another .... can this be in the Kubernetes_test.go file instead of common ?
A single flow ...

create a folder ./certs

Run GenerateNewCerts -- export cert to the previously created folder

Run the UserProvidedNewCertsAndRenew test ....

Remove the ./certs folder that was created in step 1

1 and 4 in the Test function ... and the rest can be what it is now ...

mukundansundar · 2022-03-22T15:30:20Z

@yaron2 @artursouza Don't we also need to restart sidecar-injector and dashboard as part of certificate rotation ?

yaron2 · 2022-03-22T15:31:59Z

@yaron2 @artursouza Don't we also need to restart sidecar-injector and dashboard as part of certificate rotation ?

Sentry watches certs from the filesystem.

Addressed

cmd/renew_certificate.go

mukundansundar · 2022-03-22T17:25:15Z

During release validations during endgame ... test this against a cluster running apps and validate working.

mukundansundar · 2022-03-22T19:42:10Z

@pravinpushkar Wait for all containers to be up needs to be added in this PR.

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

addressed

pravinpushkar requested review from a team as code owners March 4, 2022 14:20

pravinpushkar force-pushed the feature/renew_cert branch 3 times, most recently from a650191 to dc38140 Compare March 9, 2022 11:01

shubham1172 reviewed Mar 9, 2022

View reviewed changes

cmd/mtls.go Outdated Show resolved Hide resolved

shubham1172 reviewed Mar 9, 2022

View reviewed changes

cmd/renew_certificate.go Outdated Show resolved Hide resolved

shubham1172 reviewed Mar 9, 2022

View reviewed changes

pravinpushkar force-pushed the feature/renew_cert branch from fe1220a to 6695f1e Compare March 11, 2022 11:31

pravinpushkar changed the title ~~[WIP] Changes for cli command to renew certificate~~ Changes for cli command to renew certificate Mar 11, 2022

pravinpushkar force-pushed the feature/renew_cert branch 4 times, most recently from 82feed2 to e802685 Compare March 14, 2022 17:49

artursouza self-assigned this Mar 15, 2022

pravinpushkar force-pushed the feature/renew_cert branch 2 times, most recently from 4045818 to 97ac56f Compare March 21, 2022 06:02

yaron2 previously requested changes Mar 21, 2022

View reviewed changes

cmd/renew_certificate.go Outdated Show resolved Hide resolved

pravinpushkar force-pushed the feature/renew_cert branch from 97ac56f to 920549d Compare March 21, 2022 18:04

artursouza suggested changes Mar 22, 2022

View reviewed changes

cmd/renew_certificate.go Show resolved Hide resolved

pravinpushkar force-pushed the feature/renew_cert branch 3 times, most recently from 8802c82 to 9be44d0 Compare March 22, 2022 09:16

mukundansundar reviewed Mar 22, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

tests/e2e/kubernetes/kubernetes_test.go Outdated Show resolved Hide resolved

pravinpushkar force-pushed the feature/renew_cert branch from 28971de to 4a1fdcf Compare March 22, 2022 11:38

mukundansundar reviewed Mar 22, 2022

View reviewed changes

artursouza previously approved these changes Mar 22, 2022

View reviewed changes

yaron2 previously requested changes Mar 22, 2022

View reviewed changes

cmd/renew_certificate.go Outdated Show resolved Hide resolved

msfussell mentioned this pull request Mar 22, 2022

New CLI mtls commands for renewing the generated root and issued certificates dapr/docs#2285

Closed

pravinpushkar added 12 commits March 23, 2022 05:20

Changes for cli command to renew certificate

53fb65d

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

adding renew-cert as subcommand to mtls

161eb8c

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

Impl for generating fresh certificates with provided ttl

ece5387

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

fix lint and ttl

f26882c

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

Adding a flag to restart services

1423dca

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

Adding tests and some refactoring

c0eedce

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

Adding timeout and changing flags detail

9e30c92

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

fixing review comments

4cbe329

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

Fixing review comments and test failures

bd461c2

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

Fixing tests and readme

d2b4641

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

fixing uninstall and other certificate test

3492369

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

review comments fixes

038f841

Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>

pravinpushkar dismissed artursouza’s stale review via 038f841 March 22, 2022 23:51

pravinpushkar force-pushed the feature/renew_cert branch from 0fbccbd to 038f841 Compare March 22, 2022 23:51

pravinpushkar requested a review from yaron2 March 23, 2022 03:32

mukundansundar approved these changes Mar 23, 2022

View reviewed changes

mukundansundar merged commit 4818533 into dapr:master Mar 23, 2022

pravinpushkar mentioned this pull request Jul 13, 2022

REQUEST: Approver status for @pravinpushkar in dapr/cli dapr/community#191

Closed

4 tasks

shubham1172 mentioned this pull request Aug 11, 2022

REQUEST: Approver status for @shubham1172 in dapr/cli dapr/community#202

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for cli command to renew certificate #912

Changes for cli command to renew certificate #912

pravinpushkar commented Mar 4, 2022 •

edited

Loading

shubham1172 Mar 9, 2022

pravinpushkar Mar 9, 2022

shubham1172 Mar 10, 2022

pravinpushkar Mar 10, 2022

codecov bot commented Mar 9, 2022 •

edited

Loading

mukundansundar left a comment

mukundansundar Mar 22, 2022

mukundansundar Mar 23, 2022

mukundansundar Mar 22, 2022

mukundansundar commented Mar 22, 2022

yaron2 commented Mar 22, 2022

mukundansundar commented Mar 22, 2022

mukundansundar commented Mar 22, 2022

	print.InfoStatusEvent(os.Stdout, "Using password file to generate root certificate")
	print.InfoStatusEvent(os.Stdout, "Using existing private key file to generate root certificate")

Changes for cli command to renew certificate #912

Changes for cli command to renew certificate #912

Conversation

pravinpushkar commented Mar 4, 2022 • edited Loading

Description

Issue reference

Checklist

shubham1172 Mar 9, 2022

Choose a reason for hiding this comment

pravinpushkar Mar 9, 2022

Choose a reason for hiding this comment

shubham1172 Mar 10, 2022

Choose a reason for hiding this comment

pravinpushkar Mar 10, 2022

Choose a reason for hiding this comment

codecov bot commented Mar 9, 2022 • edited Loading

Codecov Report

mukundansundar left a comment

Choose a reason for hiding this comment

mukundansundar Mar 22, 2022

Choose a reason for hiding this comment

mukundansundar Mar 23, 2022

Choose a reason for hiding this comment

mukundansundar Mar 22, 2022

Choose a reason for hiding this comment

mukundansundar commented Mar 22, 2022

yaron2 commented Mar 22, 2022

mukundansundar commented Mar 22, 2022

mukundansundar commented Mar 22, 2022

pravinpushkar commented Mar 4, 2022 •

edited

Loading

codecov bot commented Mar 9, 2022 •

edited

Loading