EV-6388: L7 logging through Istio Waypoint Proxy#4769
Conversation
96823e4 to
7514ec0
Compare
739a32a to
17c75c9
Compare
There was a problem hiding this comment.
Pull request overview
Adds cluster-wide L7 access logging for Istio waypoint proxies by rendering class-level defaults (ConfigMap overlay) plus mesh-wide EnvoyFilters in the Istio root namespace, and introduces a new spec.waypointLogging toggle on the Istio CR (defaulting to enabled). The change also centralizes policySyncPathPrefix coordination logic between the Istio and ApplicationLayer controllers.
Changes:
- Render and manage waypoint L7 logging resources (defaults ConfigMap + 2 EnvoyFilters) and add a minimal typed
EnvoyFilterwrapper to avoid the Istio client-go dependency. - Add
Istio.spec.waypointLogging(Enabled/Disabled) and wire it into rendering and image resolution. - Coordinate Felix
policySyncPathPrefixownership between Istio and ApplicationLayer controllers via shared utility predicates/helpers, and remove the waypoint pull-secret replication controller.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/render/istio/l7waypoint.go | New renderer for waypoint defaults ConfigMap + EnvoyFilters enabling ALS and forwarded-header propagation. |
| pkg/render/istio/l7waypoint_test.go | Unit tests validating rendered overlay YAML and EnvoyFilter specs. |
| pkg/render/istio/istio.go | Wires waypoint logging objects into Istio component rendering and resolves the l7-collector image. |
| pkg/render/istio/istio_test.go | Tests for Enterprise-only waypoint L7 resources and WaypointLogging enable/disable behavior. |
| pkg/render/istio/envoyfilter.go | Adds lightweight typed EnvoyFilter + scheme registration helper. |
| pkg/imports/crds/operator/operator.tigera.io_istios.yaml | CRD schema update adding spec.waypointLogging. |
| api/v1/istio_types.go | Adds WaypointLogging field to IstioSpec. |
| api/v1/zz_generated.deepcopy.go | Deepcopy support for the new WaypointLogging field. |
| pkg/controller/utils/utils.go | Adds GetIstio() helper to fetch the Istio CR (NotFound tolerated). |
| pkg/controller/utils/policy_sync.go | New shared policySyncPathPrefix coordination helpers/predicates. |
| pkg/controller/utils/policy_sync_test.go | Unit tests for the new policy-sync coordination helpers. |
| pkg/controller/istio/istio_controller.go | Registers EnvoyFilter + autoscaling/v2 into scheme; adds policySyncPathPrefix reconciliation; removes waypoint secrets controller hookup. |
| pkg/controller/istio/istio_controller_test.go | Adds tests covering policySyncPathPrefix coordination and updates scheme registration. |
| pkg/controller/applicationlayer/applicationlayer_controller.go | Updates Felix policySyncPathPrefix logic to coordinate with Istio state. |
| pkg/controller/applicationlayer/applicationlayer_controller_test.go | Updates/extends tests for policySyncPathPrefix behavior with Istio present. |
| internal/controller/istio_controller.go | Updates RBAC markers (gateway watch removed). |
| pkg/controller/istio/waypoint/waypoint_suite_test.go | Removed (waypoint secrets controller test suite). |
| pkg/controller/istio/waypoint/waypoint_secrets_controller.go | Removed (waypoint pull-secret replication controller). |
| pkg/controller/istio/waypoint/waypoint_secrets_controller_test.go | Removed (tests for waypoint pull-secret replication controller). |
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0c571d7 to
654ba42
Compare
Adds L7 logging for Gateways using the istio-waypoint GatewayClass. The istio controller now creates three static resources in the Istio root namespace (calico-system) and Istio's deployment controller applies them as class-level defaults to all waypoints cluster-wide: - tigera-waypoint-l7-defaults ConfigMap injects the l7-collector sidecar (with --mode=waypoint on the existing ComponentL7Collector image) and the shared emptyDir + Felix CSI volumes into every waypoint pod. - tigera-waypoint-l7-als EnvoyFilter enables gRPC ALS on main_internal. - tigera-waypoint-l7-srcport EnvoyFilter captures the Forwarded header on connect_terminate and propagates the client IP as filter state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The operator's chart ClusterRole now grants only get/list/watch on envoyfilters (the controller-runtime cache lists/watches them cluster-wide). Render a namespace-scoped Role + RoleBinding in the Istio system namespace granting the operator create/update/delete, so its EnvoyFilter mutations are confined to the one namespace it manages them in. The grant is rendered operator-side (not in the Helm chart) because that namespace does not exist at chart-install time; creating it relies on the operator's existing escalate/bind RBAC verbs. Ordered before the EnvoyFilters so the write grant exists first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
IstioRequiresPolicySync returned true for any Enterprise install with an Istio CR, so policySyncPathPrefix (and Felix's nodeagent socket) stayed set even when Istio.Spec.WaypointLogging was explicitly Disabled and the renderer omitted the waypoint l7-collector and EnvoyFilters. The predicate no longer matched the renderer gate. Add a nil-safe (*Istio).WaypointLoggingEnabled helper (default true when the field is unset) and use it from both IstioRequiresPolicySync and the renderer's waypointLoggingEnabled, so the policy-sync predicate and the render gate share one source of truth. Adds unit cases for the unset, Enabled, and Disabled paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
L7WaypointObjects renders five resources (the l7-collector defaults ConfigMap, the EnvoyFilter-writer Role and RoleBinding, and two EnvoyFilters), but the comment in Objects still said three. Update it so future maintainers reasoning about what gets created and deleted are not misled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DesiredPolicySyncPathPrefix cleared policySyncPathPrefix when it held the operator-managed default and neither the applicationlayer nor istio controllers needed it. That field is shared: egressgateway and Gateway API also set the same default and never clear it, so an applicationlayer/istio cleanup reconcile could wipe a default another controller still relied on and break it. Restore the prior "leave as is" contract: preserve any non-empty existing value (custom override or the shared default) and only set the default into an empty field, never clearing a value this path may not own. Update the applicationlayer and istio controller tests, which had been changed to expect the regressive clearing, to assert preservation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
configureIstioDSCPMark wrote the DSCP annotation into fc.Annotations without checking for a nil map. configureIstioAmbientMode initializes the map only when it writes the mode annotation and can return without doing so, so the DSCP write could panic on a nil map during reconciliation. Initialize the map before writing, mirroring the ambient-mode path, and add a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The waypoint L7 test still described "three" resources in two comments, but the renderer manages five (defaults ConfigMap, EnvoyFilter-writer Role and RoleBinding, and two EnvoyFilters). Update both comments so they do not mislead when extending the test or the feature surface area. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
93dbc6b to
fab1da8
Compare
| ) | ||
| // DefaultPolicySyncPrefix is the operator-managed value for | ||
| // FelixConfiguration.policySyncPathPrefix. | ||
| const DefaultPolicySyncPrefix = utils.DefaultPolicySyncPrefix |
There was a problem hiding this comment.
This is declared and exported in 2 different files.
There was a problem hiding this comment.
Removed it from pkg/controller/applicationlayer/applicationlayer_controller.go in b73dd61
| func Add(mgr manager.Manager, opts options.ControllerOptions) error { | ||
| // Register the typed EnvoyFilter so the L7 waypoint resources rendered by | ||
| // pkg/render/istio can be created/updated via the controller-runtime client. | ||
| istio.AddEnvoyFilterToScheme(mgr.GetScheme()) |
There was a problem hiding this comment.
What is the reason that we do this differently from other resources? Others are declared in pkg/apis/register.go. Can we add these here too?
There was a problem hiding this comment.
No reason, moved it to pkg/apis/register.go in faedec2.
| if c.waypointLoggingEnabled() { | ||
| objs = append(objs, L7WaypointObjects(c.cfg.IstioNamespace, c.L7CollectorImage)...) | ||
| } else { | ||
| toDelete = append(toDelete, L7WaypointObjects(c.cfg.IstioNamespace, "")...) |
There was a problem hiding this comment.
Should the list be reversed seeing as the order matters according to l7waypoint.go?
There was a problem hiding this comment.
Yes, you're right. I never noticed it went into a bad state if you had an active waypoint proxy, then disabled it.
Fixed in 7b5bf75.
The applicationlayer controller re-exported DefaultPolicySyncPrefix as an alias of utils.DefaultPolicySyncPrefix, but nothing referenced the alias. Remove it so utils.DefaultPolicySyncPrefix stays the single source of truth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move EnvoyFilter scheme registration out of the istio controller's Add and into pkg/apis, alongside the other third-party types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deleting the Role/RoleBinding before the EnvoyFilters removes permission to delete the EnvoyFilters. Reverse the delete slice so the EnvoyFilters are removed first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- istio_test.go: three -> five resources - Register autoscaling/v2 centrally via pkg/apis AddToSchemes instead of a local AddToScheme call in the istio controller's Add (mirrors the EnvoyFilter scheme-registration change). - Remove the dead exported EnvoyFilterGVK() helper and its test; the render code builds the GVK inline and nothing else referenced it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds L7 logging for every Gateway that uses the istio-waypoint GatewayClass. The istio controller now creates five static resources in the Istio root namespace (calico-system) and Istio's deployment controller applies the class-default ones to all waypoints cluster-wide:
A small typed EnvoyFilter struct is introduced so the component handler (which casts to metav1.ObjectMetaAccessor) can manage the resources without taking on the networking.istio.io client-go dependency.
Description
Release Note
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.