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

operator: Add alertingrule tenant id label for all rules #8748

Merged
merged 4 commits into from
Mar 15, 2023

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Mar 8, 2023

What this PR does / why we need it:
The following PR adds a AlertingRule defaulting webhook is to populate the tenant ID to all rules to enable managing Alertmanager notifications per tenant.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@periklis periklis requested a review from xperimental March 8, 2023 14:32
@periklis periklis requested a review from a team as a code owner March 8, 2023 14:32
@periklis periklis self-assigned this Mar 8, 2023
Copy link
Contributor

@Red-GV Red-GV left a comment

Choose a reason for hiding this comment

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

Few nits. lgtm otherwise.

@periklis periklis force-pushed the alerts-tenant-label branch from 1483748 to 3a603c0 Compare March 10, 2023 12:55
@pull-request-size pull-request-size bot added size/S and removed size/L labels Mar 10, 2023
@periklis periklis changed the title operator: Add alertingrule defaulting webhook operator: Add alertingrule tenant id label for all rules Mar 10, 2023
@periklis periklis force-pushed the alerts-tenant-label branch from 3a603c0 to c17bcee Compare March 10, 2023 12:56
Copy link
Contributor

@Red-GV Red-GV left a comment

Choose a reason for hiding this comment

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

lgtm

@periklis periklis force-pushed the alerts-tenant-label branch from 3cb005f to d9263a5 Compare March 15, 2023 11:02
Copy link
Collaborator Author

@periklis periklis left a comment

Choose a reason for hiding this comment

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

self-review

rule.Labels = map[string]string{}
}

rule.Labels[tenantLabel] = a.Spec.TenantID
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I thought I had posted this comment last week already, but apparently I never clicked "publish" ...

It looks like we need to do a DeepCopy somewhere, because we are modifying the Groups (dirtying the lister cache). These are all pointer-types, so not only the one in alertingRuleSpec is modified but also the original in AlertingRule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah you posted this on slack, I started working it out and stashed it. It got forgotten :(

@periklis periklis merged commit 52f6b82 into grafana:main Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants