feat: Added changes to support crud for graphql-cost-decoration custo…#428
feat: Added changes to support crud for graphql-cost-decoration custo…#428shivaygupta-dotcom wants to merge 8 commits intomainfrom
Conversation
…pdated version of k8s Apis
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
- Coverage 28.80% 28.38% -0.42%
==========================================
Files 121 123 +2
Lines 16480 16928 +448
==========================================
+ Hits 4747 4805 +58
- Misses 11116 11497 +381
- Partials 617 626 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…corations in tryRegisterEntityType
| if !skipCustomEntities && len(config.CustomEntityTypes) > 0 { | ||
| // Get custom entities with types given in config.CustomEntityTypes. | ||
| // Register all entity types first (sequentially) to avoid data race on the registry map. | ||
| // The registry is not thread-safe, so we must complete all registrations before |
There was a problem hiding this comment.
We were using a mutex for that. What was the issue here?
There was a problem hiding this comment.
So basically, mutex only protected the Register() (write) operation, but not the subsequent Lookup() (read) that happens inside GetAllCustomEntitiesWithType.
I have got race conditions on one test where we were adding multiple decorations simultaneously, there i have faced this issue and then added this fix.
| for _, entityType := range config.CustomEntityTypes { | ||
| if err := tryRegisterEntityType(client, custom.Type(entityType)); err != nil { | ||
| group.Go(func() error { | ||
| return fmt.Errorf("custom entity %s: %w", entityType, err) |
There was a problem hiding this comment.
What is the purpose of this goroutine?
There was a problem hiding this comment.
The function getProxyConfiguration doesn't return an error directly - it uses errgroup.Group to collect errors from concurrent operations.
| } | ||
| for i := 0; i < len(want); i++ { | ||
| for i := range want { | ||
| if want[i] == nil || have[i] == nil { |
There was a problem hiding this comment.
Why this test fix? Saw similar one in some other PR of yours too.
There was a problem hiding this comment.
Added comment on another PR, I will remove this fix from one of the PR, once another one gets merged.
#376 (comment)
|
Last check under Testing in your PR description seems like a typing error. |
Didn't got this, I have added error case in sync_test. |
3 participants
https://konghq.atlassian.net/browse/FTI-7307
Kong/deck#1909
Summary
This PR adds full CRUD support for the graphql_ratelimiting_cost_decorations custom entity used by Kong's GraphQL Rate Limiting Advanced plugin.
Full changelog
Issues resolved
Fix #XXX
Documentation
Testing