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

fix(controllers) expand TypeMeta population #4758

Closed
wants to merge 2 commits into from
Closed

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Sep 28, 2023

What this PR does / why we need it:

Expands TypeMeta population (see earlier work). Predicting where we'll need this isn't always easy, so just stuffing it in every controller to be safe.

Add TypeMeta population to controller reference predicates.

Use a scheme-based helper to populate TypeMeta from a runtime.object instead of local GVK helpers or pre-defined objects.

Adds a test to confirm credentials update properly.

Which issue this PR fixes:

The added test breaks without the predicate changes. Updating a credential Secret (and presumably other referenced Secrets) after its initial load would not modify Kong configuration.

We add referent Secrets directly to the store from reference updaters, This populates the Secret initially, but future updates need to run through the Secret reconciler. The reconciler predicate was not building the reference key properly without TypeMeta and would never reconcile Secrets with references.

Special notes for your reviewer:

Ideally we could centralize TypeMeta injection, but I don't think we can (barring an upstream fix). I initially tried adding it to store.Add(), but this doesn't work for anything that might need it in the reconciler. AFAIK there's nowhere in controller-runtime that'd let us add a global preprocessing step.

Trying to load manager configuration outside the manager results in a package import loop that I'm not particularly inclined to untangle. Removing feature gates for the scheme builder should be fine, since simply being aware of types doesn't do anything on its own.

This came out of research on #4672, but I'm still not fully sure why outdated CRDs trigger this. I suspect that the issue is somewhere in the reference builder store update since the predicate was never working, and that updater is seemingly the only way into the store that bypasses the reconciler.

Sadly, my reproduction steps (disable CRD installation in the test harness, helm install wat kong/kong --version 2.13.1; helm delete to get the old CRDs, install consumer group CRD and GWAPI) stopped reproducing the issue after I located this second store insert path, so I wasn't able to explore it fully.

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

@rainest rainest requested a review from a team as a code owner September 28, 2023 22:22
Add a type-agnostic helper to populate TypeMeta using a scheme with
registered types.

Populate TypeMeta throughout all controllers and reference filter
predicates.

Remove feature gate limitations on types added to the manager scheme to
simplify its use where config is not readily available.
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Attention: 113 lines in your changes are missing coverage. Please review.

Comparison is base (edfd7e2) 67.5% compared to head (e649d08) 50.4%.
Report is 6 commits behind head on main.

❗ Current head e649d08 differs from pull request most recent head fee389a. Consider uploading reports for the commit fee389a to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##            main   #4758      +/-   ##
========================================
- Coverage   67.5%   50.4%   -17.2%     
========================================
  Files        163     163              
  Lines      18667   18563     -104     
========================================
- Hits       12609    9359    -3250     
- Misses      5323    8309    +2986     
- Partials     735     895     +160     
Files Coverage Δ
internal/manager/setup.go 40.5% <100.0%> (-35.2%) ⬇️
internal/controllers/gateway/gateway_controller.go 67.7% <20.0%> (+11.5%) ⬆️
...nal/controllers/gateway/gatewayclass_controller.go 72.4% <20.0%> (-0.8%) ⬇️
...ternal/controllers/gateway/grpcroute_controller.go 73.0% <20.0%> (+55.7%) ⬆️
...ternal/controllers/gateway/httproute_controller.go 68.6% <20.0%> (+4.3%) ⬆️
...l/controllers/gateway/referencegrant_controller.go 58.3% <20.0%> (+35.0%) ⬆️
...nternal/controllers/gateway/tcproute_controller.go 76.8% <20.0%> (+61.1%) ⬆️
...nternal/controllers/gateway/tlsroute_controller.go 76.3% <20.0%> (+60.3%) ⬆️
...nternal/controllers/gateway/udproute_controller.go 73.1% <20.0%> (+57.3%) ⬆️
internal/manager/scheme/scheme.go 16.0% <20.0%> (-6.3%) ⬇️
... and 5 more

... and 117 files with indirect coverage changes

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

@rainest rainest marked this pull request as draft September 28, 2023 23:36
@rainest
Copy link
Contributor Author

rainest commented Sep 28, 2023

Oh boy! Race conditions!

Back to draft pending that.

@rainest
Copy link
Contributor Author

rainest commented Oct 2, 2023

Github doesn't want to run actions for some reason, on to new PR pastures!

@rainest rainest closed this Oct 2, 2023
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.

None yet

1 participant