Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MON-1631: prometheus: remove ui access #1532

Merged
merged 1 commit into from
Feb 3, 2022
Merged

MON-1631: prometheus: remove ui access #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions assets/prometheus-k8s/route.yaml → assets/prometheus-k8s/api-route.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ metadata:
name: prometheus-k8s
namespace: openshift-monitoring
spec:
path: /api
port:
targetPort: web
tls:
Expand Down
15 changes: 15 additions & 0 deletions assets/prometheus-k8s/federate-route.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
apiVersion: v1
kind: Route
metadata:
name: prometheus-k8s-federate
namespace: openshift-monitoring
spec:
path: /federate
port:
targetPort: web
tls:
insecureEdgeTerminationPolicy: Redirect
termination: Reencrypt
to:
kind: Service
name: prometheus-k8s
1 change: 1 addition & 0 deletions assets/prometheus-k8s/prometheus.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ spec:
memory: 10Mi
enableFeatures: []
externalLabels: {}
externalURL: https://prometheus-k8s.openshift-monitoring.svc:9091
image: quay.io/prometheus/prometheus:v2.32.1
listenLocal: true
nodeSelector:
Expand Down
30 changes: 28 additions & 2 deletions jsonnet/components/prometheus.libsonnet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ function(params)
data: {},
},

// OpenShift route to access the Prometheus UI.
route: {
// OpenShift route to access the Prometheus api.
apiRoute: {
apiVersion: 'v1',
kind: 'Route',
metadata: {
Expand All @@ -35,6 +35,31 @@ function(params)
kind: 'Service',
name: 'prometheus-k8s',
},
path: '/api',
port: {
targetPort: 'web',
},
tls: {
termination: 'Reencrypt',
insecureEdgeTerminationPolicy: 'Redirect',
},
},
},

// OpenShift route to access the Prometheus federate endpoint.
federateRoute: {
apiVersion: 'v1',
kind: 'Route',
metadata: {
name: 'prometheus-k8s-federate',
namespace: cfg.namespace,
},
spec: {
to: {
kind: 'Service',
name: 'prometheus-k8s',
},
path: '/federate',
port: {
targetPort: 'web',
},
Expand Down Expand Up @@ -316,6 +341,7 @@ function(params)
'kube-rbac-proxy',
'metrics-client-certs',
],
externalURL: 'https://prometheus-k8s.openshift-monitoring.svc:9091',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh alerts, of course. Yeah the console route makes sense. Will add that and ping @kyoto is there are any objections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @kyoto, lets try it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR for handling the Console redirect: openshift/console#10963

configMaps: ['serving-certs-ca-bundle', 'kubelet-serving-ca-bundle', 'metrics-client-ca'],
probeNamespaceSelector: cfg.namespaceSelector,
podMonitorNamespaceSelector: cfg.namespaceSelector,
Expand Down
33 changes: 20 additions & 13 deletions pkg/manifests/manifests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ var (
PrometheusK8sProxySecret = "prometheus-k8s/proxy-secret.yaml"
PrometheusRBACProxySecret = "prometheus-k8s/kube-rbac-proxy-secret.yaml"
PrometheusUserWorkloadRBACProxySecret = "prometheus-user-workload/kube-rbac-proxy-secret.yaml"
PrometheusK8sRoute = "prometheus-k8s/route.yaml"
PrometheusK8sAPIRoute = "prometheus-k8s/api-route.yaml"
PrometheusK8sFederateRoute = "prometheus-k8s/federate-route.yaml"
PrometheusK8sHtpasswd = "prometheus-k8s/htpasswd-secret.yaml"
PrometheusK8sServingCertsCABundle = "prometheus-k8s/serving-certs-ca-bundle.yaml"
PrometheusK8sKubeletServingCABundle = "prometheus-k8s/kubelet-serving-ca-bundle.yaml"
Expand Down Expand Up @@ -323,14 +324,6 @@ func NewFactory(namespace, namespaceUserWorkload string, c *Config, infrastructu
}
}

func (f *Factory) PrometheusExternalURL(host string) *url.URL {
return &url.URL{
Scheme: "https",
Host: host,
Path: "/",
}
}

func (f *Factory) AlertmanagerExternalURL(host string) *url.URL {
return &url.URL{
Scheme: "https",
Expand Down Expand Up @@ -1344,8 +1337,19 @@ func (f *Factory) PrometheusK8sThanosSidecarServiceMonitor() (*monv1.ServiceMoni
return s, nil
}

func (f *Factory) PrometheusK8sRoute() (*routev1.Route, error) {
r, err := f.NewRoute(f.assets.MustNewAssetReader(PrometheusK8sRoute))
func (f *Factory) PrometheusK8sAPIRoute() (*routev1.Route, error) {
r, err := f.NewRoute(f.assets.MustNewAssetReader(PrometheusK8sAPIRoute))
if err != nil {
return nil, err
}

r.Namespace = f.namespace

return r, nil
}

func (f *Factory) PrometheusK8sFederateRoute() (*routev1.Route, error) {
r, err := f.NewRoute(f.assets.MustNewAssetReader(PrometheusK8sFederateRoute))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1405,7 +1409,7 @@ func (f *Factory) PrometheusK8sTrustedCABundle() (*v1.ConfigMap, error) {
return cm, nil
}

func (f *Factory) PrometheusK8s(host string, grpcTLS *v1.Secret, trustedCABundleCM *v1.ConfigMap) (*monv1.Prometheus, error) {
func (f *Factory) PrometheusK8s(grpcTLS *v1.Secret, trustedCABundleCM *v1.ConfigMap) (*monv1.Prometheus, error) {
p, err := f.NewPrometheus(f.assets.MustNewAssetReader(PrometheusK8s))
if err != nil {
return nil, err
Expand All @@ -1420,7 +1424,10 @@ func (f *Factory) PrometheusK8s(host string, grpcTLS *v1.Secret, trustedCABundle
}

p.Spec.Image = &f.config.Images.Prometheus
p.Spec.ExternalURL = f.PrometheusExternalURL(host).String()

if f.consoleConfig != nil {
p.Spec.ExternalURL = f.consoleConfig.Status.ConsoleURL + "/monitoring"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)

Suggested change
p.Spec.ExternalURL = f.consoleConfig.Status.ConsoleURL + "/monitoring"
p.Spec.ExternalURL = path.Join(f.consoleConfig.Status.ConsoleURL, "monitoring")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Makes sense yes, any objections against a followup PR? The manifest creation for Alertmanager has the same code and could use the same fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not at all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more robust than the simple + 👍

}

if f.config.ClusterMonitoringConfiguration.PrometheusK8sConfig.Resources != nil {
p.Spec.Resources = *f.config.ClusterMonitoringConfiguration.PrometheusK8sConfig.Resources
Expand Down
6 changes: 1 addition & 5 deletions pkg/manifests/manifests_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ func TestUnconfiguredManifests(t *testing.T) {
t.Fatal(err)
}

_, err = f.PrometheusK8s("prometheus-k8s.openshift-monitoring.svc", &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}}, nil)
_, err = f.PrometheusK8s(&v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}}, nil)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -1005,7 +1005,6 @@ func TestPrometheusK8sRemoteWrite(t *testing.T) {

f := NewFactory("openshift-monitoring", "openshift-user-workload-monitoring", c, defaultInfrastructureReader(), &fakeProxyReader{}, NewAssets(assetsPath), &APIServerConfig{}, &configv1.Console{})
p, err := f.PrometheusK8s(
"prometheus-k8s.openshift-monitoring.svc",
&v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
&v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
)
Expand Down Expand Up @@ -1068,7 +1067,6 @@ ingress:

f := NewFactory("openshift-monitoring", "openshift-user-workload-monitoring", c, defaultInfrastructureReader(), &fakeProxyReader{}, NewAssets(assetsPath), &APIServerConfig{}, &configv1.Console{})
p, err := f.PrometheusK8s(
"prometheus-k8s.openshift-monitoring.svc",
&v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
&v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
)
Expand Down Expand Up @@ -1384,7 +1382,6 @@ func TestPrometheusK8sAdditionalAlertManagerConfigsSecret(t *testing.T) {
f := NewFactory("openshift-monitoring", "openshift-user-workload-monitoring", c, defaultInfrastructureReader(), &fakeProxyReader{}, NewAssets(assetsPath), &APIServerConfig{}, &configv1.Console{})

p, err := f.PrometheusK8s(
"prometheus-k8s.openshift-monitoring.svc",
&v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
&v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
)
Expand Down Expand Up @@ -2640,7 +2637,6 @@ func TestNonHighlyAvailableInfrastructure(t *testing.T) {
name: "Prometheus",
getSpec: func(f *Factory) (spec, error) {
p, err := f.PrometheusK8s(
"prometheus-k8s.openshift-monitoring.svc",
&v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
&v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}},
)
Expand Down
2 changes: 1 addition & 1 deletion pkg/tasks/configsharing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func NewConfigSharingTask(client *client.Client, factory *manifests.Factory, con
}

func (t *ConfigSharingTask) Run(ctx context.Context) error {
promRoute, err := t.factory.PrometheusK8sRoute()
promRoute, err := t.factory.PrometheusK8sAPIRoute()
if err != nil {
return errors.Wrap(err, "initializing Prometheus Route failed")
}
Expand Down
27 changes: 21 additions & 6 deletions pkg/tasks/prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,34 @@ func (t *PrometheusTask) Run(ctx context.Context) error {
return errors.Wrap(err, "creating kubelet serving CA Bundle ConfigMap failed")
}

r, err := t.factory.PrometheusK8sRoute()
r, err := t.factory.PrometheusK8sAPIRoute()
if err != nil {
return errors.Wrap(err, "initializing Prometheus Route failed")
return errors.Wrap(err, "initializing Prometheus API Route failed")
}

err = t.client.CreateRouteIfNotExists(ctx, r)
if err != nil {
return errors.Wrap(err, "creating Prometheus Route failed")
return errors.Wrap(err, "creating Prometheus API Route failed")
}

host, err := t.client.WaitForRouteReady(ctx, r)
_, err = t.client.WaitForRouteReady(ctx, r)
if err != nil {
return errors.Wrap(err, "waiting for Prometheus Route to become ready failed")
return errors.Wrap(err, "waiting for Prometheus API Route to become ready failed")
}

fr, err := t.factory.PrometheusK8sFederateRoute()
if err != nil {
return errors.Wrap(err, "initializing Prometheus Federate Route failed")
}

err = t.client.CreateRouteIfNotExists(ctx, fr)
if err != nil {
return errors.Wrap(err, "creating Prometheus Federate Route failed")
}

_, err = t.client.WaitForRouteReady(ctx, fr)
if err != nil {
return errors.Wrap(err, "waiting for Prometheus Federate Route to become ready failed")
}

ps, err := t.factory.PrometheusK8sProxySecret()
Expand Down Expand Up @@ -365,7 +380,7 @@ func (t *PrometheusTask) Run(ctx context.Context) error {
}

klog.V(4).Info("initializing Prometheus object")
p, err := t.factory.PrometheusK8s(host, s, trustedCA)
p, err := t.factory.PrometheusK8s(s, trustedCA)
if err != nil {
return errors.Wrap(err, "initializing Prometheus object failed")
}
Expand Down