Webex Feature/API #5305

eumel8 · 2023-01-26T09:05:31Z

Signed-off-by: Frank Kloeker f.kloeker@telekom.de

Description

Prometheus Alertmanager brings the functionality of Webex Notification. Start of Operator implementation was here
This is the part of API/CRD extension

Type of change

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

ref:#5295

Changelog entry

Add Webex configs to AlertmanagerConfig CRD

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

eumel8 · 2023-01-26T09:05:57Z

the bundle version needs to bump up

xiu · 2023-01-26T09:42:00Z

Great!

@eumel8 you can generate the bundle by using make generate. To help with formatting as well, make format and to check the formatting of the docs, make check-docs (https://github.com/prometheus-operator/prometheus-operator#contributing).

I see your tests are not passing, make test-unit can help there (https://github.com/prometheus-operator/prometheus-operator#testing).

simonpasquier · 2023-01-26T11:09:47Z

Thanks for starting this! You'd need to update https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/apis/monitoring/v1beta1/conversion_from.go and https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/apis/monitoring/v1beta1/conversion_to.go to convert between v1alpha1 and v1beta1 types.

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go

simonpasquier · 2023-01-26T11:12:17Z

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go

+// WebexConfig configures notification via Cisco Webex
+// See https://prometheus.io/docs/alerting/latest/configuration/#webex_config
+type WebexConfig struct {
+	SendResolved *bool       `json:"sendResolved,omitempty"`


Suggested change

SendResolved *bool `json:"sendResolved,omitempty"`

// Whether to notify about resolved alerts.

SendResolved *bool `json:"sendResolved,omitempty"`

simonpasquier · 2023-01-26T11:13:16Z

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go

+// See https://prometheus.io/docs/alerting/latest/configuration/#webex_config
+type WebexConfig struct {
+	SendResolved *bool       `json:"sendResolved,omitempty"`
+	APIURL       string      `json:"apiURL,omitempty"`


Suggested change

APIURL string `json:"apiURL,omitempty"`

// The Webex Teams API URL if different from the default API URL.

APIURL string `json:"apiURL,omitempty"`

simonpasquier · 2023-01-26T11:14:28Z

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go

+	APIURL       string      `json:"apiURL,omitempty"`
+	RoomID       string      `json:"roomID,omitempty"`
+	Message      string      `json:"message,omitempty"`
+	HTTPConfig   *HTTPConfig `json:"httpConfig,omitempty"`


Suggested change

HTTPConfig *HTTPConfig `json:"httpConfig,omitempty"`

// HTTP client configuration.

HTTPConfig *HTTPConfig `json:"httpConfig,omitempty"`

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

eumel8 · 2023-01-26T17:09:11Z

thx for starting review, @xiu @simonpasquier

simonpasquier

Can you add comments for each field? Next step is to wire the webex integration in

prometheus-operator/pkg/alertmanager/amcfg.go

Line 464 in 6be89b4

    
           func (cb *configBuilder) convertReceiver(ctx context.Context, in *monitoringv1alpha1.Receiver, crKey types.NamespacedName) (*receiver, error) {

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go

pkg/apis/monitoring/v1beta1/alertmanager_config_types.go

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

eumel8 · 2023-02-03T22:53:38Z

just wondering, make generate fails on Ubuntu:

mv github.com/prometheus-operator/prometheus-operator/pkg/client/{versioned,informers,listers} pkg/client
mv: cannot stat 'github.com/prometheus-operator/prometheus-operator/pkg/client/informers': No such file or directory
mv: cannot stat 'github.com/prometheus-operator/prometheus-operator/pkg/client/listers': No such file or directory
make: *** [Makefile:142: k8s-client-gen] Error 1

xiu · 2023-02-04T10:11:36Z

just wondering, make generate fails on Ubuntu:

mv github.com/prometheus-operator/prometheus-operator/pkg/client/{versioned,informers,listers} pkg/client
mv: cannot stat 'github.com/prometheus-operator/prometheus-operator/pkg/client/informers': No such file or directory
mv: cannot stat 'github.com/prometheus-operator/prometheus-operator/pkg/client/listers': No such file or directory
make: *** [Makefile:142: k8s-client-gen] Error 1

This is not the issue as I see it. Check https://github.com/prometheus-operator/prometheus-operator/actions/runs/4088515306/jobs/7050273000, the step is checking whether all generated content has been committed. It thus runs make generate and does a global diff. In this case, it found changes.

Can you run make generate and commit the changes to your branch and try again?

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

eumel8 · 2023-02-04T18:40:45Z

@xiu This was the error on my local Ubuntu WSL. I couldn't found out the cause and made make generate on another machine. Maybe there are some hints for build requirements beside make and Go.

xiu · 2023-02-04T20:25:48Z

@xiu This was the error on my local Ubuntu WSL. I couldn't found out the cause and made make generate on another machine. Maybe there are some hints for build requirements beside make and Go.

Oh, I don't have a Windows machine to try that out unfortunately. Glad you made it work!

eumel8 · 2023-02-14T21:52:17Z

@simonpasquier I'm not sure, anything else to change here?

simonpasquier · 2023-02-16T15:28:06Z

@simonpasquier I'm not sure, anything else to change here?

You need to update the convertReceiver() method to convert the Webex configuration from the CRD to the native Alertmanager configuration.

You can take a look at #4726 which did the same thing but for the Telegram integration. Does it make sense?

fctb · 2023-03-17T16:18:23Z

any time when it will get unblocked?

eumel8 · 2023-03-19T09:13:00Z

@fctb It's progressing slowly. I'm a bit overwhelmed with the feature. Any help is appreciated, hopefully it will be over soon.

This reverts commit f6bda1d.

This reverts commit d77dfe2.

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

simonpasquier · 2023-07-06T15:52:58Z

@eumel8 I've added a few commits on top of your PR to address my comments.

simonpasquier · 2023-07-06T15:53:34Z

test/framework/resources/alertmanager-main-secret.yaml

is this change necessary for this PR? Otherwise I'd prefer to revert :)

I've reverted the change - if the E2E suite passes, it is indeed not necessary. I'd only changed it, as it didn't work when I ran the E2E tests locally and after debugging I'd found out the issue were the missing fields in the AM Config 🤷

Tests passed, so thanks for reviewing this change as well. Reverted as requested.

puffitos · 2023-07-06T17:13:42Z

test/e2e/scrapeconfig_test.go

@@ -216,7 +216,7 @@ func testPromOperatorStartsWithoutScrapeConfigCRD(t *testing.T) {

 	restarts, err := framework.GetPodRestartCount(context.Background(), ns, pl.Items[0].GetName())
 	require.NoError(t, err)
-	require.Lenf(t, restarts, 1, "expected to get 1 container but got %d", len(restarts))
+	require.Emptyf(t, restarts, "expected to get 1 container but got %d", len(restarts))


This test was failing on my machine with the message "expected to get 1 container but got 1". On line 213 one Pod is explicitly expected. The GetPodRestartCount will therefore always return a map with at least one container, which should have 0 restarts. The restarts map has therefore a length of at least 1.

Emptyf checks whether the map is empty, which it shouldn't. The error msg is even more cryptic, as it refers to the length of the map, which should be one according to the message. But here we're checking whether the map's nil / empty?

I can't get my head around as to how this tests works 😄 I just wanted to explain, why I made this change and why I didn't consider it unnecessary.

simonpasquier

very good! One last nit...

pkg/alertmanager/operator.go

ArthurSens

Oof, I approved and forgot to merge this one after 3 weeks... sorry! 😅

Approving again and I'll let simon take another look if he wants to :)

I'm putting a reminder to merge it this week if there is no more change requests

Webex Feature/API

73ba079

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

eumel8 requested a review from a team as a code owner January 26, 2023 09:05

simonpasquier reviewed Jan 26, 2023

View reviewed changes

eumel8 added 2 commits January 26, 2023 18:00

make generate

9383189

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

fix RoomID

65211e7

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

simonpasquier requested changes Feb 3, 2023

View reviewed changes

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved

pkg/apis/monitoring/v1beta1/alertmanager_config_types.go Outdated Show resolved Hide resolved

comment webex vars

abbe4aa

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

eumel8 and others added 2 commits February 4, 2023 18:46

make generate

99c3c7e

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>

Merge branch 'main' into feat/webex

a5bd003

Merge branch 'main' into feat/webex

e69eadc

eumel8 and others added 6 commits March 17, 2023 22:43

Merge branch 'main' into feat/webex

663da5e

fix alertmanager stuff for webex receiver

56a613d

fix format error

2c1a858

fix roomID validation

710d248

fix check webex conf

6241b8a

make generate

2983d3e

github-actions bot added the stale label May 19, 2023

zeritti mentioned this pull request May 30, 2023

kube-prometheus-stack Add webexConfigs definition prometheus-community/helm-charts#3440

Closed

3 tasks

puffitos added 12 commits June 21, 2023 19:14

chore: reverted types to upstream AM configuration

8d8b94d

feat: version check for webex

9d64267

feat: URL type for v1beta1 and v1alpha1

d77dfe2

feat: URL validation

f6bda1d

Revert "feat: URL validation"

5a1a704

This reverts commit f6bda1d.

Revert "feat: URL type for v1beta1 and v1alpha1"

a1e278e

This reverts commit d77dfe2.

feat: AM version validation for webex

5395c5c

fix: types in tests fixed

f33e9c2

Merge remote-tracking branch 'upstream/main' into feat/webex

1ce3225

chore: merged from upstream

450b5fd

feat: added URL type to all API versions for webex

028cf1f

chore: make generate format

8feea07

pull-request-size bot added size/XL and removed size/XXL labels Jun 23, 2023

puffitos and others added 4 commits June 23, 2023 22:06

fix: corrected API URL for tests

2621e3d

Synchronize v1beta1 types with v1alpha1

edfdcbd

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

Merge branch 'main' into feat/webex

7f232be

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

Revert unnecessary change

9dff799

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

pull-request-size bot added size/XXL and removed size/XL labels Jul 6, 2023

simonpasquier reviewed Jul 6, 2023

View reviewed changes

puffitos reviewed Jul 6, 2023

View reviewed changes

chore: reverted am secret changes for tests

f3a8f23

ArthurSens approved these changes Jul 13, 2023

View reviewed changes

simonpasquier reviewed Jul 28, 2023

View reviewed changes

pkg/alertmanager/operator.go Outdated Show resolved Hide resolved

chore: version incompatibility error msg fixed

189c9cb

ArthurSens approved these changes Jul 31, 2023

View reviewed changes

ArthurSens merged commit bfb0d07 into prometheus-operator:main Aug 2, 2023
16 checks passed

eumel8 deleted the feat/webex branch September 28, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webex Feature/API #5305

Webex Feature/API #5305

eumel8 commented Jan 26, 2023 •

edited by ArthurSens

eumel8 commented Jan 26, 2023

xiu commented Jan 26, 2023

simonpasquier commented Jan 26, 2023

simonpasquier Jan 26, 2023

simonpasquier Jan 26, 2023

simonpasquier Jan 26, 2023

eumel8 commented Jan 26, 2023

simonpasquier left a comment

eumel8 commented Feb 3, 2023

xiu commented Feb 4, 2023

eumel8 commented Feb 4, 2023

xiu commented Feb 4, 2023

eumel8 commented Feb 14, 2023

simonpasquier commented Feb 16, 2023

fctb commented Mar 17, 2023

eumel8 commented Mar 19, 2023

simonpasquier commented Jul 6, 2023

simonpasquier Jul 6, 2023

puffitos Jul 6, 2023

puffitos Jul 6, 2023 •

edited

puffitos Jul 6, 2023 •

edited

simonpasquier left a comment

ArthurSens left a comment

	SendResolved *bool `json:"sendResolved,omitempty"`
	// Whether to notify about resolved alerts.
	SendResolved *bool `json:"sendResolved,omitempty"`

	APIURL string `json:"apiURL,omitempty"`
	// The Webex Teams API URL if different from the default API URL.
	APIURL string `json:"apiURL,omitempty"`

	HTTPConfig *HTTPConfig `json:"httpConfig,omitempty"`
	// HTTP client configuration.
	HTTPConfig *HTTPConfig `json:"httpConfig,omitempty"`

Webex Feature/API #5305

Webex Feature/API #5305

Conversation

eumel8 commented Jan 26, 2023 • edited by ArthurSens

Description

Type of change

Changelog entry

eumel8 commented Jan 26, 2023

xiu commented Jan 26, 2023

simonpasquier commented Jan 26, 2023

simonpasquier Jan 26, 2023

Choose a reason for hiding this comment

simonpasquier Jan 26, 2023

Choose a reason for hiding this comment

simonpasquier Jan 26, 2023

Choose a reason for hiding this comment

eumel8 commented Jan 26, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

eumel8 commented Feb 3, 2023

xiu commented Feb 4, 2023

eumel8 commented Feb 4, 2023

xiu commented Feb 4, 2023

eumel8 commented Feb 14, 2023

simonpasquier commented Feb 16, 2023

fctb commented Mar 17, 2023

eumel8 commented Mar 19, 2023

simonpasquier commented Jul 6, 2023

simonpasquier Jul 6, 2023

Choose a reason for hiding this comment

puffitos Jul 6, 2023

Choose a reason for hiding this comment

puffitos Jul 6, 2023 • edited

Choose a reason for hiding this comment

puffitos Jul 6, 2023 • edited

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

eumel8 commented Jan 26, 2023 •

edited by ArthurSens

puffitos Jul 6, 2023 •

edited

puffitos Jul 6, 2023 •

edited