MON-1631: prometheus: remove ui access #1532
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
approved
jan--f
changed the title
jan--f
force-pushed
the
remove-prometheus-ui-access
branch
6 times, most recently
from
4bedb31
to
ba49ce4
Compare
| @@ -42,14 +42,16 @@ spec: | |||
| - metrics-client-ca | |||
| containers: | |||
| - args: | |||
| - -request-logging=true | |||
There was a problem hiding this comment.
Why request-logging is needed in this case?
There was a problem hiding this comment.
It's just for debugging :) I can't figure out why oauth-proxy isn't authorizing the requests I expect it to.
jan--f
force-pushed
the
remove-prometheus-ui-access
branch
3 times, most recently
from
46660e9
to
7f776c1
Compare
jan--f
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
1 similar comment
|
/retest |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| @@ -366,7 +366,7 @@ function(params) | |||
| '-upstream=http://localhost:9090', | |||
| '-openshift-service-account=prometheus-k8s', | |||
| '-openshift-sar={"resource": "namespaces", "verb": "get"}', | |||
| '-openshift-delegate-urls={"/": {"resource": "namespaces", "verb": "get"}}', | |||
| '-openshift-delegate-urls={"/api": {"resource": "namespaces", "verb": "get"},"/metrics": {"resource": "namespaces", "verb": "get"},"/federate": {"resource": "namespaces", "verb": "get"}}', | |||
There was a problem hiding this comment.
i'm not sure that we need to declare /metrics since the metrics should be scraped from the kube-rbac-proxy container?
There was a problem hiding this comment.
I went by what the functional tests look for. https://github.com/openshift/origin/blob/master/test/extended/prometheus/prometheus.go#L546 checks for /metrics on port 9091.
There was a problem hiding this comment.
Ok, can you add a TODO comment to be fixed once 4.11 opens?
| @@ -366,7 +366,7 @@ function(params) | |||
| '-upstream=http://localhost:9090', | |||
| '-openshift-service-account=prometheus-k8s', | |||
| '-openshift-sar={"resource": "namespaces", "verb": "get"}', | |||
There was a problem hiding this comment.
Should we remove this line to disable login via OAuth?
jan--f
force-pushed
the
remove-prometheus-ui-access
branch
4 times, most recently
from
86d9e72
to
6ba0862
Compare
| @@ -316,6 +341,7 @@ function(params) | |||
| 'kube-rbac-proxy', | |||
| 'metrics-client-certs', | |||
| ], | |||
| externalURL: 'https://prometheus-k8s.openshift-monitoring.svc:9091', | |||
There was a problem hiding this comment.
hmm I missed this detail 🤔...
Ideally I would expect that we link back to the OCP console but the generated URL is hardcoded to <externalURL>/graph?g0.expr=up&g0.tab=1 which wouldn't work for the console. Maybe @kyoto has some ideas?
There was a problem hiding this comment.
Not sure I follow...link back from where? Afaiu this is what the Prometheus UI uses internally. With this PR it should only be accessible through the Prometheus Service, so setting this as the external URL should suffice.
There was a problem hiding this comment.
Sorry for the missing details :)
When Prometheus sends alerts to Alertmanager, the payload includes a generatorURL field that is a back-link to the Prometheus UI (see https://prometheus.io/docs/alerting/latest/clients/). The URL is constructed by concatenating --web.external-url and /graph?g0.expr=<alert expression>&g0.tab=1 (see here and here).
My thinking is that if the generator URL would link to the console route and the console redirects to the "Observe > Metrics" page... The other option would be to discuss upstream if it would be possible to have a full customization of the generator URL (not sure if it makes sense in general).
There was a problem hiding this comment.
Ahh alerts, of course. Yeah the console route makes sense. Will add that and ping @kyoto is there are any objections.
There was a problem hiding this comment.
Adding a Console redirect sounds like a reasonable workaround to me.
Can we have the generator URL go directly to <Console URL>/monitoring in the Console? Then, if I understand correctly, the constructed URL would be <Console URL>/monitoring/graph?g0.expr=<alert expression>&g0.tab=1, which Console would then redirect to <Console URL>/monitoring/query-browser?query0=<alert expression>?
There was a problem hiding this comment.
Looks good @kyoto, lets try it.
There was a problem hiding this comment.
PR for handling the Console redirect: openshift/console#10963
jan--f
force-pushed
the
remove-prometheus-ui-access
branch
3 times, most recently
from
c699ae3
to
e542aeb
Compare
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
25 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@jan--f: The following test 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. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Signed-off-by: Jan Fajerski [email protected]