-
Notifications
You must be signed in to change notification settings - Fork 93
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.
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
add initial support for alertmanager external-url #622
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import ( | |
"context" | ||
"errors" | ||
"fmt" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -74,6 +75,7 @@ route: | |
}, | ||
Key: "my-secret-key", | ||
}, | ||
ExternalURL: "https://alertmanager.mycompany.com/", | ||
}, | ||
&corev1.Secret{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
|
@@ -108,15 +110,15 @@ func testAlertmanager(ctx context.Context, t *OperatorContext, spec *monitoringv | |
}) | ||
t.Run("deployed", t.subtest(func(ctx context.Context, t *OperatorContext) { | ||
t.Parallel() | ||
testAlertmanagerDeployed(ctx, t) | ||
testAlertmanagerDeployed(ctx, t, spec) | ||
})) | ||
t.Run("config set", t.subtest(func(ctx context.Context, t *OperatorContext) { | ||
t.Parallel() | ||
testAlertmanagerConfig(ctx, t, secret, key) | ||
})) | ||
} | ||
|
||
func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext) { | ||
func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext, spec *monitoringv1.ManagedAlertmanagerSpec) { | ||
err := wait.Poll(time.Second, 1*time.Minute, func() (bool, error) { | ||
var ss appsv1.StatefulSet | ||
if err := t.Client().Get(ctx, client.ObjectKey{Namespace: t.namespace, Name: operator.NameAlertmanager}, &ss); err != nil { | ||
|
@@ -135,6 +137,27 @@ func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext) { | |
return false, fmt.Errorf("unexpected annotations (-want, +got): %s", diff) | ||
} | ||
|
||
// If spec is empty, no need to assert EXTRA_ARGS. | ||
if spec == nil { | ||
return true, nil | ||
} | ||
var wantArgs []string | ||
for _, c := range ss.Spec.Template.Spec.Containers { | ||
if c.Name != "alertmanager" { | ||
continue | ||
} | ||
// We're mainly interested in the dynamic flags but checking the entire set including | ||
// the static ones is ultimately simpler. | ||
if externalURL := spec.ExternalURL; externalURL != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bit confused again - can't we prepare wantArgs before looping through Alertmanager statetful set containers? It feel like you do it inside this loop for a reason, but I can't find any 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
wantArgs = append(wantArgs, fmt.Sprintf("--web.external-url=%q", spec.ExternalURL)) | ||
} | ||
|
||
if diff := cmp.Diff(strings.Join(wantArgs, " "), getEnvVar(c.Env, "EXTRA_ARGS")); diff != "" { | ||
return false, fmt.Errorf("unexpected flags (-want, +got): %s", diff) | ||
} | ||
return true, nil | ||
} | ||
|
||
return true, nil | ||
}) | ||
if err != nil { | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -986,6 +986,9 @@ spec: | |||||
required: | ||||||
- key | ||||||
x-kubernetes-map-type: atomic | ||||||
externalURL: | ||||||
type: string | ||||||
description: ExternalURL is the external URL the managed Alertmanager will be available under. This is used for generating links back to the Alertmanager itself in fired alerts. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it wouldn't be easier to put exact flag description. It's essentially more than just alert link, it also fixes UI when reverse proxies are used. WDYT?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. Done. |
||||||
rules: | ||||||
type: object | ||||||
description: Rules specifies how the operator configures and deployes rule-evaluator. | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super small nit, but I was quite confused thinking initially this variable as used down there is Alertmanager spec not alertmanager configuration spec (kind of) - a bit more explicitness might help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure no prob. Changed it to
config
.