generate client key and certificates #1282

stlaz · 2021-07-14T09:26:16Z

This is a PR to wire the creation of client certificates for metrics scraping. It uses library-go idiomatic controller logic which is not used yet in CMO.

Kindly asking for feedback. If the general direction is accepted, I will continue with wiring those certificates to Prometheus.

/cc @openshift/openshift-team-monitoring

stlaz · 2021-07-14T09:31:26Z

This PR includes some fixes to the original of #1252, namely:

the client CA is now passed in as a config map, there's no need to have it as a secret
the added RBAC now includes permissions to handle events as that's a new thing for the operator
the CSRs now use the generateName instead of a single name to prevent the name of the CSR being taken and thus preventing us from creating a CSR and progressing further. This fix also adds the label required by the CPO to approve the CSR

stlaz · 2021-07-14T14:02:42Z

looks like the failures in the kube-rbac-proxy are persisting, I'll try to check what's going on

stlaz · 2021-07-15T09:07:06Z

The problem was in the client-ca being missing for the kube-rbac-proxy-thanos container in the prometheus-k8s workload. Nevertheless, the kube-rbac-proxy should not panic in such cases but that's something that can be investigated outside of this PR.

stlaz · 2021-07-15T09:55:40Z

Opened brancz/kube-rbac-proxy#131 to track the kube-rbac-proxy issue

stlaz · 2021-07-15T11:28:52Z

added fixes for other failing kube-rbac-proxies that did not have the client-ca configured

stlaz · 2021-07-15T13:59:55Z

mhm, the tech debt of running things sequentially is showing as the operator refuses to progress to the client-ca creation while the prometheus-operator fails to deploy because it's missing the client-ca 😐 In an ideal world, this would be handled by two separate controllers.

I'll see if rearranging the order is possible.

pkg/tasks/prometheus.go

stlaz · 2021-07-19T12:00:18Z

the related test errors from the "common" test runs seem to have passed and the cluster installation is now successful.

I can see that the operator test still fails. Very likely because of the kube-rbac-proxy bug that's prevents our client-cert-authenticated metrics scraping requests from succeeding. That's a shame. I'll try to see if I can fix that.

simonpasquier · 2021-07-19T14:14:03Z

Yes it looks like the error is now with the kube-rbac-proxy of the prometheus-operator in the openshift-user-workload-monitoring namespace [1].

Maybe you could only enable client TLS authentication for a few targeted service monitors (e.g. node_exporter, kubelet and kube-state-metrics)?

[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-monitoring-operator/1282/pull-ci-openshift-cluster-monitoring-operator-master-e2e-agnostic-operator/1417043735262269440/artifacts/e2e-agnostic-operator/gather-extra/artifacts/pods/openshift-user-workload-monitoring_prometheus-operator-6cc4bfb495-w4w9f_kube-rbac-proxy.log

stlaz · 2021-07-19T16:44:15Z

I believe I've found the issue, we should be green once brancz/kube-rbac-proxy#132 makes it to the release payload

go.mod

sthaha · 2021-07-20T01:06:09Z

Would the PR also include tests?

stlaz · 2021-07-20T10:20:55Z

I'm using the current e2e tests of this repository (quite successfully, actually, issues were found and are being fixed).

For unit tests, please refer to https://github.com/openshift/library-go/blob/8cc52ce9b72b591c2c1946c81c21f823bbc43a1d/pkg/operator/csr/controller_test.go#L33

stlaz · 2021-07-20T10:25:33Z

fixed the go.mod in the latest revision

simonpasquier · 2021-07-20T12:25:50Z

@sthaha I agree with @stlaz here about tests. The purpose of the change is to switch some service monitors to use TLS client auth (instead of bearer tokens) and the CMO e2e tests already ensure that all monitoring components are scraped successfully so we should be good if all tests pass.

…ble to progress The prometheus-operator itself is using an kube-rbac-proxy and therefore needs to have the client-ca for its deployment.

slaskawi · 2021-07-20T17:17:33Z

/retest

slaskawi · 2021-07-20T17:19:35Z

@simonpasquier Just FYI - @stlaz started his PTO today and I'll be looking at this PR when he's off. Please let me know if we need any additional changes.

sthaha · 2021-07-21T00:12:41Z

@sthaha I agree with @stlaz here about tests. The purpose of the change is to switch some service monitors to use TLS client auth (instead of bearer tokens) and the CMO e2e tests already ensure that all monitoring components are scraped successfully so we should be good if all tests pass.

Thank you both 🙇, I was wondering how this change is validated :)

sthaha · 2021-07-21T00:14:52Z

