feat() HTTPRoute AllowListing Controller#19
Conversation
* New files:
- pkg/controllers/httproute_controller.go - HTTPRouteAllowlistingReconciler that watches HTTPRoute objects, resolves CIDRs from the existing
allowlist annotations, and creates an AuthorizationPolicy with:
- From rule: resolved CIDRs as remoteIpBlocks
- To rule: spec.hostnames from the HTTPRoute (omitted if no hostnames)
- TargetRefs pointing to the gateway named in ipam.adevinta.com/gateway
- Created in a cross-namespace if ipam.adevinta.com/namespace is set (sophisticated case)
- pkg/controllers/httproute_controller_test.go - 11 tests covering all the scenarios from the DOTO
* Modified files:
- controllers.go - added httpRouteSupportEnabled bool parameter and registration block
- main.go - added --httproute-support-enabled flag
- controllers_test.go - updated 4 call sites to pass the new false argument
* pkg/controllers/httproute_controller.go - annotation predicate
- For(&gatewayApiv1.HTTPRoute{}, builder.WithPredicates(annotationPredicate)) - only HTTPRoutes with allowlist-group or cluster-allowlist-group annotations trigger reconciliation
- UpdateFunc checks both ObjectOld and ObjectNew so annotation removal triggers a reconcile to clean up the orphaned AuthorizationPolicy
* cmd/ingress-allowlisting-controller/main.go - label selector
- New flag --httproute-label-selector (e.g. protect=true)
- When set alongside --httproute-support-enabled, wires a cache.Options.ByObject label selector on HTTPRoute - this is enforced at the API server, so unlabelled HTTPRoutes never enter the informer cache at all
- If the selector string is invalid, the process fails fast at startup
The two mechanisms stack: label selector shrinks what the cache holds, annotation predicate further skips reconciles for things in the cache that have no allowlist annotation.
* test:
- Extracted hasAllowlistAnnotation from the closure into a package-level function - the closure made it untestable in isolation
- TestHasAllowlistAnnotation covers the 5 annotation combinations (local, cluster, both, unrelated, none)
- TestAnnotationPredicate covers all 4 event types, with special attention to the UpdateFunc tricky cases: annotation added, annotation removed (must pass for cleanup), and neither side having an annotation (must be
filtered)
* Controller: - Dropped gatewayAnnotation() and namespaceAnnotation() methods - New gatewayParentRef() helper that finds the first spec.parentRefs entry with kind=Gateway / group=gateway.networking.k8s.io (nil kind/group also match, since Gateway is the default) - Target namespace is now parentRef.Namespace when it differs from httproute.Namespace, otherwise same namespace - no annotation needed - mergeAnnotation() helper returns <prefix>/merge - When merge=<name>, reconcile delegates to reconcileMergedGateway for each cross-namespace ref (same-namespace refs are silently skipped - merge doesn't apply there) - reconcileMergedGateway lists all HTTPRoutes cluster-wide, finds siblings with the same merge=<name> + pointing to the same gateway, unions their CIDRs and hostnames (deduplicating both), then writes a single AuthorizationPolicy named just httproute.Name in the gateway namespace - Without merge=<name> the existing path runs unchanged - buildAuthorizationRules extracted as a shared helper used by both paths * Tests: - All tests migrated from ipam.adevinta.com/gateway + ipam.adevinta.com/namespace annotations to spec.parentRefs - TestReconcileHTTPRouteNoGatewayAnnotation renamed to TestReconcileHTTPRouteNoParentRef - New TestGatewayParentRef with 6 subtests covering the matching logic (nil defaults, wrong kind, wrong group, multiple refs, namespace field) - Added parentRef() helper to keep test fixtures concise - TestReconcileHTTPRouteMerge - two routes in different namespaces produce one policy with merged CIDRs and hostnames - TestReconcileHTTPRouteMergeDeduplicatesIPs - both routes have identical CIDR lists, policy contains no duplicates - TestReconcileHTTPRouteMergeIgnoresDifferentName - a differently-named route pointing to the same gateway is not merged in - TestReconcileHTTPRouteMergeWithoutMergeAnnotationIsIndependent - route without the annotation uses the normal <namespace>-<name> path * Helm: - values.yaml - added httproute.enabled: false and httproute.labelSelector: "" - deployment.yaml - wires --httproute-support-enabled and optionally --httproute-label-selector when labelSelector is set - rbac.yaml - adds httproutes watch + authorizationpolicies full CRUD under httproute.enabled, as a ClusterRole/ClusterRoleBinding so it covers all namespaces including cross-namespace gateway targets
This is very edge case, but easy to fix, where a sibling left this group (change merge) and all routes from previous merge group should be reconciled.
1. Gateway vs HTTPRoute — don't mix 2. DENY policy for hostname protection 3. Namespace selector on the Gateway listener [skip ci]
The merge sibling watch now only fires when a merge annotation actually changes from a non-empty value to a different value.
There was a problem hiding this comment.
thanks for the contribution, overall looks sane there only one thing i don't like.
Merge mode (ipam.adevinta.com/merge=<key>) - for staging environments where the same service is deployed across multiple namespaces pointing to a shared gateway. All HTTPRoutes sharing the same merge key and gateway produce a single AP with unioned CIDRs and hostnames. Not recommended for production - see security warning in README.
Why do we need such merge mode? that optimization to reuse the same authorization policy makes it harder to debug and reason about, what is wrong on having an Authorization Policy per route?
@InsomniaCoder wdyt?
Fsero
left a comment
There was a problem hiding this comment.
about
+The HTTPRoute AP bypasses the Gateway AP entirely. **Do not use both `allowlist-group` on a Gateway and on its HTTPRoutes simultaneously.** Pick one level:
+
+- Use **Gateway-level** when you want a single uniform allowlist for all traffic through the gateway, regardless of hostname.
+- Use **HTTPRoute-level** when you need per-route control with different CIDRs per service.
i think we should discourage the use of AuthorizationPolicy at GatewayLevel the main point of using this controller is having granularity.
I think I agree that it makes it harder to debug and reason about. @sielaq what about splitting the merge mode in a follow up PR? |
|
|
after chatting IRL with @sielaq i understand merge feature is optional! |
Idea is to make it more modular. for ex.: to add Traefik (or NGinx GF, Kong) in the future, interface(s) got implemented. so we can just register in controllers.go - and nothing else changes. NEW: * separate k8s writers (istio_l4 / istio_l7 / istio_l7_merge) * create tests accordingly * RBAC fails on start when permissions are missing - each writers must expose RequiredPermissions info this is very useful for debugging * introduce README2.md for deeper dive about conventions, controller support matrix etc. * gateway class auto detection -> pick equivalent writer
|
@Fsero and @InsomniaCoder after discussion with Fabian, I took his idea to make it more modular and re-implement it with interfaces. Kindly please review once again |
0cc5927 to
1cb3f0f
Compare
* context.Background() instead of propagated ctx * mergeTosByPaths path key not sorted - without sorting: two to ops with same paths in different order miss the merge * reconcileMergedGateway handles the case where siblings are still actively reconciling (confirmed-empty guard) * cleanupOrphanedMergedAPs handles the case where the departing route itself was the last member it scans managed APs for orphaned merge groups and cleans them up with the same APIReader * guard in buildAuthorizationRules: return nil rules if allowedIPs is empty * use fancy dedupSorted() instead of cp+sort * when "allowlisting: enabled" is removed, controller-runtime evicts the route from the label-selector cache * adding more test
Adds support for HTTPRoute resources alongside the existing Ingress and Gateway controllers. Enabled via
--httproute-support-enabledflag orhttproute.enabled: truein Helm.How it works
The controller watches HTTPRoute objects and creates Istio AuthorizationPolicy resources. Gateway name and target namespace are derived automatically from spec.parentRefs - no extra annotations required beyond the existing
ipam.adevinta.com/allowlist-group / cluster-allowlist-group.Naming:
Merge mode (
ipam.adevinta.com/merge=<key>) - for staging environments where the same service is deployed across multiple namespaces pointing to a shared gateway. AllHTTPRoutessharing the same merge key and gateway produce a single AP with unioned CIDRs and hostnames. Not recommended for production - see security warning in README.Performance - annotation predicate filters reconcile events to only
HTTPRouteswith allowlist annotations. Optional--httproute-label-selectorrestricts the informer cache at the API server level for clusters withthousands of
HTTPRoutes.Labels - every managed AP carries
app.kubernetes.io/managed-by,ipam.adevinta.com/owner-namespace, andipam.adevinta.com/owner-namelabels, enabling hard reset (viakubectl delete authorizationpolicies -A -l app.kubernetes.io/managed-by=ingress-allowlisting-controller).Orphan cleanup - on first reconcile after restart the controller sweeps all its APs and deletes any whose owner
HTTPRouteno longer exists or would no longer produce that AP (covers: route deleted, parentRef removed, switch to/from merge mode). Cross-namespace APs cannot use k8s GC (owner refs are stripped cross-namespace), so restart-based cleanup is the designed solution.