Skip to content

Conversation

@yaacov
Copy link
Member

@yaacov yaacov commented Aug 29, 2019

Description:
It will be nice to have an option to proxy prometheus from outside the cluster for devel.

This PR adds the option to proxy Prometheus and AlertManager when running outside the
cluster.

It adds new CLI options:

./bin/bridge --help
...
  -k8s-mode-off-cluster-alertmanager string
    	DEV ONLY. URL of the cluster's AlertManager server.
  -k8s-mode-off-cluster-prometheus string
    	DEV ONLY. URL of the cluster's Pormetheus server.

...

Usage:
Already integrated when running dev scripts:

source ./contrib/oc-environment.sh
./bin/bridge

And

./examples/run-bridge.sh

Ref:
https://docs.google.com/document/d/1epyQuj0AqL77zCLuea6T6WUKq-v47NbT9v24pOBFS1A/edit

https://jira.coreos.com/browse/CONSOLE-737

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2019
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. component/backend Related to backend labels Aug 29, 2019
@yaacov
Copy link
Member Author

yaacov commented Aug 29, 2019

@dtaylor113 Hi,
This is a way to get data from prometheus when running is dev env.
How do you run pages [1] that need to get data from prometheus on dev env ?

[1] #2497

@yaacov
Copy link
Member Author

yaacov commented Aug 29, 2019

@rawagner @suomiy @mareklibra @vojtechszocs @spadgett Hi,

Is there a way to get data from Promethean/AlertManger in devel mode ? what am I missing ?

@yaacov
Copy link
Member Author

yaacov commented Aug 29, 2019

/test e2e-aws

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 29, 2019
README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should recommend people set this even for development. It's too easy to forget you have this disabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 removed, we now use proxy to get the data, no CORS 🎉

Copy link
Member

Choose a reason for hiding this comment

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

Can we have an off-cluster prometheus URL and proxy to that instead of the service? That more closely mirrors a real environment and avoids the same origin problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Thanks !

Copy link
Member Author

Choose a reason for hiding this comment

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

use the proxy now 🎂 🍻 🍰

@spadgett
Copy link
Member

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2019
@yaacov yaacov force-pushed the bypass-monitoring-proxy-for-dev branch from 893f363 to 81f3106 Compare August 30, 2019 04:49
@yaacov yaacov changed the title [WIP] Bypass monitoring proxy for dev Bypass monitoring proxy for dev Aug 30, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2019
@yaacov yaacov changed the title Bypass monitoring proxy for dev Optional proxy of off cluster Prometheus. Aug 30, 2019
@yaacov
Copy link
Member Author

yaacov commented Aug 30, 2019

/test e2e-aws-console-olm
/test e2e-aws

@yaacov
Copy link
Member Author

yaacov commented Aug 30, 2019

/test e2e-aws-console-olm

@yaacov yaacov changed the title Optional proxy of off cluster Prometheus. Optional proxy of Prometheus from outside the cluster Aug 30, 2019
Copy link
Member

Choose a reason for hiding this comment

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

Typo: Prmetheus -> Prometheus

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks !

Copy link
Member

@kyoto kyoto Aug 30, 2019

Choose a reason for hiding this comment

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

Typo: clusters -> cluster's (and same in the line below)

Copy link
Member Author

Choose a reason for hiding this comment

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

I should wait with fixing :-)

@yaacov
Copy link
Member Author

yaacov commented Aug 30, 2019

@spadgett hi, removed the need to use un secure browser, now using proxy, please re-review.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for making this work

Copy link
Member

Choose a reason for hiding this comment

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

You can avoid the jq requirement with

Suggested change
export BRIDGE_K8S_MODE_OFF_CLUSTER_PROMETHEUS=$(oc -n openshift-monitoring get configmap sharing-config -o json | jq .data.prometheusURL | tr -d '"')
export BRIDGE_K8S_MODE_OFF_CLUSTER_PROMETHEUS=$(oc -n openshift-monitoring get configmap sharing-config -o jsonpath='{.data.prometheusURL}')

(same for below)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the proxy config still set? Is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, my bad ... fixing 👍

@spadgett
Copy link
Member

lgtm, just need to squash. thanks!

@yaacov yaacov force-pushed the bypass-monitoring-proxy-for-dev branch from 3724d04 to ab163e2 Compare August 30, 2019 16:06
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

👍

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spadgett, yaacov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2019
@yaacov
Copy link
Member Author

yaacov commented Aug 30, 2019

@spadgett do-not-merge/hold label ?

@spadgett
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2019
@spadgett
Copy link
Member

/retest

@yaacov
Copy link
Member Author

yaacov commented Aug 30, 2019

/test e2e-aws-console
/test e2e-aws

@yaacov
Copy link
Member Author

yaacov commented Aug 30, 2019

/test e2e-aws-console

@spadgett spadgett added this to the v4.2 milestone Aug 30, 2019
@yaacov
Copy link
Member Author

yaacov commented Aug 31, 2019

/test e2e-aws-console-olm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/backend Related to backend lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants