Add kube-rbac-proxy and prom-label-proxy to Alertmanager #701

simonpasquier · 2020-03-12T16:20:10Z

~~This needs e2e tests.~~

assets/alertmanager/alertmanager.yaml

s-urbaniak · 2020-03-13T09:57:05Z

just nits around resource requests, else lgtm module e2e tests 👍

lilic · 2020-03-13T14:29:34Z

Why do we need this PR? Is this for thanos ruler/querier integration with alertmanager? 🤔

s-urbaniak · 2020-03-13T17:02:33Z

@lilic xref https://github.com/openshift/enhancements/blob/master/enhancements/monitoring/user-workload-monitoring.md#alertmanager-silences

brancz · 2020-03-16T10:08:49Z

lgtm as well after e2e tests

s-urbaniak · 2020-03-16T11:28:55Z

@simonpasquier i believe it is unrelated but do you mind to peek at the e2e failure?

level=error msg="Cluster operator monitoring Degraded is True with UpdatingAlertmanagerFailed: Failed to rollout the stack. Error: running task Updating Alertmanager failed: waiting for Alertmanager object changes failed: waiting for Alertmanager: expected 3 replicas, updated 0 and available 0"
level=fatal msg="failed to initialize the cluster: Cluster operator monitoring is still updating"

pkg/manifests/manifests.go

lilic

Looks good!

lilic · 2020-03-26T13:40:51Z

manifests/0000_50_cluster_monitoring_operator_02-role.yaml

@@ -145,6 +147,25 @@ rules:
  - namespaces
  verbs:
  - get
+- apiGroups:


Why do we need this?

I'm not sure, it was automatically generated by the script...

cc @s-urbaniak @paulfantom

I see, I guess CMO needs this permissions as well, explains it.

pkg/manifests/manifests.go

pkg/tasks/clustermonitoringoperator.go

lilic · 2020-03-26T13:47:55Z

test/e2e/alertmanager_test.go

+}
+
+// The Alertmanager API should be protected by the OAuth proxy.
+func TestAlertmanagerOAuthProxy(t *testing.T) {


Can you explain how this is testing alertmanger oauth proxy, we just do a query against alertmanager.

We're querying Alertmanager via its OpenShift route which should be mapped to the OAuth proxy service. This is copied from

cluster-monitoring-operator/test/e2e/user_workload_monitoring_test.go

Lines 376 to 403 in e597c67

err = framework.Poll(5*time.Second, 5*time.Minute, func() error {

body, err := f.AlertmanagerClient.AlertmanagerQuery(

"filter", `alertname="VersionAlert"`,

"active", "true",

)

if err != nil {

t.Fatal(err)

}

res, err := gabs.ParseJSON(body)

if err != nil {

return err

}

count, err := res.ArrayCount()

if err != nil {

return err

}

if count == 1 {

return nil

}

return fmt.Errorf("expected 1 fired VersionAlert, got %d", count)

})

if err != nil {

t.Fatal(err)

}

I should probably add an e2e test checking that a request without sufficient permissions is denied.

brancz · 2020-03-26T13:55:46Z

test/e2e/alertmanager_test.go

+	if err != nil {
+		t.Fatalf("%v (data: %q)", err, string(b))
+	}
+	silID, ok := parsed.Path("silenceID").Data().(string)


let's additionally validate that the namespace label exists/is enforced no?

done, the original silence is created with a namespace="openshift-monitoring" matcher and the test verifies that it is modified to namespace="test-kube-rbac-proxy".

simonpasquier · 2020-03-26T14:25:28Z

/retest

simonpasquier · 2020-03-26T15:46:42Z

/test e2e-aws-operator

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

lilic

Thanks!

/lgtm
/hold

in case brancz has anymore questions.

lilic · 2020-03-27T10:02:44Z

manifests/0000_50_cluster_monitoring_operator_02-role.yaml

@@ -145,6 +147,25 @@ rules:
  - namespaces
  verbs:
  - get
+- apiGroups:


I see, I guess CMO needs this permissions as well, explains it.

openshift-ci-robot · 2020-03-27T12:28:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilic, simonpasquier

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:

~~OWNERS~~ [lilic,simonpasquier]

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

simonpasquier · 2020-03-27T13:15:14Z

Thanks Lili! cc @brancz

simonpasquier · 2020-03-30T08:58:52Z

/hold cancel

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2020

openshift-ci-robot requested review from s-urbaniak and squat March 12, 2020 16:20

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2020

pgier mentioned this pull request Mar 12, 2020

Update jsonnet bundler v0.3.1 #703

Merged

s-urbaniak reviewed Mar 13, 2020

View reviewed changes

assets/alertmanager/alertmanager.yaml Outdated Show resolved Hide resolved

s-urbaniak reviewed Mar 13, 2020

View reviewed changes

assets/alertmanager/alertmanager.yaml Outdated Show resolved Hide resolved

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch 2 times, most recently from 7465b88 to b241d5e Compare March 13, 2020 14:21

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2020

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch from b241d5e to a7191e3 Compare March 16, 2020 08:55

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2020

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch 11 times, most recently from 79990c6 to 6850c49 Compare March 25, 2020 13:01

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch from 6850c49 to e6df898 Compare March 25, 2020 14:37

simonpasquier changed the title ~~[WIP] Add rbac and label proxy to alertmanager~~ Add rbac and label proxy to alertmanager Mar 25, 2020

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 Mar 25, 2020

simonpasquier changed the title ~~Add rbac and label proxy to alertmanager~~ Add kube-rbac-proxy and prom-label-proxy to alertmanager Mar 25, 2020

simonpasquier changed the title ~~Add kube-rbac-proxy and prom-label-proxy to alertmanager~~ Add kube-rbac-proxy and prom-label-proxy to Alertmanager Mar 25, 2020

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch from ebfe47c to 001fa60 Compare March 26, 2020 09:36

brancz reviewed Mar 26, 2020

View reviewed changes

pkg/manifests/manifests.go Outdated Show resolved Hide resolved

lilic reviewed Mar 26, 2020

View reviewed changes

brancz reviewed Mar 26, 2020

View reviewed changes

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch 2 times, most recently from df2bf16 to f4f37b3 Compare March 26, 2020 15:07

simonpasquier added 6 commits March 27, 2020 10:38

jsonnet,pkg: add kube-rbac-proxy and prom-label-proxy to Alertmanager

4231d68

assets,pkg/manifests: regenerate

5a11742

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

test/e2e: add test for Alertmanager proxies

db94828

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

pkg/manifests: use container names instead of indices

17319b8

test/e2e/framework: fix comment

88e33ba

test/e2e/framework: fix closing of HTTP requests

655bfcf

simonpasquier force-pushed the add-rbac-and-label-proxy-to-alertmanager branch from f4f37b3 to 655bfcf Compare March 27, 2020 09:38

lilic reviewed Mar 27, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2020

openshift-ci-robot assigned lilic Mar 27, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 30, 2020

openshift-merge-robot merged commit 6791732 into openshift:master Mar 30, 2020

pgier mentioned this pull request Jun 23, 2020

Bug 1813221: pkg/manifests: use alertmanager container names instead of index #819

Closed

1 task

simonpasquier deleted the add-rbac-and-label-proxy-to-alertmanager branch June 24, 2020 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kube-rbac-proxy and prom-label-proxy to Alertmanager #701

Add kube-rbac-proxy and prom-label-proxy to Alertmanager #701

simonpasquier commented Mar 12, 2020 •

edited

s-urbaniak commented Mar 13, 2020

lilic commented Mar 13, 2020 •

edited

s-urbaniak commented Mar 13, 2020

brancz commented Mar 16, 2020

s-urbaniak commented Mar 16, 2020

lilic left a comment

lilic Mar 26, 2020

simonpasquier Mar 26, 2020

lilic Mar 27, 2020

lilic Mar 26, 2020

simonpasquier Mar 26, 2020

brancz Mar 26, 2020 •

edited

simonpasquier Mar 27, 2020

simonpasquier commented Mar 26, 2020

simonpasquier commented Mar 26, 2020

lilic left a comment

lilic Mar 27, 2020

openshift-ci-robot commented Mar 27, 2020

simonpasquier commented Mar 27, 2020

simonpasquier commented Mar 30, 2020

	err = framework.Poll(5time.Second, 5time.Minute, func() error {
	body, err := f.AlertmanagerClient.AlertmanagerQuery(
	"filter", `alertname="VersionAlert"`,
	"active", "true",
	)
	if err != nil {
	t.Fatal(err)
	}

	res, err := gabs.ParseJSON(body)
	if err != nil {
	return err
	}

	count, err := res.ArrayCount()
	if err != nil {
	return err
	}

	if count == 1 {
	return nil
	}

	return fmt.Errorf("expected 1 fired VersionAlert, got %d", count)
	})
	if err != nil {
	t.Fatal(err)
	}

Add kube-rbac-proxy and prom-label-proxy to Alertmanager #701

Add kube-rbac-proxy and prom-label-proxy to Alertmanager #701

Conversation

simonpasquier commented Mar 12, 2020 • edited

s-urbaniak commented Mar 13, 2020

lilic commented Mar 13, 2020 • edited

s-urbaniak commented Mar 13, 2020

brancz commented Mar 16, 2020

s-urbaniak commented Mar 16, 2020

lilic left a comment

Choose a reason for hiding this comment

lilic Mar 26, 2020

Choose a reason for hiding this comment

simonpasquier Mar 26, 2020

Choose a reason for hiding this comment

lilic Mar 27, 2020

Choose a reason for hiding this comment

lilic Mar 26, 2020

Choose a reason for hiding this comment

simonpasquier Mar 26, 2020

Choose a reason for hiding this comment

brancz Mar 26, 2020 • edited

Choose a reason for hiding this comment

simonpasquier Mar 27, 2020

Choose a reason for hiding this comment

simonpasquier commented Mar 26, 2020

simonpasquier commented Mar 26, 2020

lilic left a comment

Choose a reason for hiding this comment

lilic Mar 27, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 27, 2020

simonpasquier commented Mar 27, 2020

simonpasquier commented Mar 30, 2020

simonpasquier commented Mar 12, 2020 •

edited

lilic commented Mar 13, 2020 •

edited

brancz Mar 26, 2020 •

edited