-
Notifications
You must be signed in to change notification settings - Fork 603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update console for monitoring changes #909
Update console for monitoring changes #909
Conversation
Currently blocked because
|
The serving certs controller works a bit differently now. The service-ca.crt is not mounted into the serviceaccount secrets anymore, you need to create a configmap that has a special annotation, the configmap will then be populated with the ca.crt automatically by the serving certs controller. |
@benjaminapetersen fyi, this affects the operator |
@brancz I'm having trouble connecting to 9092 from inside the pod. Any ideas? 9091 is fine. This is a 4.0 install.
|
@spadgett the error |
@s-urbaniak I thought so, too, but I can reach the same service on port 9091. Just not 9092. (The service does have a port 9092 defined.) |
@spadgett you might be right 🤔 When I look at the service I see it has a But that port is not declared in the container: https://github.com/openshift/cluster-monitoring-operator/blob/1322e56e961511994a4a1a5ef55152d3b389575c/assets/prometheus-k8s/prometheus.yaml#L67-L77 cc @metalmatze for verification |
@spadgett good catch, we verified it's the missing port name, submitting a fix to the cluster monitoring operator as we speak. |
@spadgett once openshift/cluster-monitoring-operator#183 is merged and the images are rebuilt, you can retry :-) |
@s-urbaniak great, thank you! |
I'm still struggling to get port 9092 working. 9091 seems to work. This is from inside the console pod.
The decoded query here is
|
I pulled out the service-ca.crt changes into a separate PR, #960. It will require console operator changes as well to pass the service-ca file path on startup. |
It seems like the request still can't get through the kube-rbac-proxy: I would like to help you debug this. Is there any easy way to work on this? Is a normal OpenShift 4 cluster enough? Do I need a patched version? |
Sorry, I meant to get back to you on this. This is a normal OpenShift 4.0 cluster installed from the 0.7.0 installer. I'm logged in as the |
@spadgett @metalmatze I gave this a debug session and found that the only missing piece was the Given the kube-rbac proxy configuration in openshift (check with If I take your request from above and just modify it slightly, adding a
|
Thank you for looking into it! |
@s-urbaniak Thanks, I see now. I didn't realize |
No worries. That additional separate |
For queries that should NOT have a namespace, do we need to use service port 9091 instead? For example: https://github.com/openshift/console/blob/master/frontend/public/components/cluster-overview.jsx#L49 |
yes, for "cluster wide" metrics the existing port should continue to be used |
I'm seeing certificate errors connecting to port 9092 using service-ca.crt. It works OK for 9091.
|
Sorry, wrong hostname above, but I still see the error using the correct host. (No error using port 9091.)
|
Yeah, I don't see the kube-rbac-proxy container doing anything with the service serving certificate. I opened issue MON-511. |
Yes you're right, we'll try and fix that as soon as we can! Sorry for the inconvenience. |
8decd18
to
84506a5
Compare
The proxy is working for me now 👍 |
/hold |
jenkins rebuild |
1 similar comment
jenkins rebuild |
84506a5
to
e56bac7
Compare
e56bac7
to
cc36ec4
Compare
/assign @kyoto |
Stale element flake /retest
|
OLM test flake /retest
|
Tests are green, and I've been able to validate metrics work for a normal user by patching |
{ | ||
name: 'Used', | ||
<Line title="Memory Usage" namespace={ns.metadata.name} query={[ | ||
{ name: 'Used', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Was this newline accidentally removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
// In OpenShift, the user must be able to list namespaces to query Prometheus. | ||
return canListNS; | ||
}; | ||
const canAccessPrometheus = (prometheusFlag) => prometheusFlag && !!prometheusBasePath && !!prometheusTenancyBasePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, this will prevent all requirePrometheus()
wrapped components from rendering unless both prometheusBasePath
and prometheusTenancyBasePath
are set. Is that what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was trying to keep the client logic simple. Currently, the console serve sets both values together, so if one is set the other will always be, too. We assume the RBAC proxy will always be there in OpenShift 4.0. Let me know if that's not the case.
@spadgett LGTM apart from one nit and one question. |
* Proxy to port 9092, which has the tenancy proxy in front of it * Remove the `CAN_LIST_NS` check since users can now see metrics in their own namespaces https://jira.coreos.com/browse/CONSOLE-1035
cc36ec4
to
7ccd37e
Compare
The rbac proxy is always there in 4.0. It's part of the Prometheus pod that also answers the non-tenancy requests, so this sounds good to me. |
/lgtm |
CAN_LIST_NS
check since users can now see metrics in their own namespaceshttps://jira.coreos.com/browse/CONSOLE-1035
@kyoto @brancz
/hold