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 #4767

Merged
merged 7 commits into from
Oct 13, 2023
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Oct 2, 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. Failing that, we can add it in either resource-specific functions or the key generator. The latter is more universal (assuming we don't end up with custom key formats for other types) if a bit cumbersome due to a new error return.

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

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

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

Comparison is base (82dbd35) 78.0% compared to head (e804d0b) 77.8%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4767     +/-   ##
=======================================
- Coverage   78.0%   77.8%   -0.2%     
=======================================
  Files        163     165      +2     
  Lines      18531   18504     -27     
=======================================
- Hits       14458   14401     -57     
- Misses      3264    3281     +17     
- Partials     809     822     +13     
Files Coverage Δ
...nal/controllers/configuration/secret_controller.go 56.0% <ø> (ø)
...trollers/configuration/zz_generated_controllers.go 48.3% <ø> (-2.7%) ⬇️
internal/controllers/gateway/gateway_controller.go 72.2% <ø> (-0.9%) ⬇️
...nal/controllers/gateway/gatewayclass_controller.go 79.2% <ø> (ø)
...ternal/controllers/gateway/grpcroute_controller.go 71.6% <ø> (-1.6%) ⬇️
...ternal/controllers/gateway/httproute_controller.go 75.6% <ø> (-1.1%) ⬇️
...l/controllers/gateway/referencegrant_controller.go 62.7% <ø> (ø)
...nternal/controllers/gateway/tcproute_controller.go 74.0% <ø> (+2.1%) ⬆️
...nternal/controllers/gateway/tlsroute_controller.go 75.7% <ø> (ø)
...nternal/controllers/gateway/udproute_controller.go 73.9% <ø> (+2.1%) ⬆️
... and 7 more

... and 8 files with indirect coverage changes

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

@rainest rainest force-pushed the chore/central-meta branch 2 times, most recently from d0ec5e1 to ccdad4f Compare October 3, 2023 22:20
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.
@rainest rainest marked this pull request as ready for review October 4, 2023 00:50
@rainest rainest requested a review from a team as a code owner October 4, 2023 00:50
@rainest
Copy link
Contributor Author

rainest commented Oct 4, 2023

https:/Kong/kubernetes-ingress-controller/actions/runs/6399242484/attempts/1?pr=4767 failed, but looks like maybe a flake. Doesn't look like it should be particular to router flavor.

Re-running but not staying around to confirm, 🤞

If it works, this is ready for review.

internal/util/types.go Outdated Show resolved Hide resolved
@pmalek
Copy link
Member

pmalek commented Oct 9, 2023

I'm not exactly sure I understand the "why" of this PR 🤔

I can see 2 paragraphs in the PR description

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.

But those don't really explain why would we need this change. Can we elaborate more on the "why" in this PR? Would it be a big effort to work towards the reproduction of the issue that gets fixed here?

@rainest
Copy link
Contributor Author

rainest commented Oct 10, 2023

But those don't really explain why would we need this change. Can we elaborate more on the "why" in this PR? Would it be a big effort to work towards the reproduction of the issue that gets fixed here?

It's in the "What this fixes". If you created a Secret with a credential, it would load into the store initially, but the controller wouldn't actually track updates to it. If you attempted to change a basic-auth password or similar, your old password would continue being valid, and your new one would not work. The test changes reliably fail on existing code.

There was a separate issue where having old CRDs would prevent the initial load also. This is the part that I can't replicate reliably. It's odd, and I'd like to understand it, but it has a known fix that you should do anyway (run with the current CRDs). Absent reliable repro steps that unfortunately remains a curiousity.

@rainest rainest enabled auto-merge (squash) October 11, 2023 21:17
czeslavo and others added 2 commits October 12, 2023 10:48
Override Get to populate TypeMeta for the manager client as a whole,
rather than at the controller definition level.

Rename controllerOpts to managerOpts in manager setup, since that's what
it is.

Restore meta population and scheme retrieval to object key function.

Change a failure condition assert to require in TestConsumerCredential.
@rainest rainest requested a review from czeslavo October 12, 2023 23:19
@rainest rainest merged commit b87e726 into main Oct 13, 2023
34 checks passed
@rainest rainest deleted the chore/central-meta branch October 13, 2023 16:22
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.

4 participants