Extended PrometheusK8sConfig and PrometheusRestrictedConfig with AdditionalAlertManagerConfigs #1132

dislbenn · 2021-04-27T01:55:23Z

I added CHANGELOG entry for this change.
No user-facing changes, so no entry in CHANGELOG was needed.

openshift-ci-robot · 2021-04-27T01:55:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dislbenn
To complete the pull request process, please assign dgrisonnet after the PR has been reviewed.
You can assign the PR to them by writing /assign @dgrisonnet in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

clyang82 · 2021-05-10T02:10:21Z

assets/prometheus-k8s/prometheus.yaml

@@ -39,6 +39,7 @@ spec:
  configMaps:
  - serving-certs-ca-bundle
  - kubelet-serving-ca-bundle
+  - hub-router-ca


I do not think you should add hub-router-ca here. I think you can make the cluster-monitoring-config configmap to support configuring configmap so that it can be able to update with customized configmap externally. I am not sure if it is correct approach. @simonpasquier can you give us recommendations? Thanks.

That's right, we don't want to hard-code an arbitrary CA.

simonpasquier · 2021-05-10T19:56:31Z

assets/prometheus-k8s/prometheus.yaml

@@ -39,6 +39,7 @@ spec:
  configMaps:
  - serving-certs-ca-bundle
  - kubelet-serving-ca-bundle
+  - hub-router-ca


That's right, we don't want to hard-code an arbitrary CA.

simonpasquier · 2021-05-10T20:22:01Z

pkg/manifests/config.go

+	VolumeClaimTemplate           *monv1.EmbeddedPersistentVolumeClaim `json:"volumeClaimTemplate"`
+	RemoteWrite                   []monv1.RemoteWriteSpec              `json:"remoteWrite"`
+	TelemetryMatches              []string                             `json:"-"`
+	AdditionalAlertManagerConfigs *v1.SecretKeySelector                `json:"additionalAlertManagerConfigs"`


Thinking more about the user experience here, I'm not sure that the configuration should just be a reference to an uncontrolled secret. The risk is high that the provided configuration is invalid and breaks Prometheus. Also not all fields of the alertmanager_config section are worth exposing (for instance configuring an external Alertmanager endpoint only needs the static_configs service discovery) and some might be not be usable in practice (for instance any field like ca_file that reference a file on disk).

It might be more appropriate to expose the main fields that are required to configure external Alertmanager endpoints. From there, CMO could generate the secret to be referenced in the Prometheus resource.

Here is the rough idea:

type PrometheusK8sConfig struct { ... AlertmanagerConfigs []AlertmanagerConfig `json:"additionalAlertManagerConfigs"` } type AlertmanagerConfig struct { # URL to the Alertmanager server. URL net.URL `json:"url"` Timeout string `json:"timeout"` # Reference to a key in a Secret containing the certificate authority to verify the server's certificate. CA *v1.SecretKeySelector # Authn parameters (to be defined) ... }

bjoydeep · 2021-05-12T05:43:41Z

@simonpasquier @dislbenn when the Prometheus on cluster A will post the alert to Alertmanager on the Hub via this, how will the user know from which cluster this came? in ACM metric collector, when we take in data from Prometheus on cluster A and send it to the Thanos API Gateway on the Hub, we do append the clusterid and clustername as labels. Is this happening already - may be I missed it in the code?

clyang82 · 2021-05-12T12:46:30Z

@simonpasquier @dislbenn when the Prometheus on cluster A will post the alert to Alertmanager on the Hub via this, how will the user know from which cluster this came? in ACM metric collector, when we take in data from Prometheus on cluster A and send it to the Thanos API Gateway on the Hub, we do append the clusterid and clustername as labels. Is this happening already - may be I missed it in the code?

@bjoydeep @morvencao is trying to use relabel_configs to add clusterid and clustername in managed cluster before send alerts to hub alertmanager.

morvencao · 2021-05-12T13:54:45Z

I also found another configuration AdditionalAlertRelabelConfigs that may resolve the relabel issue for alerts.
@simonpasquier may know more details.
see: https://github.com/openshift/prometheus-operator/blob/34a5b31452901e7d88240420f372969a01128691/pkg/apis/monitoring/v1/types.go#L299
Will try that and give feedback here.

clyang82 · 2021-05-16T08:05:15Z

/test e2e-aws-single-node

clyang82 · 2021-05-18T14:42:33Z

/assign @simonpasquier

simonpasquier · 2021-05-20T08:39:28Z

Regarding alert relabeling, I would expect that using external labels is enough for your use case?

clyang82 · 2021-05-20T08:43:57Z

Regarding alert relabeling, I would expect that using external labels is enough for your use case?

Yes. external labels is enough for our cases. Do you want to reduce the risk to not support alert relabeling right now?

simonpasquier · 2021-05-20T08:47:29Z

Do you want to reduce the risk to not support alert relabeling right now?

yes

clyang82 · 2021-05-20T08:53:02Z

Do you want to reduce the risk to not support alert relabeling right now?
yes

Can we keep using []*monv1.ProbeTargetStaticConfig and just ignore RelabelConfigs? or do you suggest to define a new object in CMO? Thanks.

clyang82 · 2021-05-20T14:38:49Z

/retest

Signed-off-by: clyang82 <chuyang@redhat.com>

clyang82 · 2021-06-04T02:11:25Z

/retest

simonpasquier · 2021-06-07T12:35:17Z

pkg/tasks/prometheus.go

+			if err != nil {
+				return errors.Wrap(err, "reconciling Prometheus additionalAlertManagerConfigs secret failed")
+			}
+		}


