helm/metrics: add a new gauge metric to monitor helm releases #579
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,90 @@ | ||||||
| package metrics | ||||||
|
Member
There was a problem hiding this comment. lets put this into the
Contributor
Author
There was a problem hiding this comment. Moved to |
||||||
|
|
||||||
| import ( | ||||||
| "log" | ||||||
| "os" | ||||||
|
|
||||||
| "helm.sh/helm/v3/pkg/action" | ||||||
| "k8s.io/cli-runtime/pkg/genericclioptions" | ||||||
| "k8s.io/client-go/rest" | ||||||
| k8smetrics "k8s.io/component-base/metrics" | ||||||
| "k8s.io/component-base/metrics/legacyregistry" | ||||||
| "k8s.io/klog/v2" | ||||||
| ) | ||||||
|
|
||||||
| var ( | ||||||
| helmChartReleaseHealthStatus = k8smetrics.NewGaugeVec( | ||||||
| &k8smetrics.GaugeOpts{ | ||||||
| Name: "helm_chart_release_health_status", | ||||||
| Help: "Health of the Helm release", | ||||||
| }, | ||||||
| []string{"releaseName", "chartName", "chartVersion"}, | ||||||
| ) | ||||||
| ) | ||||||
|
|
||||||
| func init() { | ||||||
| legacyregistry.MustRegister(helmChartReleaseHealthStatus) | ||||||
| } | ||||||
|
|
||||||
| func HandleHelmChartReleaseHealthStatus() { | ||||||
| defer recoverMetricPanic() | ||||||
|
|
||||||
| actionConfig, err := getActionConfig() | ||||||
| if err != nil { | ||||||
| klog.Errorf("metric helm_chart_release_health_status unhandled: %v", err) | ||||||
| return | ||||||
| } | ||||||
| listAction := action.NewList(actionConfig) | ||||||
| releases, err := listAction.Run() | ||||||
| if err != nil { | ||||||
| klog.Errorf("metric helm_chart_release_health_status unhandled: %v", err) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| if len(releases) == 0 { | ||||||
| // Initialize metrics with value 0 | ||||||
| // Reference: https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics | ||||||
| helmChartReleaseHealthStatus.WithLabelValues("", "", "").Set(0) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| for _, release := range releases { | ||||||
| releaseStatus := release.Info.Status.String() | ||||||
| healthStatus := 1 | ||||||
| if releaseStatus == "failed" || releaseStatus == "unknown" { | ||||||
| healthStatus = 0 | ||||||
| } | ||||||
| klog.V(4).Infof("metric helm_chart_release_health_status %d: %s %s %s", healthStatus, release.Name, release.Chart.Metadata.Name, release.Chart.Metadata.Version) | ||||||
| helmChartReleaseHealthStatus.WithLabelValues(release.Name, release.Chart.Metadata.Name, release.Chart.Metadata.Version).Set(float64(healthStatus)) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Reference: https://github.com/helm/helm/issues/7430#issuecomment-620489002 | ||||||
| func getActionConfig() (*action.Configuration, error) { | ||||||
| actionConfig := new(action.Configuration) | ||||||
| var kubeConfig *genericclioptions.ConfigFlags | ||||||
|
Member
There was a problem hiding this comment. Dont think kubeconfig is the right name
Suggested change
Member
There was a problem hiding this comment. also why do we need to initialize the configFlags, since on line in 72 you are setting it ? cant we jusst set it directly ? |
||||||
| // Create the rest config instance with ServiceAccount values loaded in them | ||||||
| config, err := rest.InClusterConfig() | ||||||
| if err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| // Create the ConfigFlags struct instance with initialized values from ServiceAccount | ||||||
| kubeConfig = genericclioptions.NewConfigFlags(false) | ||||||
| kubeConfig.APIServer = &config.Host | ||||||
| kubeConfig.BearerToken = &config.BearerToken | ||||||
| kubeConfig.CAFile = &config.CAFile | ||||||
| // Empty string for all namespaces | ||||||
| if err := actionConfig.Init(kubeConfig, "", os.Getenv("HELM_DRIVER"), log.Printf); err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| return actionConfig, nil | ||||||
| } | ||||||
|
|
||||||
| // We will never want to panic our operator because of metric saving. | ||||||
| // Therefore, we will recover our panics here and error log them | ||||||
| // for later diagnosis but will never fail the operator. | ||||||
| func recoverMetricPanic() { | ||||||
| if r := recover(); r != nil { | ||||||
| klog.Errorf("Recovering from metric function - %v", r) | ||||||
| } | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why does the operator need to be able to fetch any secret from the cluster? Looks like a potential security issue. If we need we should rather create a Role for a specific namespace. But on the other hand I dont see any usage in the added code, where we are fetching any secret for the metrics.
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.
The
helm list --all-namespacesaction frommetrics.gorequires this particular permission. If I remove this it would produce: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.
This is pretty concerning because console-operator is effectively running as root on the cluster with this change. Is there any way to add the metrics without needing this permission?
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.
Yeah this is a hard no. We won't escalate the console operator to have read on Secrets cluster-wide.
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.
Understand, we are looking for alternative options to mitigate. It won't be a full solution if we have to use the users identity on the console side, but that is what we will pivot towards. For future reference @spadgett and @jwforres, If we had a way to filter Allow Rules by Resource Type like for example "type": "helm.sh/release.v1", would that work to reduce the access escalation? Trying to figure out if I propose a change in Kubernetes RBAC upstream on this.