Support cert-manager for certificate management #277

Gallardot · 2020-02-24T14:20:48Z

What problem does this PR solve?

#237

What is changed and how does it work?

Check List

Tests

No code

Code changes

Has Terraform scripts change

Side effects

No

Related changes

Need to update the documentation

Does this PR introduce a user-facing change?:

Support cert-manager for certificate management.
Add doc for the helm charts.

claassistantio · 2020-02-24T14:20:53Z

All committers have signed the CLA.

cwen0 · 2020-02-24T14:23:09Z

@Gallardot Thanks for your contribution! Please sign CLA, first!

zhouqiang-cl · 2020-02-24T14:27:07Z

@Gallardot thank you very much.

zhouqiang-cl · 2020-02-24T15:01:11Z

@yeya24 please take a look too

yeya24

Thanks for your great work! Overall LGTM, just a few nits. Let @cwen0 give another review.

yeya24 · 2020-02-24T22:40:03Z

helm/chaos-mesh/README.md

+
+## Parameters
+
+The following tables list the configurable parameters of the Chaos Mesh chart and their default values.


Wow. This is amazing! I think every user needs that, thanks!

yeya24 · 2020-02-24T22:41:06Z

helm/chaos-mesh/README.md

+| `chaosDaemon.image` | the docker image for chaos-daemon | `pingcap/chaos-mesh:latest` |
+| `chaosDaemon.imagePullPolicy` | image pull policy | `Always` |
+| `chaosDaemon.grpcPort` |  | `31767` |
+| `chaosDaemon.httpPort` |  | `31766` |


Is it better to add some comments about the two ports? Like their usage?

Not very familiar with chaosDaemon. I may need some help.
chaosDaemon.grpcPort | the port which grpc server listens on
chaosDaemon.httpPort | the port which http server listens on

code from here
https://github.com/pingcap/chaos-mesh/blob/7ce47e8d6fb9d047fc82a0b555f5b690f42f8942/cmd/chaos-daemon/main.go#L38-L39

It's OK?

I think it is okay. Thanks

yeya24 · 2020-02-24T22:42:39Z

helm/chaos-mesh/README.md

+| `controllerManager.nodeSelector` |  Node labels for chaos-controller-manager pod assignment | `{}` |
+| `controllerManager.tolerations` |  Toleration labels for chaos-controller-manager pod assignment | `[]` |
+| `controllerManager.affinity` |  Map of chaos-controller-manager node/pod affinities | `{}` |
+| `controllerManager.podAnnotations` | Deployment chaos-controller-manager annotations | `{}`|


Deployment chaos-controller-manager annotations is kind of misleading since these annotations are for pods. What about just simply Pod annotations of chaos-controller-manager

yeya24 · 2020-02-24T22:43:14Z

helm/chaos-mesh/templates/_helpers.tpl

+*/}}
+{{- define "chaos-mesh.webhook" -}}
+{{- printf "admission-webhook.pingcap.com" -}}
+{{- end -}}


We may need a new line here

yeya24 · 2020-02-24T22:43:21Z

helm/chaos-mesh/templates/post-install-create-cert-job.yaml

+                  kubectl -n ${K8S_NAMESPACE} apply -f -
+
+{{- end }}


yeya24 · 2020-02-24T22:43:37Z

helm/chaos-mesh/templates/pre-delete-job.yaml

+              kubectl delete secrets -n {{ .Release.Namespace }} {{ template "chaos-mesh.certs" . }} || true
+{{- end }}


yeya24 · 2020-02-24T22:43:49Z

helm/chaos-mesh/templates/webhookconfiguration.yaml

+  secretName: {{ template "chaos-mesh.certs" . }}
+  issuerRef:
+    name: chaos-mesh-selfsigned
+{{- end }}


yeya24 · 2020-02-24T22:44:10Z

helm/chaos-mesh/values.yaml

+    enabled: false
+    autoDeleteSecret: false
+
+  FailurePolicy: Ignore


yeya24 · 2020-02-24T22:45:46Z

helm/chaos-mesh/README.md