Late realization for me (sorry!) but we need to delete the secret in case the admin had configured additional Alertmanagers and then remove them all.

…gured Signed-off-by: clyang82 <chuyang@redhat.com>

simonpasquier · 2021-06-08T13:47:54Z

/test e2e-agnostic-upgrade

simonpasquier

/lgtm

This looks good from my end. The code changes are well isolated meaning that I'm confident that users that don't configure additional Alertmanagers have no risk of being impacted by the change.

pkg/manifests/config.go

CHANGELOG.md

clyang82 · 2021-06-09T01:00:30Z

/test e2e-agnostic-upgrade

openshift-bot · 2021-06-09T04:13:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-09T06:13:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-09T07:13:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-10T16:15:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-10T16:27:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-10T17:03:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Co-authored-by: Simon Pasquier <spasquie@redhat.com>

eparis · 2021-06-10T21:07:46Z

/lgtm
looks like just a rebase

bjoydeep · 2021-06-10T22:51:36Z

/test e2e-agnostic-upgrade

openshift-bot · 2021-06-10T23:39:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

simonpasquier · 2021-06-11T08:43:28Z

/lgtm

ping @eparis for overriding the bugzilla/valid-bug label

openshift-ci · 2021-06-11T08:43:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dislbenn, eparis, 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~~ [simonpasquier]

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 requested review from jan--f and paulfantom April 27, 2021 01:55

dislbenn marked this pull request as draft April 27, 2021 01:56

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 Apr 27, 2021

dislbenn changed the title ~~Modified PrometheusK8sConfig to allow a list AlertmanagerEndpoints~~ Modified PrometheusK8sConfig to allow a list AdditionalAlertManagerConfigs May 7, 2021

dislbenn marked this pull request as ready for review May 7, 2021 20:03

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2021

clyang82 reviewed May 10, 2021

View reviewed changes

dislbenn changed the title ~~Modified PrometheusK8sConfig to allow a list AdditionalAlertManagerConfigs~~ Modified PrometheusK8sConfig and PrometheusRestrictedConfig to allow a list AdditionalAlertManagerConfigs May 10, 2021

dislbenn changed the title ~~Modified PrometheusK8sConfig and PrometheusRestrictedConfig to allow a list AdditionalAlertManagerConfigs~~ Extended PrometheusK8sConfig and PrometheusRestrictedConfig with AdditionalAlertManagerConfigs May 10, 2021

simonpasquier reviewed May 10, 2021

View reviewed changes

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

clyang82 force-pushed the dbennett/alertmanager-configmap-extend branch from b8f96e7 to 3225923 Compare May 13, 2021 12:40

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2021

clyang82 force-pushed the dbennett/alertmanager-configmap-extend branch 2 times, most recently from a04a5fb to 96a91fd Compare May 14, 2021 09:57

clyang82 force-pushed the dbennett/alertmanager-configmap-extend branch 2 times, most recently from b4cb43f to ff5bfcc Compare May 17, 2021 09:45

openshift-ci bot assigned simonpasquier May 18, 2021

update comments

571a418

Signed-off-by: clyang82 <chuyang@redhat.com>

simonpasquier reviewed Jun 7, 2021

View reviewed changes

cleanup the secret if there is no additionalAlertManagerConfigs confi…

550fad0

…gured Signed-off-by: clyang82 <chuyang@redhat.com>

simonpasquier reviewed Jun 8, 2021

View reviewed changes

pkg/manifests/config.go Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 8, 2021

Update CHANGELOG.md

1a001ac

Co-authored-by: Simon Pasquier <spasquie@redhat.com>

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2021

dislbenn and others added 2 commits June 10, 2021 14:21

Update pkg/manifests/config.go

abffd9b

Co-authored-by: Simon Pasquier <spasquie@redhat.com>

Merge branch 'master' into dbennett/alertmanager-configmap-extend

d295760

openshift-ci bot assigned eparis Jun 10, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2021

eparis added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 11, 2021

openshift-merge-robot merged commit 7168290 into openshift:master Jun 11, 2021

clyang82 deleted the dbennett/alertmanager-configmap-extend branch June 11, 2021 11:42

fpetkovski mentioned this pull request Jul 9, 2021

Allow configuring additional alertmanagers for UWM Prometheus and Thanos #1271

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended PrometheusK8sConfig and PrometheusRestrictedConfig with AdditionalAlertManagerConfigs #1132

Extended PrometheusK8sConfig and PrometheusRestrictedConfig with AdditionalAlertManagerConfigs #1132

dislbenn commented Apr 27, 2021 •

edited

openshift-ci-robot commented Apr 27, 2021

clyang82 May 10, 2021

simonpasquier May 10, 2021

simonpasquier May 10, 2021

simonpasquier May 10, 2021

bjoydeep commented May 12, 2021

clyang82 commented May 12, 2021

morvencao commented May 12, 2021

clyang82 commented May 16, 2021

clyang82 commented May 18, 2021

simonpasquier commented May 20, 2021

clyang82 commented May 20, 2021

simonpasquier commented May 20, 2021 •

edited

clyang82 commented May 20, 2021

clyang82 commented May 20, 2021

clyang82 commented Jun 4, 2021

simonpasquier Jun 7, 2021

simonpasquier commented Jun 8, 2021

simonpasquier left a comment

clyang82 commented Jun 9, 2021

openshift-bot commented Jun 9, 2021

openshift-bot commented Jun 9, 2021

openshift-bot commented Jun 9, 2021

openshift-bot commented Jun 10, 2021

openshift-bot commented Jun 10, 2021

openshift-bot commented Jun 10, 2021

eparis commented Jun 10, 2021

bjoydeep commented Jun 10, 2021

openshift-bot commented Jun 10, 2021

simonpasquier commented Jun 11, 2021

openshift-ci bot commented Jun 11, 2021

Extended PrometheusK8sConfig and PrometheusRestrictedConfig with AdditionalAlertManagerConfigs #1132

Extended PrometheusK8sConfig and PrometheusRestrictedConfig with AdditionalAlertManagerConfigs #1132

Conversation

dislbenn commented Apr 27, 2021 • edited

openshift-ci-robot commented Apr 27, 2021

clyang82 May 10, 2021

Choose a reason for hiding this comment

simonpasquier May 10, 2021

Choose a reason for hiding this comment

simonpasquier May 10, 2021

Choose a reason for hiding this comment

simonpasquier May 10, 2021

Choose a reason for hiding this comment

bjoydeep commented May 12, 2021

clyang82 commented May 12, 2021

morvencao commented May 12, 2021

clyang82 commented May 16, 2021

clyang82 commented May 18, 2021

simonpasquier commented May 20, 2021

clyang82 commented May 20, 2021

simonpasquier commented May 20, 2021 • edited

clyang82 commented May 20, 2021

clyang82 commented May 20, 2021

clyang82 commented Jun 4, 2021

simonpasquier Jun 7, 2021

Choose a reason for hiding this comment

simonpasquier commented Jun 8, 2021

simonpasquier left a comment

Choose a reason for hiding this comment

clyang82 commented Jun 9, 2021

openshift-bot commented Jun 9, 2021

openshift-bot commented Jun 9, 2021

openshift-bot commented Jun 9, 2021

openshift-bot commented Jun 10, 2021

openshift-bot commented Jun 10, 2021

openshift-bot commented Jun 10, 2021

eparis commented Jun 10, 2021

bjoydeep commented Jun 10, 2021

openshift-bot commented Jun 10, 2021

simonpasquier commented Jun 11, 2021

openshift-ci bot commented Jun 11, 2021

dislbenn commented Apr 27, 2021 •

edited

simonpasquier commented May 20, 2021 •

edited