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

Fix errors in clustermetrics validation #979

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eliihen
Copy link
Contributor

@eliihen eliihen commented Dec 9, 2024

This PR fixes the following error:

When using urlFrom in the prometheus destination, it only uses the url field and throws due to url being Nil:

$ helm dep up; helm template observability .
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "grafana" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Deleting outdated charts
Error: template: observability/charts/k8s-monitoring/charts/k8s-monitoring/templates/validations.yaml:9:6: executing "observability/charts/k8s-monitoring/charts/k8s-monitoring/templates/validations.yaml" at <include (printf "features.%s.validate" $feature) $>: error calling include: template: observability/charts/k8s-monitoring/charts/k8s-monitoring/templates/features/_feature_cluster_metrics.tpl:71:43: executing "features.clusterMetrics.validate" at <$destinationUrl>: invalid value; expected string

Use --debug flag to render out invalid YAML

@eliihen eliihen requested a review from petewall as a code owner December 9, 2024 12:57
@CLAassistant
Copy link

CLAassistant commented Dec 9, 2024

CLA assistant check
All committers have signed the CLA.

@eliihen eliihen force-pushed the fix-clustermetrics-validation branch from f74b1fe to c67b315 Compare December 10, 2024 07:53
@petewall
Copy link
Collaborator

Thanks for catching this. There's a real bug here. I think we'll need to think a bit more about this.

In the URL case:

- type: prometheus
  url: http://prom.example.com/api/prom

the OpenCost validation tries to build a url for OpenCost to send promql queries to.

In the URL From case:

- type: prometheus
  urlFrom: sys.env("PROM_URL")

It can't turn that into a recommendation. Because of the OpenCost Helm chart, I can't use the same urlFrom semantic. What needs to happen is an "escape hatch" part to the validation that just says:

You wanna configure OpenCost? This is what you'll need:

clusterMetrics:
  opencost:
    opencost:
      prometheus:
        username: <basic auth username>
        password: <basic auth password>
        external:
          url: <Prometheus query URL>

@eliihen
Copy link
Contributor Author

eliihen commented Dec 17, 2024

Absolutely! However, keep in mind when building the template the chart currently crashes, and this fix will ensure that doesn't happen. So while not perfect, it does show the fallback <$destinationName Query URL>, which will be an improvement over a nondescript error.

@petewall
Copy link
Collaborator

petewall commented Dec 18, 2024

This change: #1012
should help with the opencost validation. If the destination url doesn't exist, or it can't figure it out, it'll just give a more generic message.

@petewall
Copy link
Collaborator

Can you retest with the latest rc? It should handle things better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants