-
Notifications
You must be signed in to change notification settings - Fork 846
chore: adding metrics for VAP integration #4317
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jaydip Gabani <[email protected]>
| ingestDuration = "constraint_template_ingestion_duration_seconds" | ||
| statusKey = "status" | ||
| ctMetricName = "constraint_templates" | ||
| celCTMetricName = "constraint_templates_with_cel" |
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.
Instead of a tag, I am adding new metrics tracking total CT with CEL, and this can be used with constraint_templates metrics to figure out rego only CT count.
The reason of adding a new metrics rather than a tag as discussed was to maintain backward compatibility. IMO this is better but if we want to do type=CEL,REGO I can revert to that.
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.
Good call on using a separate constraint_templates_with_cel metric instead of adding a type=cel tag to constraint_templates for backward compatibility. Consider adding a brief code comment here explaining this design decision.
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.
Pull request overview
This PR adds comprehensive metrics support for ValidatingAdmissionPolicy (VAP) integration in Gatekeeper, enabling observability into the generation and status of VAP resources alongside constraint templates.
- Introduces three new metrics:
gatekeeper_constraint_templates_with_cel,gatekeeper_validating_admission_policies, andgatekeeper_validating_admission_policy_bindings - Implements thread-safe registry tracking for VAP and VAPB resources with active/error status reporting
- Adds integration tests in BATS to verify metric reporting and removes redundant pod creation calls
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/metrics.md | Documents the three new VAP-related metrics with descriptions, tags, and aggregation types |
| test/testutils/controller.go | Simplifies variable declaration by removing unnecessary parentheses |
| test/bats/test.bats | Adds metrics verification tests for VAP integration and optimizes pod creation to check for existing pods before creating new ones |
| pkg/controller/constrainttemplate/stats_reporter_test.go | Adds comprehensive test coverage for VAP, VAPB, and CEL template metrics including creation, deletion, and status updates |
| pkg/controller/constrainttemplate/stats_reporter.go | Implements metrics infrastructure for VAP and CEL constraint templates with thread-safe registries and observable gauges |
| pkg/controller/constrainttemplate/constrainttemplate_controller.go | Integrates metric reporting throughout the VAP lifecycle including creation, updates, errors, and deletion |
| pkg/controller/constraint/stats_reporter_test.go | Adds test coverage for VAPB metrics with status tracking and deletion scenarios |
| pkg/controller/constraint/stats_reporter.go | Implements VAPB metrics with registry-based tracking and status observation |
| pkg/controller/constraint/constraint_controller.go | Integrates VAPB metric reporting across all generation states including errors and successful binding creation |
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4317 +/- ##
===========================================
- Coverage 54.49% 41.33% -13.16%
===========================================
Files 134 253 +119
Lines 12329 17923 +5594
===========================================
+ Hits 6719 7409 +690
- Misses 5116 9871 +4755
- Partials 494 643 +149
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
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.
can we improve unit test coverage here?
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.
generally constraint tests are covered through CT tests so there is no reconcilation tests for constraint. For this change, I have added unit tests for stats_reporter and have added tests in bats file which should cover the code. Let me know if metrics tests through constraint lifecycle is still desired and I can add them.
| ingestDuration = "constraint_template_ingestion_duration_seconds" | ||
| statusKey = "status" | ||
| ctMetricName = "constraint_templates" | ||
| celCTMetricName = "constraint_templates_with_cel" |
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.
Good call on using a separate constraint_templates_with_cel metric instead of adding a type=cel tag to constraint_templates for backward compatibility. Consider adding a brief code comment here explaining this design decision.
sozercan
left a comment
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.
thanks! added a few comments
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
pkg/controller/constrainttemplate/constrainttemplate_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Jaydip Gabani <[email protected]>
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Signed-off-by: Jaydip Gabani <[email protected]>
| r.reporter.ReportVAPBStatus(vapBindingKey, metrics.VAPStatusError) | ||
| return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, "could not parse timestamp") | ||
| } | ||
| if t.After(time.Now()) { |
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.
do we want to emit a metric while waiting?
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 may not be transient error if parsing timestamp is resulting in an error. That is why I think it may be a good idea to emit metrics. Eventually if VAPB is generated then this metric will be replaced with active status anyways.
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
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.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
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.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
What this PR does / why we need it:
Adds metrics for VAP integration
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #4078
Special notes for your reviewer: