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.

Sign up for GitHub

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

Jump to bottom

Provisioning/Alerting: Environment variable substitution not working for annotations #84250

Open
eloo-abi opened this issue Mar 12, 2024 · 6 comments · May be fixed by #88279
Open

Provisioning/Alerting: Environment variable substitution not working for annotations #84250

eloo-abi opened this issue Mar 12, 2024 · 6 comments · May be fixed by #88279
Labels
area/alerting Grafana Alerting area/backend good first issue Good for newcomers prio/low It's a good idea, but not scheduled for any release type/feature-request

Comments

@eloo-abi
Copy link

What happened?

Hi,
we are working on enhancing our alerting right now and for this we want to use the environment variable substitution a bit more but have discovered that it is not working as expected.

We use currently env substitution for the labels in our alert files which is working.
But it looks like it is, for unknown reasons, not working when we try it in annotations which is unexpected.

We deploy our Grafana instance with Helm and use the provisioning path for setting up our alerts like this:

    extraConfigmapMounts:
      - name: grafana-alerts-provisioning
        subPath: ""
        mountPath: /etc/grafana/provisioning/alerting
        configMap: grafana-alerts
        readOnly: true

Below you can see that we have tested a few combinations for the annotations but in the screenshot you see its not rendered.
But for the labels its working as expected.

What did you expect to happen?

Environments variables are substituted in annotations in the same way as in labels.

Did this work before?

no

How do we reproduce it?

Alert file

apiVersion: 1
groups:
  - orgId: 1
    name: Kyverno
    folder: folder
    interval: 1m
    rules:
      - uid: 6pbn5054k
        title: Temporary role found
        condition: C
        data: ...
        noDataState: OK
        execErrState: Error
        for: 5m
        annotations:
          category: '{{ $labels.category }}'
          description: A temporary role was found
          policy: '{{ $labels.policy }}'
          severity: '{{ $labels.severity }}'
          summary: Temporary role found
          link: https://link.$AWS_REGION.$CLUSTER.tld/#/cluster-policy-reports/kyverno?policies={{$labels.policy}}
          link2: https://link.${AWS_REGION}.${CLUSTER}.tld/#/cluster-policy-reports/kyverno?policies={{$labels.policy}}
          link3: "https://link.${AWS_REGION}.${CLUSTER}.tld/#/cluster-policy-reports/kyverno?policies={{$labels.policy}}"
          link4: "https://link.$${AWS_REGION}.$${CLUSTER}.tld/#/cluster-policy-reports/kyverno?policies={{$labels.policy}}"
        labels:
          link: https://link.$AWS_REGION.$CLUSTER.tld/#/cluster-policy-reports/kyverno?policies={{$$labels.policy}}
          cluster: $CLUSTER
          level: error
          region: $REGION
          audiences: platform
        isPaused: false

Results in rendered label but not rendered in annotations

image

Is the bug inside a dashboard panel?

No response

Environment (with versions)?

Grafana: v10.3.3 (2527612)
OS:
Browser:

Grafana platform?

Kubernetes

Datasource(s)?

No response
@tonypowa tonypowa added the area/alerting Grafana Alerting label Apr 11, 2024
@JohnnyQQQQ JohnnyQQQQ added type/feature-request prio/low It's a good idea, but not scheduled for any release good first issue Good for newcomers area/backend labels May 22, 2024
@wasim-nihal
Copy link
Contributor

Hi @JohnnyQQQQ, is this open for contribution? I'd like to open a PR for the same.

@JohnnyQQQQ
Copy link
Member

JohnnyQQQQ commented May 23, 2024 •
edited

Yes it is. You can see how to implement it by taking a look at how we do it for labels here

grafana/pkg/services/provisioning/alerting/rules_types.go

Lines 122 to 123 in bd2b248

alertRule.Annotations = rule.Annotations.Raw
alertRule.Labels = rule.Labels.Value()

So instead of doing alertRule.Annotations = rule.Annotations.Raw for the annotations, we should call .Value().

Make sure to add some tests.

wasim-nihal added a commit to nokia/grafana that referenced this issue May 24, 2024
@wasim-nihal
adding support for env/file provider expansion for annotations in ale…
07a0bd9 
…rt rule. see: grafana#84250

Signed-off-by: Syed Nihal <syed.nihal@nokia.com>
@wasim-nihal wasim-nihal linked a pull request May 24, 2024 that will close this issue
Open
3 tasks
@JohnnyQQQQ
Copy link
Member

After a bit of further research, I now remember why we removed it. Please check out #54850

We might want to find a way to support both.

@wasim-nihal
Copy link
Contributor

Sure, I will explore a bit of template expansion and make changes to support both kinds of expansions.

@wasim-nihal
Copy link
Contributor

@JohnnyQQQQ , after looking into the codebase of the environment variable expander, I see it is possible to prevent the expansion and preserve the original formatting by prefixing the expression with an additional $.

For example, {{ $labels.instance }} can be modified to {{ $$labels.instance }} to prevent environment variable expansion and let the template expander take care to expand down the line. And, for users wanting to use env variable expansion can use ${ENV_VARIABLE}

I have validated these scenarios with from the binary build from the above commit (that uses .Value() for annotations)

Please do let me know your thoughts on this.

Snippet of alert rule used for local testing:

          noDataState: NoData
          execErrState: Error
          for: 1m
          annotations:
            description: this is example annotation description
            summary: 'Alert fired for {{ $$labels.instance }}'
            annotationEnv: ${ENV_ANNOTATION}
          labels:
            something: newthing
            instanceIs: '{{ $$labels.instance }}'
          isPaused: false
          notification_settings:
            receiver: myslack

Test Notification Received on Slack:

image

@JohnnyQQQQ
Copy link
Member

I think the main problem with this is that it would be a breaking change, as { $labels.instance } would behave differently after this change is applied and it's already widely used like this.

As I'm leaving the alerting team this week I will ping the right person to make a decision here and follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/alerting Grafana Alerting area/backend good first issue Good for newcomers prio/low It's a good idea, but not scheduled for any release type/feature-request
Projects
Alerting
Status: Backlog
Milestone
No milestone
4 participants
@JohnnyQQQQ @wasim-nihal @tonypowa @eloo-abi