feat(health): support various policies _.microgateway.airlock.com#28268
feat(health): support various policies _.microgateway.airlock.com#28268root30 wants to merge 1 commit into
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Bundle ReportBundle size has no change ✅ |
Signed-off-by: Roman Willi <roman.willi@ergon.ch>
root30
force-pushed
the
resource-customizations-microgateway-policies
branch
from
016b61e to
90bd417
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #28268 +/- ##
==========================================
- Coverage 64.84% 64.82% -0.02%
==========================================
Files 425 425
Lines 59092 59092
==========================================
- Hits 38317 38307 -10
- Misses 17217 17224 +7
- Partials 3558 3561 +3 ☔ View full report in Codecov by Harness. |
| @@ -0,0 +1,18 @@ | |||
| apiVersion: microgateway.airlock.com/v1alpha1 | |||
| kind: XCustomResource | |||
There was a problem hiding this comment.
I'm not loving the idea of using contrived resource kinds for tests. The wildcard pattern allows us to avoid duplicating a lot of script logic. But it shouldn't be used as a way to avoid writing thorough tests.
Is there a strong reason why we can't use actual examples for the tests?
There was a problem hiding this comment.
Please see my detailed response in the other conversation.
However regarding your question: We can use an actual custom resource of ours for the unknown test case if this is the sole concern.
| local hs = { status = "Progressing", message = "Waiting for " .. (obj.kind or "Policy") .. " status"} | ||
|
|
||
| local function is_policy_kind(kind) | ||
| return kind ~= nil and string.match(kind, "Policy$") ~= nil |
There was a problem hiding this comment.
I don't like the idea of matching a kind suffix. If a new kind is added that matches this suffix, there's no reason to believe it'll necessarily follow the same status field conventions. I'd recommend instead hard-coding a static list of kinds that we know this check supports.
There was a problem hiding this comment.
Thanks for raising these concerns.
For the first point, the decision to avoid using a fixed list of concrete resource kinds was intentional. We want to avoid having to update the HealthCheck implementation every time a new policy kind is introduced, as that would require a separate PR just to register the new kind. The wildcard-based approach allows the HealthCheck to automatically support additional policy resources as they are added to our implementation.
Regarding the tests, the goal was not to avoid thorough test coverage. The additional policies we plan to support are Gateway API direct attached policies, which follow the same status conventions as existing resources such as BackendTLSPolicy. Because the HealthCheck logic is based on those shared conventions rather than kind-specific behavior, we considered it more valuable to test the common behavior than to duplicate equivalent tests for every individual policy kind.
For the second point, we agree that blindly matching arbitrary suffixes would be risky if unrelated resources could satisfy the pattern. In this case, however, the expectation is that future matching resources will be Gateway API policy kinds that share the same status structure and semantics. The intent behind the suffix matching is to make the HealthCheck extensible without requiring code changes whenever a new policy kind is introduced. If we hard-code the supported kinds, we lose that extensibility and would need to keep the list synchronized as our implementation evolves.
There was a problem hiding this comment.
Are you a maintainer of microgateway? I want to make sure we have solid reason to believe the future CRDs will follow existing conventions. A link to a docs page stating this would be extra valuable. :-)
2 participants
This PR adds the custom health check for various policy Types of Airlock Microgateway.
Checklist: