Fix manifest kustomization for metrics service

[erikgb]

Also removed unused ClusterRole and ClusterRoleBinding that is supposed to grant access through kube-rbac-proxy, which is currently not in use in HNC.

Tested: Verified that metrics service works as annotated by prometheus service annotations

Fixes #115

With this change, I can scrape metrics as described by metrics-service prometheus annotations:

$ kubectl -n hnc-system run -it --rm --restart=Never curl --image=curlimages/curl sh
If you don't see a command prompt, try pressing enter.
/ $ curl http://hnc-controller-manager-metrics-service:8080/metrics

Before this change, this fails:

$ kubectl -n hnc-system run -it --rm --restart=Never curl --image=curlimages/curl sh
If you don't see a command prompt, try pressing enter.
/ $ curl https://hnc-controller-manager-metrics-service:8443/metrics
curl: (7) Failed to connect to hnc-controller-manager-metrics-service port 8443 after 0 ms: Connection refused
/ $ 

Another option is to configure the kube-rbac-proxy. Please let me know if that solution is preferred.

/cc @adrianludwin

[rjbez17]

Wouldn't just changing this to 8443 fix our issue and still allow us to use HTTPS? I'm not sure we want to take a step back in security and not use HTTPS for metrics.

[erikgb]

Wouldn't just changing this to 8443 fix our issue and still allow us to use HTTPS? I'm not sure we want to take a step back in security and not use HTTPS for metrics.

No, there is nothing providing a HTTPS listener (on port 8443). Then we need to include the kube-rbac-proxy sidecar and route all metrics scrape request through its HTTPS listener - forwarding the request to the controller-manager metrics HTTP endpoint. That is an alternative mentioned in the PR description, though more complex, and I don't see the true benefit of this. I expect many K8s distros to block namespace ingress traffic by default.

[adrianludwin]

Yeah we took out kube-rbac-proxy a while ago (it has a bunch of problems from a supply chain perspective) and I'd rather not put it back in without good justification.

[rjbez17]

We don't need rbac proxy for https, that is only needed for authz. We already serve https traffic for the webhook on 9443. So I guess I don't see why we couldn't also use it (or have the option) for 8443 for metrics. However, that is probably out of scope of this PR if metrics are currently broken.

I created this to revisit this.

[adrianludwin]

/approve
/lgtm
/hold
/assign @rjbez17

I think this is good unless Ryan has any remaining concerns.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, erikgb

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 [adrianludwin]

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

[rjbez17]

I opened #204 to follow up on my concerns.

/hold cancel
/lgtm

[erikgb]

@rjbez17 @adrianludwin Thanks for accepting this PR. I think this is a bugfix, and should be cherry-picked to the v1.0 branch. The metrics setup is borked in the 1.0 release. WDYT?

[adrianludwin]

sgtm, I'd happily approve that and then release v1.0.1.

…

On Fri, Apr 22, 2022 at 4:27 PM Erik Godding Boye ***@***.***> wrote: @rjbez17 <https://github.com/rjbez17> @adrianludwin <https://github.com/adrianludwin> Thanks for accepting this PR. I think this is a bugfix, and should be cherry-picked to the v1.0 branch. The metrics setup is borked in the 1.0 release. WDYT? — Reply to this email directly, view it on GitHub <#202 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZBQSXYZ6EHEAHV67TTVGMDUJANCNFSM5TZW3W6A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[erikgb]

sgtm, I'd happily approve that and then release v1.0.1.

Nice, here is the cherry-pick PR: #207