Skip to content

Commit f23cfb8

Browse files
AndrewChubatiukAndrewChubatiuk
committed
vmalert: do not modify notifiers in CR
1 parent 060281c commit f23cfb8

File tree

2 files changed

+48
-53
lines changed

2 files changed

+48
-53
lines changed

‎internal/controller/operator/factory/vmalert/vmalert.go‎

Lines changed: 38 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,6 @@ func CreateOrUpdate(ctx context.Context, cr *vmv1beta1.VMAlert, rclient client.C
8585
if cr.ParsedLastAppliedSpec != nil {
8686
prevCR = cr.DeepCopy()
8787
prevCR.Spec = *cr.ParsedLastAppliedSpec
88-
if err := discoverNotifierIfNeeded(ctx, rclient, prevCR); err != nil {
89-
logger.WithContext(ctx).Error(err, "cannot discover notifiers for prev spec")
90-
}
9188

9289
}
9390
if err := deletePrevStateResources(ctx, cr, rclient); err != nil {
@@ -102,9 +99,6 @@ func CreateOrUpdate(ctx context.Context, cr *vmv1beta1.VMAlert, rclient client.C
10299
return fmt.Errorf("failed create service account: %w", err)
103100
}
104101
}
105-
if err := discoverNotifierIfNeeded(ctx, rclient, cr); err != nil {
106-
return fmt.Errorf("cannot discover additional notifiers: %w", err)
107-
}
108102

109103
ac := getAssetsCache(ctx, rclient, cr)
110104

@@ -132,13 +126,21 @@ func CreateOrUpdate(ctx context.Context, cr *vmv1beta1.VMAlert, rclient client.C
132126

133127
var prevDeploy *appsv1.Deployment
134128
if prevCR != nil {
135-
prevDeploy, err = newDeploy(prevCR, cmNames, ac)
129+
prevNotifiers, err := discoverNotifiers(ctx, rclient, prevCR)
130+
if err != nil {
131+
logger.WithContext(ctx).Error(err, "cannot discover notifiers for prev spec")
132+
}
133+
prevDeploy, err = newDeploy(prevCR, cmNames, prevNotifiers, ac)
136134
if err != nil {
137135
return fmt.Errorf("cannot generate prev deploy spec: %w", err)
138136
}
139137
}
140138

141-
newDeploy, err := newDeploy(cr, cmNames, ac)
139+
newNotifiers, err := discoverNotifiers(ctx, rclient, cr)
140+
if err != nil {
141+
logger.WithContext(ctx).Error(err, "cannot discover notifiers for new spec")
142+
}
143+
newDeploy, err := newDeploy(cr, cmNames, newNotifiers, ac)
142144
if err != nil {
143145
return fmt.Errorf("cannot generate new deploy for vmalert: %w", err)
144146
}
@@ -152,9 +154,9 @@ func CreateOrUpdate(ctx context.Context, cr *vmv1beta1.VMAlert, rclient client.C
152154
}
153155

154156
// newDeploy returns a busybox pod with the same name/namespace as the cr
155-
func newDeploy(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, ac *build.AssetsCache) (*appsv1.Deployment, error) {
157+
func newDeploy(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, notifiers []vmv1beta1.VMAlertNotifierSpec, ac *build.AssetsCache) (*appsv1.Deployment, error) {
156158

157-
generatedSpec, err := newPodSpec(cr, ruleConfigMapNames, ac)
159+
generatedSpec, err := newPodSpec(cr, ruleConfigMapNames, notifiers, ac)
158160
if err != nil {
159161
return nil, fmt.Errorf("cannot generate new spec for vmalert: %w", err)
160162
}
@@ -174,8 +176,8 @@ func newDeploy(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, ac *build.Ass
174176
return deploy, nil
175177
}
176178

177-
func newPodSpec(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, ac *build.AssetsCache) (*appsv1.DeploymentSpec, error) {
178-
args, err := buildArgs(cr, ruleConfigMapNames, ac)
179+
func newPodSpec(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, notifiers []vmv1beta1.VMAlertNotifierSpec, ac *build.AssetsCache) (*appsv1.DeploymentSpec, error) {
180+
args, err := buildArgs(cr, ruleConfigMapNames, notifiers, ac)
179181
if err != nil {
180182
return nil, err
181183
}
@@ -435,13 +437,13 @@ func buildAuthArgs(args []string, namespace, flagPrefix string, cfg vmv1beta1.HT
435437
return args, nil
436438
}
437439

438-
func buildArgs(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, ac *build.AssetsCache) ([]string, error) {
440+
func buildArgs(cr *vmv1beta1.VMAlert, ruleConfigMapNames []string, notifiers []vmv1beta1.VMAlertNotifierSpec, ac *build.AssetsCache) ([]string, error) {
439441
args := []string{
440442
fmt.Sprintf("-datasource.url=%s", cr.Spec.Datasource.URL),
441443
}
442444

443445
args = buildHeadersArg("datasource.headers", args, cr.Spec.Datasource.Headers)
444-
notifierArgs, err := buildNotifiersArgs(cr, ac)
446+
notifierArgs, err := buildNotifiersArgs(cr, notifiers, ac)
445447
if err != nil {
446448
return nil, err
447449
}
@@ -532,17 +534,15 @@ func createOrUpdateAssets(ctx context.Context, rclient client.Client, cr, prevCR
532534
return nil
533535
}
534536

535-
func buildNotifiersArgs(cr *vmv1beta1.VMAlert, ac *build.AssetsCache) ([]string, error) {
537+
func buildNotifiersArgs(cr *vmv1beta1.VMAlert, notifiers []vmv1beta1.VMAlertNotifierSpec, ac *build.AssetsCache) ([]string, error) {
536538
var args []string
537-
notifierTargets := cr.Spec.Notifiers
538-
539539
if _, ok := cr.Spec.ExtraArgs["notifier.blackhole"]; ok {
540540
// notifier.blackhole disables sending notifications completely, so we don't need to add any notifier args
541541
// also no need to add notifier.blackhole to args as it will be added with ExtraArgs
542542
return args, nil
543543
}
544544

545-
if len(notifierTargets) == 0 && cr.Spec.NotifierConfigRef != nil {
545+
if len(notifiers) == 0 && cr.Spec.NotifierConfigRef != nil {
546546
return append(args, fmt.Sprintf("-notifier.config=%s/%s", notifierConfigMountPath, cr.Spec.NotifierConfigRef.Key)), nil
547547
}
548548

@@ -561,7 +561,7 @@ func buildNotifiersArgs(cr *vmv1beta1.VMAlert, ac *build.AssetsCache) ([]string,
561561
oauth2Scopes := build.NewFlag("-notifier.oauth2.scopes", "")
562562
oauth2TokenURL := build.NewFlag("-notifier.oauth2.tokenUrl", "")
563563

564-
for i, nt := range notifierTargets {
564+
for i, nt := range notifiers {
565565
url.Add(nt.URL, i)
566566
ntTLS := nt.TLSConfig
567567
if ntTLS != nil {
@@ -646,7 +646,7 @@ func buildNotifiersArgs(cr *vmv1beta1.VMAlert, ac *build.AssetsCache) ([]string,
646646
if !url.IsSet() {
647647
args = append(args, "-notifier.url=")
648648
}
649-
totalCount := len(notifierTargets)
649+
totalCount := len(notifiers)
650650
args = build.AppendFlagsToArgs(args, totalCount, url, authUser, authPasswordFile)
651651
args = build.AppendFlagsToArgs(args, totalCount, tlsServerName, tlsKeys, tlsCerts, tlsCAs, tlsInsecure, headers, bearerTokenPath)
652652
args = build.AppendFlagsToArgs(args, totalCount, oauth2SecretFile, oauth2ClientID, oauth2Scopes, oauth2TokenURL)
@@ -710,51 +710,38 @@ func buildConfigReloaderContainer(cr *vmv1beta1.VMAlert, ruleConfigMapNames []st
710710
return configReloaderContainer
711711
}
712712

713-
func discoverNotifierIfNeeded(ctx context.Context, rclient client.Client, cr *vmv1beta1.VMAlert) error {
714-
var additionalNotifiers []vmv1beta1.VMAlertNotifierSpec
715-
713+
func discoverNotifiers(ctx context.Context, rclient client.Client, cr *vmv1beta1.VMAlert) ([]vmv1beta1.VMAlertNotifierSpec, error) {
714+
var notifiers []vmv1beta1.VMAlertNotifierSpec
716715
if cr.Spec.Notifier != nil {
717-
cr.Spec.Notifiers = append(cr.Spec.Notifiers, *cr.Spec.Notifier)
716+
notifiers = append(notifiers, *cr.Spec.Notifier)
718717
}
719-
// trim notifiers with non-empty notifier Selector
720-
var cnt int
721718
for i := range cr.Spec.Notifiers {
722719
n := cr.Spec.Notifiers[i]
723-
// fast path
724720
if n.Selector == nil {
725-
cr.Spec.Notifiers[cnt] = n
726-
cnt++
721+
notifiers = append(notifiers, n)
727722
continue
728723
}
729724
// discover alertmanagers
730-
var ams vmv1beta1.VMAlertmanagerList
731-
amListOpts, err := n.Selector.AsListOptions()
725+
var l vmv1beta1.VMAlertmanagerList
726+
o, err := n.Selector.AsListOptions()
732727
if err != nil {
733-
return fmt.Errorf("cannot convert notifier selector as ListOptions: %w", err)
728+
return nil, fmt.Errorf("cannot convert notifier selector as ListOptions: %w", err)
734729
}
735730
if err := k8stools.ListObjectsByNamespace(ctx, rclient, config.MustGetWatchNamespaces(), func(objects *vmv1beta1.VMAlertmanagerList) {
736-
ams.Items = append(ams.Items, objects.Items...)
737-
}, amListOpts); err != nil {
738-
return fmt.Errorf("cannot list alertmanager with discovery selector: %w", err)
739-
}
740-
741-
for _, item := range ams.Items {
742-
if !item.DeletionTimestamp.IsZero() || (n.Selector.Namespace != nil && !n.Selector.Namespace.IsMatch(&item)) {
743-
continue
731+
for _, item := range l.Items {
732+
if !item.DeletionTimestamp.IsZero() || (n.Selector.Namespace != nil && !n.Selector.Namespace.IsMatch(&item)) {
733+
continue
734+
}
735+
notifiers = append(notifiers, item.AsNotifiers()...)
744736
}
745-
dsc := item.AsNotifiers()
746-
additionalNotifiers = append(additionalNotifiers, dsc...)
737+
}, o); err != nil {
738+
return nil, fmt.Errorf("cannot list alertmanager with discovery selector: %w", err)
747739
}
748740
}
749-
cr.Spec.Notifiers = cr.Spec.Notifiers[:cnt]
750-
751-
if len(additionalNotifiers) > 0 {
752-
sort.Slice(additionalNotifiers, func(i, j int) bool {
753-
return additionalNotifiers[i].URL > additionalNotifiers[j].URL
754-
})
755-
}
756-
cr.Spec.Notifiers = append(cr.Spec.Notifiers, additionalNotifiers...)
757-
return nil
741+
sort.Slice(notifiers, func(i, j int) bool {
742+
return notifiers[i].URL < notifiers[j].URL
743+
})
744+
return notifiers, nil
758745
}
759746

760747
func deletePrevStateResources(ctx context.Context, cr *vmv1beta1.VMAlert, rclient client.Client) error {

‎internal/controller/operator/factory/vmalert/vmalert_test.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,11 @@ func TestBuildNotifiers(t *testing.T) {
623623
ctx := context.Background()
624624
fclient := k8stools.GetTestClientWithObjects(tt.predefinedObjects)
625625
ac := getAssetsCache(ctx, fclient, tt.cr)
626-
if got, err := buildNotifiersArgs(tt.cr, ac); err != nil {
626+
notifiers, err := discoverNotifiers(ctx, fclient, tt.cr)
627+
if err != nil {
628+
t.Errorf("discoverNotifiers(), unexpected error: %s", err)
629+
}
630+
if got, err := buildNotifiersArgs(tt.cr, notifiers, ac); err != nil {
627631
t.Errorf("buildNotifiersArgs(), unexpected error: %s", err)
628632
} else if !reflect.DeepEqual(got, tt.want) {
629633
t.Errorf("BuildNotifiersArgs() = \ngot \n%v, \nwant \n%v", got, tt.want)
@@ -727,7 +731,11 @@ func Test_buildVMAlertArgs(t *testing.T) {
727731
ctx := context.Background()
728732
fclient := k8stools.GetTestClientWithObjects(nil)
729733
ac := getAssetsCache(ctx, fclient, tt.cr)
730-
if got, err := buildArgs(tt.cr, tt.ruleConfigMapNames, ac); err != nil {
734+
notifiers, err := discoverNotifiers(ctx, fclient, tt.cr)
735+
if err != nil {
736+
t.Errorf("discoverNotifiers(), unexpected error: %s", err)
737+
}
738+
if got, err := buildArgs(tt.cr, tt.ruleConfigMapNames, notifiers, ac); err != nil {
731739
t.Errorf("buildArgs(), unexpected error: %s", err)
732740
} else if !reflect.DeepEqual(got, tt.want) {
733741
assert.Equal(t, tt.want, got)

0 commit comments

Comments
 (0)