Skip to content

Comments

cec: support for explicit control of Cilium Envoy filter injection#5

Open
MitchLewis930 wants to merge 1 commit intopr_045_beforefrom
pr_045_after
Open

cec: support for explicit control of Cilium Envoy filter injection#5
MitchLewis930 wants to merge 1 commit intopr_045_beforefrom
pr_045_after

Conversation

@MitchLewis930
Copy link

PR_045

Currently, the Cilium Envoy network- and L7 filters are always automatically
injected when the CiliumEnvoyConfig is used for L7LB (parameter `isL7LB` - that
is set to true when `Spec.Services` are defined on the CEC).

This commit adds the possibility for a more explicit configuration of this
behaviour by adding the annotation `cec.cilium.io/inject-cilium-filters`.

If the annotation is present on the `CiliumEnvoyConfig` it is used to decide
whether Cilium Envoy filters should be automatically injected or not.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@greptile-apps
Copy link

greptile-apps bot commented Jan 30, 2026

Greptile Overview

Greptile Summary

This PR introduces explicit control over Cilium Envoy filter injection through a new annotation cec.cilium.io/inject-cilium-filters. Previously, Cilium filters were automatically injected only when L7 load balancing services were defined. This change allows users to explicitly enable or disable filter injection via annotation, overriding the default behavior.

Key Changes:

  • Added new CECPrefix and CECInjectCiliumFilters annotation constants
  • Created injectCiliumEnvoyFilters() helper function to determine filter injection based on annotation or service presence
  • Separated isL7LB parameter from filter injection logic in parseResources()
  • Added comprehensive test coverage for the new annotation-based control

Critical Issue Found:

  • addCiliumEnvoyConfig() at line 92-93 incorrectly passes len(cecSpec.Services) > 0 twice instead of calling injectCiliumEnvoyFilters(), breaking the annotation-based control for newly added configs

Confidence Score: 1/5

  • This PR has a critical bug that breaks the main feature for new CEC additions
  • The implementation contains a critical logic error in addCiliumEnvoyConfig that prevents the annotation-based filter control from working when adding new CiliumEnvoyConfig resources. While update and delete paths are correct, and test coverage is comprehensive, the bug in the add path means the feature's primary use case is broken.
  • Pay close attention to pkg/ciliumenvoyconfig/cec_manager.go - the bug at lines 92-93 must be fixed

Important Files Changed

Filename Overview
pkg/ciliumenvoyconfig/cec_manager.go Critical bug in addCiliumEnvoyConfig - incorrect parameter passed, preventing annotation-based filter control
pkg/annotation/k8s.go Added new CEC annotation constant and prefix, simple and safe change
pkg/ciliumenvoyconfig/cec_resource_parser.go Separated isL7LB and injectCiliumEnvoyFilters parameters for explicit filter injection control

Sequence Diagram

sequenceDiagram
    participant User as User/K8s API
    participant CEC as CEC Manager
    participant Helper as injectCiliumEnvoyFilters
    participant Parser as CEC Resource Parser
    participant Envoy as Envoy Config

    User->>CEC: Create/Update CiliumEnvoyConfig
    Note over User,CEC: With annotation cec.cilium.io/inject-cilium-filters
    
    CEC->>Helper: injectCiliumEnvoyFilters(metadata, spec)
    
    alt Annotation exists
        Helper->>Helper: Parse annotation value
        alt Valid boolean
            Helper-->>CEC: Return parsed boolean
        else Invalid value
            Helper-->>CEC: Fall back to len(spec.Services) > 0
        end
    else No annotation
        Helper-->>CEC: Return len(spec.Services) > 0
    end
    
    CEC->>Parser: parseResources(..., injectCiliumEnvoyFilters, ...)
    
    alt injectCiliumEnvoyFilters == true
        Parser->>Parser: Inject Cilium Network filters
        Parser->>Parser: Inject Cilium L7 filters
        Parser-->>CEC: Resources with Cilium filters
    else injectCiliumEnvoyFilters == false
        Parser-->>CEC: Resources without Cilium filters
    end
    
    CEC->>Envoy: Apply Envoy configuration
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 92 to +93
len(cecSpec.Services) > 0,
len(cecSpec.Services) > 0,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect parameter passed to parseResources - should call injectCiliumEnvoyFilters(&cecObjectMeta, cecSpec) instead of len(cecSpec.Services) > 0

Suggested change
len(cecSpec.Services) > 0,
len(cecSpec.Services) > 0,
len(cecSpec.Services) > 0,
injectCiliumEnvoyFilters(&cecObjectMeta, cecSpec),
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/ciliumenvoyconfig/cec_manager.go
Line: 92:93

Comment:
incorrect parameter passed to `parseResources` - should call `injectCiliumEnvoyFilters(&cecObjectMeta, cecSpec)` instead of `len(cecSpec.Services) > 0`

```suggestion
		len(cecSpec.Services) > 0,
		injectCiliumEnvoyFilters(&cecObjectMeta, cecSpec),
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants