Skip to content
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 validation for pod/service monitors for TargetAllocator and skip invalid ones #2328

Merged
Merged
Changes from 2 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1b7e32b
fix for adding validation
rashmichandrashekar Nov 9, 2023
a55e688
removing unused references
rashmichandrashekar Nov 9, 2023
401ad2c
adding tests
rashmichandrashekar Nov 29, 2023
b582198
adding some changes
rashmichandrashekar Nov 29, 2023
1a90fae
syncing with latest main
rashmichandrashekar Nov 29, 2023
2c276dc
cleaning up
rashmichandrashekar Nov 29, 2023
e97f969
adding change log
rashmichandrashekar Nov 29, 2023
26843ed
Merge branch 'main' into rashmi/add-validation
rashmichandrashekar Dec 4, 2023
533dff8
running goimports and adding return value check code for namespace in…
rashmichandrashekar Dec 4, 2023
537a2bb
fixing lint error
rashmichandrashekar Dec 4, 2023
90bcb75
fixing tests and comment
rashmichandrashekar Dec 4, 2023
dfcc746
adding permissions for e2e tests
rashmichandrashekar Dec 4, 2023
5406145
adding cluster roles instead of roles
rashmichandrashekar Dec 4, 2023
883b583
updaintg readme
rashmichandrashekar Dec 4, 2023
7885db1
fixing comments
rashmichandrashekar Dec 5, 2023
83abf55
Merge branch 'main' into rashmi/add-validation
rashmichandrashekar Dec 5, 2023
c9f3aa5
adding contant to same block
rashmichandrashekar Jan 2, 2024
436b888
Merge branch 'rashmi/add-validation' of https://github.com/rashmichan…
rashmichandrashekar Jan 2, 2024
319e72f
merging with upstream main
rashmichandrashekar Jan 12, 2024
4fc1403
fixing lint errors
rashmichandrashekar Jan 12, 2024
a943cdb
running go import
rashmichandrashekar Jan 13, 2024
bf33615
adding namespaces since that is required for informer
rashmichandrashekar Jan 13, 2024
bc6dc56
adding extected warnings
rashmichandrashekar Jan 13, 2024
b627b24
Merge branch 'main' into rashmi/add-validation
rashmichandrashekar Jan 17, 2024
b742f0a
addressing comments
rashmichandrashekar Jan 18, 2024
d062dbd
adding test for namespace label update
rashmichandrashekar Jan 18, 2024
859ed13
Merge branch 'main' into rashmi/add-validation
rashmichandrashekar Jan 18, 2024
da40b95
fixing goimports
rashmichandrashekar Jan 18, 2024
f0bc9d2
making namespaceselectores as labelselectors
rashmichandrashekar Jan 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 127 additions & 6 deletions cmd/otel-allocator/watcher/promOperator.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package watcher
import (
"context"
"fmt"
"regexp"
"time"

"github.com/go-kit/log"
Expand All @@ -27,8 +28,10 @@ import (
monitoringclient "github.com/prometheus-operator/prometheus-operator/pkg/client/versioned"
"github.com/prometheus-operator/prometheus-operator/pkg/informers"
"github.com/prometheus-operator/prometheus-operator/pkg/prometheus"
"github.com/prometheus/common/model"
promconfig "github.com/prometheus/prometheus/config"
kubeDiscovery "github.com/prometheus/prometheus/discovery/kubernetes"
"github.com/prometheus/prometheus/model/relabel"
"gopkg.in/yaml.v2"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -220,8 +223,12 @@ func (w *PrometheusCRWatcher) LoadConfig(ctx context.Context) (*promconfig.Confi
smRetrieveErr := w.informers[monitoringv1.ServiceMonitorName].ListAll(w.serviceMonitorSelector, func(sm interface{}) {
monitor := sm.(*monitoringv1.ServiceMonitor)
key, _ := cache.DeletionHandlingMetaNamespaceKeyFunc(monitor)
w.addStoreAssetsForServiceMonitor(ctx, monitor.Name, monitor.Namespace, monitor.Spec.Endpoints, store)
serviceMonitorInstances[key] = monitor
validateError := w.addStoreAssetsForServiceMonitor(ctx, monitor.Name, monitor.Namespace, monitor.Spec.Endpoints, store)
if validateError != nil {
w.logger.Error(validateError, "Failed validating ServiceMonitor, skipping", "ServiceMonitor:", monitor.Name, "in namespace", monitor.Namespace)
} else {
serviceMonitorInstances[key] = monitor
}
})
if smRetrieveErr != nil {
return nil, smRetrieveErr
Expand All @@ -231,8 +238,12 @@ func (w *PrometheusCRWatcher) LoadConfig(ctx context.Context) (*promconfig.Confi
pmRetrieveErr := w.informers[monitoringv1.PodMonitorName].ListAll(w.podMonitorSelector, func(pm interface{}) {
monitor := pm.(*monitoringv1.PodMonitor)
key, _ := cache.DeletionHandlingMetaNamespaceKeyFunc(monitor)
w.addStoreAssetsForPodMonitor(ctx, monitor.Name, monitor.Namespace, monitor.Spec.PodMetricsEndpoints, store)
podMonitorInstances[key] = monitor
validateError := w.addStoreAssetsForPodMonitor(ctx, monitor.Name, monitor.Namespace, monitor.Spec.PodMetricsEndpoints, store)
if validateError != nil {
w.logger.Error(validateError, "Failed validating PodMonitor, skipping", "PodMonitor:", monitor.Name, "in namespace", monitor.Namespace)
} else {
podMonitorInstances[key] = monitor
}
})
if pmRetrieveErr != nil {
return nil, pmRetrieveErr
Expand Down Expand Up @@ -288,8 +299,9 @@ func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor(
smName, smNamespace string,
endps []monitoringv1.Endpoint,
store *assets.Store,
) {
) error {
var err error
var validateErr error
for i, endp := range endps {
objKey := fmt.Sprintf("serviceMonitor/%s/%s/%d", smNamespace, smName, i)

Expand All @@ -315,11 +327,33 @@ func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor(
if err = store.AddSafeAuthorizationCredentials(ctx, smNamespace, endp.Authorization, smAuthKey); err != nil {
break
}

for _, rl := range endp.RelabelConfigs {
if rl.Action != "" {
if validateErr = validateRelabelConfig(*rl); validateErr != nil {
break
}
}
}

for _, rl := range endp.MetricRelabelConfigs {
if rl.Action != "" {
if validateErr = validateRelabelConfig(*rl); validateErr != nil {
break
}
}
}
}

if err != nil {
w.logger.Error(err, "Failed to obtain credentials for a ServiceMonitor", "serviceMonitor", smName)
}

if validateErr != nil {
return validateErr
}

return nil
}

// addStoreAssetsForServiceMonitor adds authentication / authorization related information to the assets store,
Expand All @@ -331,7 +365,7 @@ func (w *PrometheusCRWatcher) addStoreAssetsForPodMonitor(
pmName, pmNamespace string,
podMetricsEndps []monitoringv1.PodMetricsEndpoint,
store *assets.Store,
) {
) error {
var err error
for i, endp := range podMetricsEndps {
objKey := fmt.Sprintf("podMonitor/%s/%s/%d", pmNamespace, pmName, i)
Expand All @@ -358,9 +392,96 @@ func (w *PrometheusCRWatcher) addStoreAssetsForPodMonitor(
if err = store.AddSafeAuthorizationCredentials(ctx, pmNamespace, endp.Authorization, smAuthKey); err != nil {
break
}

for _, rl := range endp.RelabelConfigs {
if rl.Action != "" {
if validateErr = validateRelabelConfig(*rl); validateErr != nil {
break
}
}
}

for _, rl := range endp.MetricRelabelConfigs {
if rl.Action != "" {
if validateErr = validateRelabelConfig(*rl); validateErr != nil {
break
}
}
}
}

if err != nil {
w.logger.Error(err, "Failed to obtain credentials for a PodMonitor", "podMonitor", pmName)
}

if validateErr != nil {
return validateErr
}

return nil
}

// validateRelabelConfig validates relabel config for service and pod monitor,
// based on the service monitor and pod metrics endpoints specs.
// This code borrows from
// https://github.com/prometheus-operator/prometheus-operator/blob/ba536405154d18f3a6f312818283d671182af6f3/pkg/prometheus/resource_selector.go#L237
func validateRelabelConfig(rc monitoringv1.RelabelConfig) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful for us to reach out to the prometheus operator folks to see if they would expose this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jaronoff97. I have reached out to them, waiting for a response,(prometheus-operator/prometheus-operator#6057 (comment)) based on that I can add the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also created an issue here - prometheus-operator/prometheus-operator#6076

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaronoff97 - There is a question about breaking changes. I have answered it from what I understand and what I think the way forward would be. Could you please respond to provide your perspective and correct me if i am wrong?
prometheus-operator/prometheus-operator#6076 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, i responded there :) thanks for making the issue. Let's wait to hear from them before merging this – i would love to just use their method rather than having to write it ourselves.

relabelTarget := regexp.MustCompile(`^(?:(?:[a-zA-Z_]|\$(?:\{\w+\}|\w+))+\w*)+$`)

if _, err := relabel.NewRegexp(rc.Regex); err != nil {
return fmt.Errorf("invalid regex %s for relabel configuration", rc.Regex)
}

if rc.Modulus == 0 && rc.Action == string(relabel.HashMod) {
return fmt.Errorf("relabel configuration for hashmod requires non-zero modulus")
}

if (rc.Action == string(relabel.Replace) || rc.Action == string(relabel.HashMod) || rc.Action == string(relabel.Lowercase) || rc.Action == string(relabel.Uppercase) || rc.Action == string(relabel.KeepEqual) || rc.Action == string(relabel.DropEqual)) && rc.TargetLabel == "" {
return fmt.Errorf("relabel configuration for %s action needs targetLabel value", rc.Action)
}

if (rc.Action == string(relabel.Replace) || rc.Action == string(relabel.Lowercase) || rc.Action == string(relabel.Uppercase) || rc.Action == string(relabel.KeepEqual) || rc.Action == string(relabel.DropEqual)) && !relabelTarget.MatchString(rc.TargetLabel) {
return fmt.Errorf("%q is invalid 'target_label' for %s action", rc.TargetLabel, rc.Action)
}

if (rc.Action == string(relabel.Lowercase) || rc.Action == string(relabel.Uppercase) || rc.Action == string(relabel.KeepEqual) || rc.Action == string(relabel.DropEqual)) && !(rc.Replacement == relabel.DefaultRelabelConfig.Replacement || rc.Replacement == "") {
return fmt.Errorf("'replacement' can not be set for %s action", rc.Action)
}

if rc.Action == string(relabel.LabelMap) {
if rc.Replacement != "" && !relabelTarget.MatchString(rc.Replacement) {
return fmt.Errorf("%q is invalid 'replacement' for %s action", rc.Replacement, rc.Action)
}
}

if rc.Action == string(relabel.HashMod) && !model.LabelName(rc.TargetLabel).IsValid() {
return fmt.Errorf("%q is invalid 'target_label' for %s action", rc.TargetLabel, rc.Action)
}

if rc.Action == string(relabel.KeepEqual) || rc.Action == string(relabel.DropEqual) {
if !(rc.Regex == "" || rc.Regex == relabel.DefaultRelabelConfig.Regex.String()) ||
!(rc.Modulus == uint64(0) ||
rc.Modulus == relabel.DefaultRelabelConfig.Modulus) ||
!(rc.Separator == "" ||
rc.Separator == relabel.DefaultRelabelConfig.Separator) ||
!(rc.Replacement == relabel.DefaultRelabelConfig.Replacement ||
rc.Replacement == "") {
return fmt.Errorf("%s action requires only 'source_labels' and `target_label`, and no other fields", rc.Action)
}
}

if rc.Action == string(relabel.LabelDrop) || rc.Action == string(relabel.LabelKeep) {
if len(rc.SourceLabels) != 0 ||
!(rc.TargetLabel == "" ||
rc.TargetLabel == relabel.DefaultRelabelConfig.TargetLabel) ||
!(rc.Modulus == uint64(0) ||
rc.Modulus == relabel.DefaultRelabelConfig.Modulus) ||
!(rc.Separator == "" ||
rc.Separator == relabel.DefaultRelabelConfig.Separator) ||
!(rc.Replacement == relabel.DefaultRelabelConfig.Replacement ||
rc.Replacement == "") {
return fmt.Errorf("%s action requires only 'regex', and no other fields", rc.Action)
}
}
return nil
}
Loading