mhm, the tech debt of running things sequentially is showing as the operator refuses to progress to the client-ca creation while the prometheus-operator fails to deploy because it's missing the client-ca 😐 In an ideal world, this would be handled by two separate controllers.

@simonpasquier Should this go into our backlog?

stlaz · 2021-07-21T04:22:43Z

simonpasquier Should this go into our backlog?

you may want to consider library-go for your operator, I trust that the folks of #forum-apiserver would be willing to help, me including :)

simonpasquier

/lgtm

The CI is happy with the change so am I :)
Thanks @s-urbaniak @stlaz @slaskawi for the great work!

openshift-ci · 2021-07-21T12:02:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: simonpasquier, stlaz

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:

~~OWNERS~~ [simonpasquier]

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

openshift-bot · 2021-07-21T12:09:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T13:21:03Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T13:34:03Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T13:46:04Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-07-21T14:26:06Z

@stlaz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-single-node	`104addb`	link	`/test e2e-aws-single-node`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2021-07-21T14:46:02Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T14:59:11Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T15:39:28Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T15:50:04Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T16:56:04Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T17:34:03Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T17:47:11Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T18:51:47Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T19:30:46Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-21T19:43:48Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

slaskawi · 2021-07-22T05:53:47Z

🎉

openshift-ci bot requested a review from a team July 14, 2021 09:26

stlaz mentioned this pull request Jul 14, 2021

WIP: generate client key and certificates #1252

Closed

stlaz force-pushed the sur-certauth branch from 2ee20b5 to 6901ab7 Compare July 15, 2021 08:57

stlaz force-pushed the sur-certauth branch from 6901ab7 to ec56326 Compare July 15, 2021 11:28

simonpasquier reviewed Jul 15, 2021

View reviewed changes

pkg/tasks/prometheus.go Outdated Show resolved Hide resolved

stlaz force-pushed the sur-certauth branch from 7fb5fce to afd1c53 Compare July 19, 2021 08:48

sthaha reviewed Jul 20, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

stlaz force-pushed the sur-certauth branch from afd1c53 to 47e4665 Compare July 20, 2021 10:23

stlaz force-pushed the sur-certauth branch from 47e4665 to 311b1ad Compare July 20, 2021 10:41

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2021

vendor: bump library-go

5b4ef99

stlaz force-pushed the sur-certauth branch from 311b1ad to 9417768 Compare July 20, 2021 10:44

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2021

s-urbaniak and others added 4 commits July 20, 2021 14:33

generate client certificate for metrics scraping

9f78d5c

CSR: use CSR generate name and labels instead of a single name

969b089

update rbac to allow event handling

2fbfe40

make the client CA bundle be a CM, not a Secret

61359c8

s-urbaniak and others added 2 commits July 20, 2021 14:33

manifests,assets: regenerate

fe127be

move metrics-client-ca cm creation before prometheus-operator to be a…

104addb

…ble to progress The prometheus-operator itself is using an kube-rbac-proxy and therefore needs to have the client-ca for its deployment.

stlaz force-pushed the sur-certauth branch from 9417768 to 104addb Compare July 20, 2021 12:33

simonpasquier reviewed Jul 21, 2021

View reviewed changes

openshift-ci bot assigned simonpasquier Jul 21, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021

openshift-merge-robot merged commit df26aec into openshift:master Jul 21, 2021

generate client key and certificates #1282

generate client key and certificates #1282

Conversation

stlaz commented Jul 14, 2021

stlaz commented Jul 14, 2021

stlaz commented Jul 14, 2021

stlaz commented Jul 15, 2021

stlaz commented Jul 15, 2021 • edited Loading

stlaz commented Jul 15, 2021 • edited Loading

stlaz commented Jul 15, 2021 • edited Loading

stlaz commented Jul 19, 2021

simonpasquier commented Jul 19, 2021

stlaz commented Jul 19, 2021

sthaha commented Jul 20, 2021

stlaz commented Jul 20, 2021

stlaz commented Jul 20, 2021

simonpasquier commented Jul 20, 2021

slaskawi commented Jul 20, 2021

slaskawi commented Jul 20, 2021

sthaha commented Jul 21, 2021

sthaha commented Jul 21, 2021

stlaz commented Jul 21, 2021 • edited Loading

simonpasquier left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-ci bot commented Jul 21, 2021 • edited Loading

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

openshift-bot commented Jul 21, 2021

slaskawi commented Jul 22, 2021

stlaz commented Jul 15, 2021 •

edited

Loading

stlaz commented Jul 15, 2021 •

edited

Loading

stlaz commented Jul 15, 2021 •

edited

Loading

stlaz commented Jul 21, 2021 •

edited

Loading

openshift-ci bot commented Jul 21, 2021 •

edited

Loading