-
Notifications
You must be signed in to change notification settings - Fork 592
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
Report resource errors in DB mode #5785
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5785 +/- ##
======================================
Coverage ? 72.0%
======================================
Files ? 178
Lines ? 18312
Branches ? 0
======================================
Hits ? 13188
Misses ? 4132
Partials ? 992 ☔ View full report in Codecov by Sentry. |
e02e7fe
to
42cc727
Compare
Testing error event generation is always kinda wonky because we don't really want to do it in integration (breaking config in integration is bad because it's poorly isolated) or E2E (expensive) and don't have an admin API otherwise. I was able to follow #4813 somewhat to produce a very basic approximation of DB mode in envtest for only one entity type, which IMO is good enough. |
42cc727
to
800fc85
Compare
Add an argument that forces leadership to one mode or the other. Disable it for integration and env tests. The leadership election mechanism can behave strangely in tests and isn't relevant for single-instance tests.
Use the GDR result channel to log actions and build resource errors from failed actions. chore: update CHANGELOG
800fc85
to
a375496
Compare
Add minimal envtest scaffolding to test DB mode error event generation. Fix a bug where the DB mode resource error array was accessed by value instead of by reference (via its class instance's pointer).
a375496
to
68e0e3a
Compare
It is isolated in https://github.com/Kong/kubernetes-ingress-controller/tree/main/test/integration/isolated |
502c23c
to
ff1b179
Compare
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.
Mostly LGTM, one simple comment reamaining.
What this PR does / why we need it:
Uses the GDR Syncer result channel to receive individual CRUD events and errors from sync operations. Log events and generate resource errors from sync errors, which are then output as Kubernetes Events.
Which issue this PR fixes:
Fix #3492
Special notes for your reviewer:
You can finally set leadership mode via argument again! I don't actually really want this, but it's the easiest and least hacky way to disable it for testing. For debugging integration I usually had it just off in a temporary code change, but for DB mode envtest it breaks always, so I went ahead and added the flag. The other option was to use some special environment variable if we wanted to make it more hidden.
GDR discards most type information internally and just tosses
any
objects around. We would need a large scale refactor to establish something like the controller-runtimeclient.Object
interface to handle common features of Kong entities to change this, and I doubt that's happening any time soon. As such, this does horrible things withreflect
that rely on reasonable assumptions about Kong entities generated by KIC, with some error handling in case that breaks horribly.GDR errors lack the per-field errors that DB-less flattened errors provide. We can mostly work around this by just not including field information, though this does result in a cosmetic blemish in Events. We end up with an empty space where the field name should be. The field is still mentioned in the error string, so 🤷
Rest of the event looks like event.yaml.txt
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR