Skip to content

Conversation

@fiunchinho
Copy link
Member

…LB controller, when reconciling worker node sg
@fiunchinho fiunchinho self-assigned this Nov 18, 2025
@fiunchinho fiunchinho moved this to Blocked / Waiting ⛔️ in Roadmap Nov 20, 2025
@fiunchinho fiunchinho changed the title Filter out inbound rules added by external controllers, like the AWS LB controller, when reconciling worker node sg ✨ Filter out inbound rules added by external controllers, like the AWS LB controller, when reconciling worker node sg Nov 20, 2025

// Filter out rules managed by external controllers (e.g., AWS Load Balancer Controller)
// These rules should not be revoked by CAPA as they are managed by other components.
current := filterIgnoredIngressRules(sg.IngressRules)
Copy link

Choose a reason for hiding this comment

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

The variable name now isn't descriptive of its content anymore. Let's please use re-assign with the new meaning, or rename:

Suggested change
current := filterIgnoredIngressRules(sg.IngressRules)
currentFiltered := filterIgnoredIngressRules(sg.IngressRules)

But actually, I think the below comment is better for understanding the function flow...

// Duplicate rules with multiple cidr blocks/source security groups so that we are comparing similar sets.
want := expandIngressRules(specRules)

toRevoke := current.Difference(want)
Copy link

Choose a reason for hiding this comment

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

Instead of the above, I would like the filtering to be done here, since what we want to do is not delete those rules, and this is the code that does exactly that:

Suggested change
// Do not revoke rules managed by external controllers (e.g., AWS Load Balancer Controller).
toRevoke := filterIgnoredIngressRules(current.Difference(want))

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

Labels

None yet

Projects

Status: Blocked / Waiting ⛔️

Development

Successfully merging this pull request may close these issues.

4 participants