Booleanize the controller flags #1638
Conversation
Change flags for managing controllers from a custom enabled/auto/disabled type to booleans. By default, all controllers are enabled. If a controller's disable flag is set, it is disabled. Enabled controllers that have an auto loader are loaded if they are enabled and their auto loader indicates they should load. Remove the KongStateEnabled flag. This was a leftover from an earlier design where the controller managed Kong state information via a special Secret. It had no remaining associated code and was only present in configuration. Rename controller flags to reflect that they disable controllers when set. Remove enablement status type, associated utility functions, and custom flag handler.
Move deferred body close checks after the error checkers. If an error is present, no body exists, and the deferred attempt to close it segfaults.
There was a problem hiding this comment.
I'm in favor of this change personally, as I'm convinced that the simplification here will make using the controller manager more straightforward in a time when we don't have automated controller loading.
In fact I ended up recently proposing something very similar:
If we do end up going this way @rainest I would ask that you check out the diff there and perhaps incorporate some of the refactors I did there as well if it seems like a good fit to you.
Change the controller flags from disablers to enablers and flip the logic that handles them. Remove unnecessary pointer usage.
I had done a refactor of how we manage legacy vs V1 Ingress controller setup that if you feel like it wouldn't be too much of a pain to include I would like to see added, but not a blocker by any means. |
Co-authored-by: Michał Flendrich <[email protected]>
What this PR does / why we need it:
Make controller flags booleans. Do what makes sense based on those booleans and the environment, on top of #1585, see #1580 (comment)
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review:theCHANGELOG.mdrelease notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PRActual changes would introduce annoying merge conflict stuff against other stuff in flight, but this does change flag semantics. We should update changelog accordingly if this goes into #1585, but should do that after in #1585