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

Report resource errors in DB mode #5785

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Report resource errors in DB mode #5785

merged 6 commits into from
Apr 18, 2024

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Apr 2, 2024

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-runtime client.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 with reflect 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 🤷

  message: 'invalid : HTTP status 400 (message: "schema violation (methods: cannot
    set ''methods'' when ''protocols'' is ''grpc'' or ''grpcs'')")'

Rest of the event looks like event.yaml.txt

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 77.92208% with 17 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@c104c88). Click here to learn what that means.
Report is 10 commits behind head on main.

Files Patch % Lines
internal/dataplane/sendconfig/dbmode.go 79.1% 8 Missing and 6 partials ⚠️
internal/manager/setup.go 50.0% 2 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

internal/dataplane/sendconfig/dbmode.go Outdated Show resolved Hide resolved
internal/dataplane/sendconfig/dbmode.go Outdated Show resolved Hide resolved
@rainest rainest force-pushed the feat/db-error-stream branch 2 times, most recently from e02e7fe to 42cc727 Compare April 16, 2024 23:48
@rainest
Copy link
Contributor Author

rainest commented Apr 16, 2024

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.

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
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).
@rainest rainest marked this pull request as ready for review April 17, 2024 00:48
@rainest rainest requested a review from a team as a code owner April 17, 2024 00:48
@rainest rainest requested a review from randmonkey April 17, 2024 00:48
@pmalek
Copy link
Member

pmalek commented Apr 17, 2024

... breaking config in integration is bad because it's poorly isolated ...

It is isolated in https://github.com/Kong/kubernetes-ingress-controller/tree/main/test/integration/isolated

@rainest rainest requested a review from randmonkey April 17, 2024 20:41
@rainest rainest enabled auto-merge (squash) April 17, 2024 21:53
Copy link
Contributor

@randmonkey randmonkey left a 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.

internal/manager/config.go Show resolved Hide resolved
@rainest rainest merged commit 1c68106 into main Apr 18, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface deck errors as Kubernetes events
4 participants