[ON HOLD - CODE FREEZE] feat: Convert auth policy reconciler to TypedAction format#2007
Open
philbrookes wants to merge 4 commits into
Open
[ON HOLD - CODE FREEZE] feat: Convert auth policy reconciler to TypedAction format#2007philbrookes wants to merge 4 commits into
philbrookes wants to merge 4 commits into
Conversation
Replace legacy wasm.Action with wasm.TypedAction for auth policies. The auth action is now a type:"grpc" TypedAction with a CheckRequest messageBuilder, onReply handlers (deny/fail/store/headers/fail), and grpcService/grpcMethod on the auth service config. Closes #1996 Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
📝 WalkthroughWalkthroughConverts auth policy WASM action generation to typed gRPC actions with on-reply decision sequences, adds TypedAction Path/Value fields and equality/tests, validates typed-action CEL predicates, and propagates validated typed actions through gateway and Istio reconcilers into wasm action-set construction; tests updated. ChangesAuth policy WASM typed action conversion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2007 +/- ##
========================================
Coverage 74.97% 74.98%
========================================
Files 127 127
Lines 12515 12630 +115
========================================
+ Hits 9383 9470 +87
- Misses 2650 2670 +20
- Partials 482 490 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/auth_workflow_helpers.go`:
- Around line 116-124: The function joinPredicates should preserve each
predicate's internal boolean grouping by parenthesizing every predicate before
combining; update joinPredicates to map over predicates and wrap each entry with
"(" + predicate + ")", then join those wrapped predicates with " && " (and still
return "" for len==0). Refer to function joinPredicates and the current use of
strings.Join when making this change.
In `@internal/controller/envoy_gateway_extension_reconciler.go`:
- Around line 415-417: The new gRPC auth TypedAction built by
buildWasmTypedActionsForAuth (driven by effectiveAuthPoliciesMap/pathID)
requires the kuadrant-operator-grpc descriptor cluster, but
reconcileUpstreamClusters currently skips buildUpstreamEnvoyPatchPolicy when
gatewayUpstreams is empty; update reconcileUpstreamClusters to detect when auth
TypedActions are present (or when effectiveAuthPoliciesMap is non-empty) and
invoke buildUpstreamEnvoyPatchPolicy (or otherwise add the
kuadrant-operator-grpc cluster) even if gatewayUpstreams is empty so the
descriptor cluster is reconciled whenever auth TypedActions are emitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee57d40e-c869-42a2-8aae-97af87e6942d
📒 Files selected for processing (8)
internal/cel/kuadrant_validator.gointernal/controller/auth_workflow_helpers.gointernal/controller/auth_workflow_helpers_test.gointernal/controller/envoy_gateway_extension_reconciler.gointernal/controller/istio_extension_reconciler.gointernal/wasm/types.gointernal/wasm/types_test.gointernal/wasm/utils.go
Wrap each predicate in parentheses before joining with && to preserve correct boolean semantics when predicates contain || operators. Remove len(gatewayUpstreams) == 0 guard in EnvoyGateway reconciler so the proto descriptor cluster is always created, matching the Istio reconciler fix. Update integration tests for both Istio and EnvoyGateway to assert auth actions on TypedActions instead of legacy Actions. Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/envoygateway/extension_reconciler_test.go (1)
521-524: ⚡ Quick winMake auth typed-action assertions deterministic
Line 521 (and equivalent blocks below) indexes
TypedActions[0]after only checking non-emptiness. If another typed action is introduced, this can assert against the wrong element. Prefer asserting exactly one auth typed action, or selecting byService == wasm.AuthServiceNamebefore asserting sources.Suggested pattern
- g.Expect(existingWASMConfig.ActionSets[0].TypedActions).ToNot(BeEmpty()) - g.Expect(existingWASMConfig.ActionSets[0].TypedActions[0].SourcePolicyLocators).To(Equal([]string{ - "authpolicy.kuadrant.io:" + gwAuthPolicyKey.String(), - })) + authActions := make([]wasm.TypedAction, 0, len(existingWASMConfig.ActionSets[0].TypedActions)) + for _, a := range existingWASMConfig.ActionSets[0].TypedActions { + if a.Service == wasm.AuthServiceName { + authActions = append(authActions, a) + } + } + g.Expect(authActions).To(HaveLen(1)) + g.Expect(authActions[0].SourcePolicyLocators).To(Equal([]string{ + "authpolicy.kuadrant.io:" + gwAuthPolicyKey.String(), + }))Also applies to: 577-583, 652-655, 707-713, 781-784, 835-841, 909-912, 964-970
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/envoygateway/extension_reconciler_test.go` around lines 521 - 524, The test currently assumes the auth typed-action is at index 0 after checking TypedActions is non-empty; instead locate the auth action deterministically by finding the element in existingWASMConfig.ActionSets[0].TypedActions whose Service equals wasm.AuthServiceName (or assert there is exactly one matching element) and then assert its SourcePolicyLocators equals []string{"authpolicy.kuadrant.io:" + gwAuthPolicyKey.String()}; apply the same change to the other blocks that reference TypedActions[0] (lines around 577-583, 652-655, 707-713, 781-784, 835-841, 909-912, 964-970) to avoid brittle index-based assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/envoygateway/extension_reconciler_test.go`:
- Around line 521-524: The test currently assumes the auth typed-action is at
index 0 after checking TypedActions is non-empty; instead locate the auth action
deterministically by finding the element in
existingWASMConfig.ActionSets[0].TypedActions whose Service equals
wasm.AuthServiceName (or assert there is exactly one matching element) and then
assert its SourcePolicyLocators equals []string{"authpolicy.kuadrant.io:" +
gwAuthPolicyKey.String()}; apply the same change to the other blocks that
reference TypedActions[0] (lines around 577-583, 652-655, 707-713, 781-784,
835-841, 909-912, 964-970) to avoid brittle index-based assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3725a17-31ea-411d-b7e7-f22da4e773c0
📒 Files selected for processing (5)
internal/controller/auth_workflow_helpers.gointernal/controller/auth_workflow_helpers_test.gointernal/controller/envoy_gateway_extension_reconciler.gotests/envoygateway/extension_reconciler_test.gotests/istio/extension_reconciler_test.go
💤 Files with no reviewable changes (1)
- internal/controller/envoy_gateway_extension_reconciler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/auth_workflow_helpers.go
- internal/controller/auth_workflow_helpers_test.go
Restore the len(gatewayUpstreams) == 0 guard in both Istio and EnvoyGateway upstream cluster reconcilers. The guard removal was unnecessary — auth TypedAction uses the wasm-shim's built-in auth proto descriptors, not the descriptor service cluster. Update integration test expectations to include GrpcService and GrpcMethod fields on the auth service config, matching the new service builder output. Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/controller/envoy_gateway_extension_reconciler.go (1)
194-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep reconciling the upstream patch for auth-only gateways.
This guard skips the only path that adds the
kuadrant-operator-grpcdescriptor cluster.buildWasmConfigscan still emit auth gRPCTypedActions when a gateway has no registered upstreams, and the stale-policy cleanup below can then delete any existing patch, so those gateways lose descriptor support at runtime.Suggested fix
- if len(gatewayUpstreams) == 0 { - continue - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/envoy_gateway_extension_reconciler.go` around lines 194 - 196, The current early-return that skips processing when len(gatewayUpstreams) == 0 prevents adding the kuadrant-operator-grpc descriptor cluster for auth-only gateways; remove or modify that guard so buildWasmConfigs and the patch apply logic still run for gateways with zero upstreams (i.e., allow buildWasmConfigs to emit auth gRPC TypedAction even when gatewayUpstreams is empty) and ensure downstream stale-policy cleanup does not delete the descriptor-only patch—update the logic around gatewayUpstreams, buildWasmConfigs, and the patch/cleanup section to treat empty upstreams as valid for auth-only descriptor patching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/controller/envoy_gateway_extension_reconciler.go`:
- Around line 194-196: The current early-return that skips processing when
len(gatewayUpstreams) == 0 prevents adding the kuadrant-operator-grpc descriptor
cluster for auth-only gateways; remove or modify that guard so buildWasmConfigs
and the patch apply logic still run for gateways with zero upstreams (i.e.,
allow buildWasmConfigs to emit auth gRPC TypedAction even when gatewayUpstreams
is empty) and ensure downstream stale-policy cleanup does not delete the
descriptor-only patch—update the logic around gatewayUpstreams,
buildWasmConfigs, and the patch/cleanup section to treat empty upstreams as
valid for auth-only descriptor patching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a22c3a6-2842-42cf-83fd-024ef693171e
📒 Files selected for processing (4)
internal/controller/envoy_gateway_extension_reconciler.gointernal/controller/istio_extension_reconciler.gotests/envoygateway/extension_reconciler_test.gotests/istio/extension_reconciler_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/envoygateway/extension_reconciler_test.go
- internal/controller/istio_extension_reconciler.go
The GrpcService and GrpcMethod fields cause the wasm-shim to use the DynamicService path for auth, which requires the kuadrant-operator-grpc descriptor cluster. That cluster only exists when extensions register upstreams, so auth-only gateways get 503 errors. The current wasm-shim uses the static AuthTask handler based on type: "auth", which doesn't need these fields. They should be added in a follow-up when wasm-shim DynamicService auth support lands. Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Comment on lines
-92
to
+95
| func buildWasmActionsForAuth(pathID string, effectivePolicy EffectiveAuthPolicy) []wasm.Action { | ||
| const authResponseVar = "auth_response" | ||
|
|
||
| func buildWasmTypedActionsForAuth(pathID string, effectivePolicy EffectiveAuthPolicy) []wasm.TypedAction { |
Member
There was a problem hiding this comment.
nit: I'd be tempted to just leave the name of the function still as buildWasmActionsForAuth just because we'll likely change TypedAction -> Action once we remove legacy config when all this migration is done for all things
crstrn13
reviewed
Jun 1, 2026
| scope := AuthConfigNameForPath(pathID) | ||
| predicate := joinPredicates(spec.Predicates.Into()) | ||
| if predicate == "" { | ||
| predicate = "true" |
Contributor
There was a problem hiding this comment.
nit: maybe we could have a constant for the "true" cel expression or something?
Contributor
There was a problem hiding this comment.
Just noticed internal/extension/registry.go:1059 - predicateOrTrue. We could reuse that potentially.
philbrookes
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wasm.Actionwithwasm.TypedActionfor auth policies. The auth action is now atype:"grpc"TypedAction with a CheckRequestmessageBuilder,onReplyhandlers (deny/fail/store/headers/fail), andgrpcService/grpcMethodon the auth service config.PathandValuefields toTypedActionfor thestoreaction type used in auth's onReply chain (dynamic metadata from Authorino).ValidateTypedActionfor CEL validation of typed actions (validates top-level predicate, skips onReply predicates which reference wasm-shim runtime variables).BuildActionSetsForPath.Closes #1996
Test plan
make test-unit— all tests passmake run-lint— no lint issuesbuildWasmTypedActionsForAuth,buildAuthOnReply,buildAuthMessageBuilder,joinPredicates, and JSON round-trip🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behaviour Changes
Tests