+| `dashboard.resources` | CPU/Memory resource requests/limits for chaos-dashboard pod  | `requests: { cpu: "250m", memory: "512Mi" }, limits:{ cpu: "500m", memory: "1024Mi" }`  |
+| `dashboard.volume.storage` | | `3Gi` |
+| `dashboard.volume.storageClassName` | | `standard` |
+| `prometheus.create` | enable the prometheus | `false` |


Don't need the, enable Prometheus is fine

yeya24 · 2020-02-24T22:46:12Z

helm/chaos-mesh/README.md

+| `chaosDaemon.podAnnotations` | Deployment chaos-daemon annotations | `{}` |
+| `chaosDaemon.runtime` | runtime specifies which container runtime to use. Currently we only supports docker and containerd. | `docker` |
+| `chaosDaemon.socketPath` | specifies the container runtime socket | `/var/run/docker.sock` |
+| `dashboard.create` | enable the chaos-dashboard | `false` |


enable chaos-dashboard

yeya24 · 2020-02-24T23:00:32Z

helm/chaos-mesh/templates/post-install-mutatingwebhook-job.yaml

-                    service:
-                      name: chaos-mesh-controller-manager
-                      namespace: ${K8S_NAMESPACE}
-                      path: "/inject-v1-pod"


I tried to deploy with certManager.enabled = false and I got the error below in webhook-mw-job pod. Seems the service field is still required

# job failed webhook-mw-job-4fpbb 0/1 Error 0 69s webhook-mw-job-89zxk 0/1 Error 0 29s webhook-mw-job-kv44l 0/1 Error 0 99s webhook-mw-job-lrb6z 0/1 Error 0 89s webhook-mw-job-qlfsj 0/1 Error 0 101s klo webhook-mw-job-kv44l The MutatingWebhookConfiguration "chaos-mesh-sidecar-injector" is invalid: webhooks[0].clientConfig: Required value: exactly one of url or service is required

What is your k8s version? My version is v1.17.2 . System operating normally . I will fix it right now.

I am using 1.17.0

Can not recurrence this error, if I use 1.17.0. But I have added the service field on the MutatingWebhookConfiguration. Please try again. Thank you.

Thanks! With the updated PR, and then it works!

Gallardot · 2020-02-25T05:56:17Z

I have no idea for why the Jenkins job failed. I need some help.
@zhouqiang-cl

Gallardot · 2020-02-25T06:12:55Z

Maybe need to do the same things like @YangKeao at this 83a4100 ?

@zhouqiang-cl

YangKeao · 2020-02-25T07:13:14Z

@Gallardot go get sigs.k8s.io/controller-tools/cmd/[email protected] to upgrade controller-gen, and then make yaml will fix it.

zhouqiang-cl · 2020-02-25T07:13:38Z

Maybe need to do the same things like @YangKeao at this 83a4100 ?

@Gallardot @cwen0 will fix it

cwen0 · 2020-02-25T07:42:20Z

README.md

+
+```bash
+# helm 2.X
+helm delete chaos-mesh


helm delete chaos-mesh --purge

cwen0 · 2020-02-25T09:03:52Z

helm/chaos-mesh/README.md

@@ -0,0 +1,138 @@
+# Chaos Mesh


@dcalvin PTAL

cwen0 · 2020-02-25T09:17:14Z

helm/chaos-mesh/README.md

+webhook:
+  certManager:
+    enabled: true
+    autoDeleteSecret: true


autoDeleteSecret is misleading, How about naming it deleteSecret, If the deleteSecret is true, we will create a job to delete the secret, Otherwise, do nothing

Good advice. I'm confused about that too.

cwen0 · 2020-02-25T09:18:47Z

helm/chaos-mesh/templates/webhookconfiguration.yaml

@@ -0,0 +1,58 @@
+---


rename this file to webhook-configuration.yaml?

All done. Please take a look again.Thanks.

codecov-io · 2020-02-25T10:53:36Z

Codecov Report

Merging #277 into master will increase coverage by 1.43%.
The diff coverage is 39.47%.

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
+ Coverage    60.4%   61.84%   +1.43%     
==========================================
  Files          36       40       +4     
  Lines        1985     2445     +460     
==========================================
+ Hits         1199     1512     +313     
- Misses        692      833     +141     
- Partials       94      100       +6

Impacted Files	Coverage Δ
api/v1alpha1/timechaos_types.go	`4.34% <0%> (-0.31%)`	⬇️
pkg/chaosdaemon/time_server.go	`100% <100%> (ø)`	⬆️
pkg/utils/time.go	`33.33% <33.33%> (ø)`
pkg/time/time_linux.go	`49.12% <66.66%> (-0.88%)`	⬇️
pkg/webhook/inject/inject.go	`75.07% <0%> (ø)`
pkg/webhook/config/watcher/watcher.go	`54.08% <0%> (ø)`
pkg/webhook/config/watcher/config.go	`100% <0%> (ø)`
pkg/webhook/config/config.go	`90% <0%> (+2.5%)`	⬆️

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 ce4a192...7fe605f. Read the comment docs.

yeya24

I think it looks good on my side, thanks! You just need to address other reviewers' comments.

cwen0

LGTM

cwen0 · 2020-02-26T06:48:54Z

@yeya24 PTAL again, thanks!

fewdan

LGTM

zhouqiang-cl · 2020-02-26T13:41:42Z

/merge

sre-bot · 2020-02-26T13:43:51Z

/run-all-tests

Gallardot added 2 commits February 24, 2020 10:58

Support cert-manager for certificate management

080c652

doc for helm charts

9cd0fed

sre-bot added the contribution label Feb 24, 2020

Gallardot requested a review from cwen0 February 24, 2020 14:27

cwen0 added the component/helm label Feb 24, 2020

yeya24 reviewed Feb 24, 2020

View reviewed changes

yeya24 requested changes Feb 24, 2020

View reviewed changes

Fix typo and MutatingWebhookConfiguration error

6fa3621

cwen0 reviewed Feb 25, 2020

View reviewed changes

Merge branch 'master' into cert

7b30234

make changes according to review

3af233e

yeya24 previously approved these changes Feb 25, 2020

View reviewed changes

Gallardot dismissed yeya24’s stale review via 3af233e February 26, 2020 02:30

resolved conflicts

ce4a192

cwen0 approved these changes Feb 26, 2020

View reviewed changes

fewdan approved these changes Feb 26, 2020

View reviewed changes

sre-bot added the status/can-merge label Feb 26, 2020

Merge branch 'master' into cert

7fe605f

sre-bot merged commit 43450b9 into chaos-mesh:master Feb 26, 2020

Gallardot deleted the cert branch February 27, 2020 02:09

sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021

Support cert-manager for certificate management (chaos-mesh#277)

93495bc


		## Parameters

		The following tables list the configurable parameters of the Chaos Mesh chart and their default values.

		kubectl delete secrets -n {{ .Release.Namespace }} {{ template "chaos-mesh.certs" . }} \|\| true
		{{- end }}

Support cert-manager for certificate management #277

Support cert-manager for certificate management #277

Conversation

Gallardot commented Feb 24, 2020

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

claassistantio commented Feb 24, 2020 • edited Loading

cwen0 commented Feb 24, 2020

zhouqiang-cl commented Feb 24, 2020

zhouqiang-cl commented Feb 24, 2020

yeya24 left a 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

yeya24 Feb 24, 2020 • edited Loading

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

Gallardot commented Feb 25, 2020

Gallardot commented Feb 25, 2020

YangKeao commented Feb 25, 2020

zhouqiang-cl commented Feb 25, 2020

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

codecov-io commented Feb 25, 2020 • edited Loading

Codecov Report

yeya24 left a comment

Choose a reason for hiding this comment

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 commented Feb 26, 2020

fewdan left a comment

Choose a reason for hiding this comment

zhouqiang-cl commented Feb 26, 2020

sre-bot commented Feb 26, 2020

claassistantio commented Feb 24, 2020 •

edited

Loading

yeya24 Feb 24, 2020 •

edited

Loading

codecov-io commented Feb 25, 2020 •

edited

Loading