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

Alerting: Update provisioning to validate user-defined UID on create #73793

Merged
merged 14 commits into from
Sep 8, 2023

Conversation

yuri-tceretian
Copy link
Contributor

@yuri-tceretian yuri-tceretian commented Aug 24, 2023

What is this feature?
Fixes provisioning service to validate user-assigned UID of alert rule and contact points, and return error if the UID does not meet the requirements.

Validation does not affect the update operation and therefore, existing resources will not be affected.

Why do we need this feature?
To enforce uniform rules for UIDs across all APIs and databases. In SQLite mode, the long UID will be accepted because the field has TEXT type. However, in MySQL and Postgres - the UID will be rejected by the database.

Also, it will help prevent users from getting in trouble while using manually assigned UIDs. For example, UID like my=abc will be accepted by provisioning but user will not be able to reach out the rule via UI.

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #62904

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@yuri-tceretian yuri-tceretian added this to the 10.2.x milestone Aug 24, 2023
@yuri-tceretian yuri-tceretian requested a review from a team August 24, 2023 18:55
@yuri-tceretian yuri-tceretian requested a review from a team as a code owner August 24, 2023 18:55
@yuri-tceretian yuri-tceretian requested review from rwwiv, JacobsonMT, grobinson-grafana, mildwonkey, suntala and yangkb09 and removed request for a team August 24, 2023 18:55
@yuri-tceretian yuri-tceretian self-assigned this Aug 24, 2023
@yuri-tceretian yuri-tceretian changed the title Alerting: Update provisioning to validate rule's UID on creation Alerting: Update provisioning to validate user-defined UID on creation Aug 24, 2023
@yuri-tceretian yuri-tceretian changed the title Alerting: Update provisioning to validate user-defined UID on creation Alerting: Update provisioning to validate user-defined UID on create Aug 24, 2023
yuri-tceretian and others added 2 commits August 25, 2023 09:39
Co-authored-by: brendamuir <100768211+brendamuir@users.noreply.github.com>
Copy link
Contributor

@rwwiv rwwiv left a comment

Choose a reason for hiding this comment

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

Couple very minor nits around testing, but LGTM 🚀

pkg/util/shortid_generator_test.go Outdated Show resolved Hide resolved
pkg/util/shortid_generator_test.go Show resolved Hide resolved
Copy link
Contributor

@alexweav alexweav left a comment

Choose a reason for hiding this comment

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

LGTM, nice approach with making it so only newly created objects uptake this, so it's not breaking.

What's here is good, though I suspect we could go further - check the PUT for contact points:
image

What happens if you PUT /contact-points/
with:

{
    ...
    "uid": "some-long-invalid-uid"
}

I honestly don't remember the behavior here - does it try to update the actual UID of the contact point? If so, we could potentially also attempt to validate in this case too (as long as we don't break updates if the path UID is already invalid to begin with).

pkg/util/shortid_generator_test.go Outdated Show resolved Hide resolved
Co-authored-by: Alexander Weaver <weaver.alex.d@gmail.com>
@yuri-tceretian
Copy link
Contributor Author

What happens if you PUT /contact-points/

I just tested it and uid in the payload is ignored. Perhaps we need to fail the request if uid is specified.

@yuri-tceretian yuri-tceretian merged commit 99fd7b8 into main Sep 8, 2023
13 checks passed
@yuri-tceretian yuri-tceretian deleted the yuri-tceretian/fix-uid-provisioning branch September 8, 2023 19:09
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
…rafana#73793)

* add ValidateUID to util
* provisioning to validate UID on rule creation

---------

Co-authored-by: brendamuir <100768211+brendamuir@users.noreply.github.com>
Co-authored-by: Alexander Weaver <weaver.alex.d@gmail.com>
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
…73793)

* add ValidateUID to util
* provisioning to validate UID on rule creation

---------

Co-authored-by: brendamuir <100768211+brendamuir@users.noreply.github.com>
Co-authored-by: Alexander Weaver <weaver.alex.d@gmail.com>
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Provisioning API alert_rule validation of limits
5 participants