🐛 default service monitor configuration to use https #2065

johanneswuerbach · 2021-03-05T20:15:03Z

By default the ServiceMonitor scrapes port: https https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v2/config/prometheus/monitor.yaml#L13, which is exposed by the auth proxy service https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v2/config/rbac/auth_proxy_service.yaml#L10-L12 and requires an https connection https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v2/config/default/manager_auth_proxy_patch.yaml#L15

Change the ServiceMonitor configuration to work with that by default as otherwise the scraping fails with a 400 Bad Request.

Thanks to #1253 (comment) for the required flags.

k8s-ci-robot · 2021-03-05T20:15:11Z

Welcome @johanneswuerbach!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-03-05T20:15:11Z

Hi @johanneswuerbach. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

johanneswuerbach · 2021-03-05T20:57:03Z

/assign @Adirio

estroz · 2021-03-05T21:00:59Z

pkg/plugins/golang/v2/scaffolds/internal/templates/config/prometheus/monitor.go

+      scheme: https
+      bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token
+      tlsConfig:
+        insecureSkipVerify: true


At this point, no new features should go into go/v2

Suggested change

scheme: https

bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token

tlsConfig:

insecureSkipVerify: true

As the current configuration doesn’t work, it could also be seen as a bug fix, but happy to remove this part.

If that's true, then it can be left in I suppose.

