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

Add Syncer result chan #76

Merged
merged 9 commits into from
Apr 11, 2024
Merged

Add Syncer result chan #76

merged 9 commits into from
Apr 11, 2024

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Mar 30, 2024

What this does/why we need it

Adds a new result channel to diff.Syncer. When enabled, the Syncer disables its traditional console output and instead sends structs describing its actions and errors encountered per entity to an exposed channel.

Downstream library consumers can use this to associate actions and errors with entities they manage. KIC can, for example, use these to generate error Events for the Kubernetes resources associated with an entity or update Programmed status on a per-resource basis (currently it only updates resource status based on the latest complete configuration applied, which may not accurately reflect Kong state during partial applies).

Existing consumers (namely deck) will not see any change in behavior. When disabled, traditional streaming event logs print to the console as usual.

Special notes

Derived from initial work in #30. The attempt there raised concurrency concerns and difficulties extracting cprint from the library due to test dependencies.

Draft PoC of usage in Kong/kubernetes-ingress-controller#5785

@rainest rainest force-pushed the syncer-result-chan branch 2 times, most recently from 94f19aa to 95709ed Compare April 2, 2024 00:37
@rainest
Copy link
Contributor Author

rainest commented Apr 2, 2024

@Kong/team-deck this leaves existing console prints in place, unlike #30, but with the expectation that we'd like to convert deck over to them rather than simply disabling them in result channel mode, if only to not have to maintain parallel event reporting code.

Do you see any shortcomings with the design that would prevent that? AFAIK the main need is to have some way to include diff strings, which this does.

gateway-dev failure is presumably something recently added there. Looks like an incompatibility with consumer groups

@rainest rainest marked this pull request as ready for review April 2, 2024 00:52
@rainest rainest requested review from a team April 2, 2024 00:52
pkg/diff/diff.go Outdated Show resolved Hide resolved
pkg/diff/diff.go Show resolved Hide resolved
@rainest
Copy link
Contributor Author

rainest commented Apr 3, 2024

The latest gateway dev failure is due to a new key-auth field.

I'm trying to think of how I want to handle that. The pseudo-golden tests are quite clunky re version differences, and I don't exactly want to make yet another Test_Sync_ConsumerGroupsScopedPlugins_After370 after Test_Sync_ConsumerGroupsScopedPlugins_After350 that's basically identical aside from the one field 😐

pkg/diff/diff.go Outdated Show resolved Hide resolved
pkg/diff/diff.go Outdated Show resolved Hide resolved
pkg/diff/diff.go Show resolved Hide resolved
@rainest
Copy link
Contributor Author

rainest commented Apr 4, 2024

The pseudo-golden tests are both unable to deal with new entity fields for new versions and are structured so that any version-specific carveouts are basically impossible AFAICT ☹️

We do not have version gates for parts of test functionality, only the blanket runWhen() skips. Even if we did, we'd still have to maintain separate expected states or inject the extra field into one, which is still cumbersome.

We could alternately delete fields out of the actual state in testKongState(), but this would be similarly cumbersome.

We can sort of ignore fields in general, but again, same version gate caveats, and you furthermore need to use an incredibly blunt instrument due to kong.Configuration being an arbitrary map[string]interface{}: hack.diff.txt works, but yeesh.

So, sadly, went ahead and just made a 3.7 variant of the tests 😐

@rainest rainest requested review from randmonkey and pmalek April 4, 2024 00:00
@pmalek
Copy link
Member

pmalek commented Apr 4, 2024

The CI seems to be broken for quite some time. Not sure how this went unnoticed. Anyway #79 should fix it.

We can then pull in that fix to this PR to properly run tests.

rainest added 7 commits April 4, 2024 12:40
Add a new diff.EntityAction type and child types. This struct describes
an object and some action taken by the diff engine.

Add a new ResultChan diff.EntityAction field to the diff.Syncer type,
initialize it on Syncer Run(), and close it when Run() completes.

Disable console output when the result channel is enabled.

Together, these provide tooling for the Syncer to report its actions
downstream, rather than sending text descriptions of them direct to
stdout/stderr.
@rainest rainest force-pushed the syncer-result-chan branch from 31a9480 to f02ba1c Compare April 4, 2024 19:40
@rainest rainest enabled auto-merge (squash) April 4, 2024 19:41
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

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

Project coverage is 40.89%. Comparing base (3bf528e) to head (9bdc6ef).

Files Patch % Lines
pkg/diff/diff.go 0.00% 82 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   41.14%   40.89%   -0.26%     
==========================================
  Files          73       73              
  Lines       10139    10202      +63     
==========================================
  Hits         4172     4172              
- Misses       5549     5612      +63     
  Partials      418      418              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/diff/diff.go Show resolved Hide resolved
@rainest rainest merged commit 54a82f6 into main Apr 11, 2024
42 checks passed
@rainest rainest deleted the syncer-result-chan branch April 11, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants