Use kube-rbac-proxy for standalone kube-proxy metrics #839
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
approved
danwinship
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @juanluisvaladas |
|
Can you think of a cleverer way to roll this out such that all clusters get this? There would be a bit of scraping failures as the ServiceMonitor and Daemonset race, but my instinct is that's not much of a problem. (kube-proxy monitoring is nice but not stupendously critical). |
|
I don't really know a lot about monitoring, and I don't have much sense of what is and is not breaking. |
|
Given that we don't have any alerts defined for kube-proxy (oops, we should, I'll file a ticket) then I think it's safe to just roll this out and accept a bit of disruption. I've asked the monitoring folks either way. |
|
Yeah, TargetDown fires if scrape fails for more than 10 minutes; I think we don't need the affordance for old clusters. |
#819 adds them but I didn't want to merge that until we had CI for standalone kube-proxy (openshift/release#12502) which is blocked by this because we fail "Prometheus when installed on the cluster should start and expose a secured proxy and unsecured metrics" |
We don't allow the user to change the SDN metrics port, but we historically allowed them to manually specify the correct port (9101) explicitly in the kube-proxy config. However, this got broken when we switched to using the https proxy for metrics; we need to be telling kube-proxy to use port 29101 now, but if the user had specified 9101 explicitly in the config, we would have told it to use that instead and broken everything.
Don't allow the user to explicitly specify the health/metrics port values. While we have to continue to allow them to specify the old ones if they were already doing it, for backward compatibility, don't encourage it. Also, fix the bug that we were now allowing openshift-sdn users to choose the standalone-kube-proxy ports, though it wouldn't actually work right. Also, revert a change to kubeProxyConfiguration() that just made things messy and doesn't work well with the next commit.
danwinship
force-pushed
the
kube-proxy-rbac-proxy
branch
from
1a338eb
to
9c683fb
Compare
|
@squeed fixed to unconditionally deploy kube-rbac-proxy. I had to keep the possibility of continuing to use the old port though, since we allow the user to explicitly request that port in the operator config and it seems wrong to not use that port in that case? (But cleaned up some other stuff around specifying explicit ports in the kube-proxy config...) |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, squeed 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:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
@danwinship: The following tests failed, say
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. |
|
/override ci/prow/e2e-aws-sdn-multi |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@danwinship: Overrode contexts on behalf of danwinship: ci/prow/e2e-aws-sdn-multi In response to this:
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. |
As with openshift-sdn and ovn-kubernetes, we should run the standalone kube-proxy metrics behind an authenticated proxy.
To avoid possible problems with upgrading old clusters, I made it conditional on using the new metrics port.