If it is a bugfix (haven't checked that) it should have the 🐛 (:bug:) emoji.

I wasn't sure, but as the default configuration just doesn't work I think it makes sense to treat this as a bugfix.

estroz · 2021-03-05T21:18:52Z

pkg/plugins/golang/v3/scaffolds/internal/templates/config/prometheus/monitor.go

+      tlsConfig:
+        insecureSkipVerify: true


The webhook server already sets up TLS. I wonder if that cert/key can be used here.

This seems out of scope of this PR, but seems like a good idea otherwise.

camilamacedo86 · 2021-03-06T17:03:01Z

docs/book/src/multiversion-tutorial/testdata/project/config/prometheus/monitor.yaml

+      scheme: https
+      bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token
+      tlsConfig:
+        insecureSkipVerify: true
  selector:


I do not think that we should move forward here. See the doc: https://book.kubebuilder.io/reference/metrics.html

These metrics are protected by kube-auth-proxy by default if using kubebuilder. Kubebuilder v2.2.0+ scaffold a clusterrole which can be found at config/rbac/auth_proxy_client_clusterrole.yaml.

You will need to grant permissions to your Prometheus server so that it can scrape the protected metrics. To achieve that, you can create a clusterRoleBinding to bind the clusterRole to the service account that your Prometheus server uses. You can run the following kubectl command to create it. If using kubebuilder <project-prefix> is the namePrefix field in config/default/kustomization.yaml. kubectl create clusterrolebinding metrics --clusterrole=<project-prefix>-metrics-reader --s

Did you follow that?

Yes, but I'm not sure how your comment is related.

The proxy is by default exposed only via https so scraping using scheme: http will result in a 400 error. Annoyingly this directly implemented in golang, so the rbac proxy doesn't log anything and prometheus doesn't log the response body.

The bearerTokenFile is required for prometheus to actually include a Authorization header.

This header is required so the proxy can actually validate whether the request has the necessary permissions (you get a 401 otherwise, while the missing binding would result in a 403)

The insecureSkipVerify is required as the rbac proxy uses a self-generate certificate in the current configuration https://github.com/brancz/kube-rbac-proxy/blob/e4b31758aedb3d29d8f50d615828401a30d28c1e/main.go#L261

@estroz proposed to use the webhook certificate #2065 (comment), but this seems out of scope of this PR.

I can be missing something here our misunderstanding. However, see: https://github.com/kubernetes-sigs/kubebuilder/pull/1317/files#diff-20a08dcfd7ad4ed3343d00cf32c7844ce41a2d8d5f5df5b6de56852a510c5517R46 from the PR: #1317 which via the rbac rules allows accessing metrics behind kube-rbac-proxy.

Then, see that in the e2e tests that are checked:

https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/plugin_cluster_test.go#L165-L170

https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/plugin_cluster_test.go#L272-L303

So, I am trying here understand why in the e2e test we get HTTP 200 from the metrics endpoint and you are facing 400 or 403. What is missing? What has been working our e2e tests and not for you?

Is it because we are getting the token to check and doing the handshake and you are not?

Then, I understand that the default configuration has been allowing Prometheus to get the metrics. See that the problem point out by you was solved in the past with the rbac roles #1317.

Anyway, shows that also has some difference between using Prometheus Operator instead of kube-prometheus which has been solved with the solution applied here as you point out : #1253 (comment)

So, I will

/hold cancel

However, would be very nice to have a new issue with the steps required to reproduce the scenario for the future we are able to check that. Describing the cluster and Prometheus used, and if the rbac roles for allowing the Prometheus get the metrics were or not used, and what are the steps to check the error.

Actually, Camila's point is pretty sound. If e2e tests are working and this is being tested, we need an issue describing the steps to get this error so that we can reproduce and check that it is an actual bug and not just an error on your side.

Go 1.16? Not supported yet. There are some modules related changes (like some commands not tidying up go.mod anymore) that haven't been taking into account in kubebuilder nor any of their dependencies.

Ah that is unfortunate. If I have some time I can update e2e code after this is merged.

Just to highlight: We might be unable to reproduce the issue in the e2e as it now if it is specific only when to use Prometheus Operator instead of kube-prometheus.

If you create an issue describing the need of an e2e test that covers this case, I'm ok merging this.

Opened #2087

camilamacedo86 · 2021-03-07T09:43:16Z

testdata/project-v3/config/prometheus/monitor.yaml

+      scheme: https
+      bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token
+      tlsConfig:
+        insecureSkipVerify: true


Would not it remove the need to get the token in the e2e tests?

https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/plugin_cluster_test.go#L165-L170

https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/plugin_cluster_test.go#L272-L303

If yes, I think we need to remove that from the e2e tests to validate your solution as well.

No, this only instructs how prometheus should scrape the service, but doesn’t change anything about the service itself. As prometheus isn’t used in e2e tests this doesn’t really change anything as this part is not test covered yet.

Adirio · 2021-03-08T11:57:18Z

/ok-to-test

camilamacedo86

It shows OK since more than one person shows that have this problem and solves it the same way. We might be unable to reproduce the issue in the e2e as it now if it is specific only when to use Prometheus Operator instead of kube-prometheus.

So, it is fine for me.

More info: https://github.com/kubernetes-sigs/kubebuilder/pull/2065/files#r589002353

johanneswuerbach · 2021-03-15T18:13:03Z

I created the related e2e test ticket #2087, anything else missing here?

estroz

Since the metrics endpoint is protected by kube-rbac-proxy, I don't see a reason against merging this.

/lgtm

k8s-ci-robot · 2021-03-17T21:13:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, estroz, johanneswuerbach

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~~ [camilamacedo86,estroz]

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 5, 2021

k8s-ci-robot requested review from estroz and mengqiy March 5, 2021 20:15

feat: improve service monitor configuration

f81fb0d

johanneswuerbach force-pushed the improve-service-monitor branch from 26d0997 to f81fb0d Compare March 5, 2021 20:22

k8s-ci-robot assigned Adirio Mar 5, 2021

estroz reviewed Mar 5, 2021

View reviewed changes

camilamacedo86 reviewed Mar 6, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2021

johanneswuerbach changed the title ~~✨ improve service monitor configuration~~ 🐛 default service monitor configuration to use https Mar 6, 2021

johanneswuerbach changed the title ~~🐛 default service monitor configuration to use https~~ 🐛 default service monitor configuration to use https Mar 6, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2021

camilamacedo86 reviewed Mar 7, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2021

camilamacedo86 approved these changes Mar 9, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

johanneswuerbach mentioned this pull request Mar 15, 2021

E2E test for prometheus metrics #2087

Closed

estroz approved these changes Mar 17, 2021

View reviewed changes

k8s-ci-robot assigned estroz Mar 17, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2021

camilamacedo86 mentioned this pull request Mar 17, 2021

Align Helm/Ansible plugins with the changes made for Golang ( go/v3 ) operator-framework/operator-sdk#4542

Closed

k8s-ci-robot merged commit faff148 into kubernetes-sigs:master Mar 17, 2021

This was referenced Apr 12, 2021

Extend custom metrics paragraph #1906

Closed

Prometheus ServiceMonitor failing to scrape operator metrics served though kube-proxy HTTPS 8443 port operator-framework/operator-sdk#4764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 default service monitor configuration to use https #2065

🐛 default service monitor configuration to use https #2065

johanneswuerbach commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

johanneswuerbach commented Mar 5, 2021

estroz Mar 5, 2021

johanneswuerbach Mar 5, 2021

estroz Mar 5, 2021

Adirio Mar 5, 2021

johanneswuerbach Mar 6, 2021 •

edited

estroz Mar 5, 2021

johanneswuerbach Mar 6, 2021

camilamacedo86 Mar 6, 2021

camilamacedo86 Mar 6, 2021

johanneswuerbach Mar 6, 2021

camilamacedo86 Mar 7, 2021

Adirio Mar 8, 2021

Adirio Mar 8, 2021

estroz Mar 9, 2021

camilamacedo86 Mar 9, 2021

Adirio Mar 9, 2021

johanneswuerbach Mar 15, 2021

camilamacedo86 Mar 7, 2021

johanneswuerbach Mar 8, 2021

Adirio commented Mar 8, 2021

camilamacedo86 left a comment

johanneswuerbach commented Mar 15, 2021

estroz left a comment

k8s-ci-robot commented Mar 17, 2021

🐛 default service monitor configuration to use https #2065

🐛 default service monitor configuration to use https #2065

Conversation

johanneswuerbach commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

johanneswuerbach commented Mar 5, 2021

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

johanneswuerbach Mar 6, 2021 • edited

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

Adirio commented Mar 8, 2021

camilamacedo86 left a comment

Choose a reason for hiding this comment

johanneswuerbach commented Mar 15, 2021

estroz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 17, 2021

johanneswuerbach Mar 6, 2021 •

edited