Skip to content

Fix nodeSelector validation in CiliumNetworkPolicy Specs array#40702

Merged
joestringer merged 1 commit intocilium:mainfrom
pillai-ashwin:cnp-nodeselector-bug
Aug 21, 2025
Merged

Fix nodeSelector validation in CiliumNetworkPolicy Specs array#40702
joestringer merged 1 commit intocilium:mainfrom
pillai-ashwin:cnp-nodeselector-bug

Conversation

@pillai-ashwin
Copy link
Copy Markdown
Member

@pillai-ashwin pillai-ashwin commented Jul 25, 2025

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer's Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fix nodeSelector validation in CiliumNetworkPolicy Specs array

This PR fixes a bug where namespaced CiliumNetworkPolicy objects with nodeSelector specified in the specs array were being silently accepted but ignored, instead of being properly rejected with a validation error.

Problem

The issue was that nodeSelector validation was only applied to the singular spec field but not to individual rules within the specs array. This led to inconsistent behavior:

  • spec: { nodeSelector: {} } → Correctly rejected
  • specs: [{ nodeSelector: {} }] → Silently accepted but ignored

Solution

Added the missing nodeSelector validation check inside the r.Specs loop in the Parse method of CiliumNetworkPolicy. Now both cases consistently reject the policy with the error message: "Invalid CiliumNetworkPolicy spec: rule cannot have NodeSelector"

Testing

  • Added comprehensive test case in TestParseWithNodeSelector to cover the specs array scenario
  • All existing tests pass, ensuring no regressions
  • Verified the exact policy from the issue is now properly rejected

Fixes: #40268

Fix validation bug where namespaced CiliumNetworkPolicies with nodeSelector in specs array were silently accepted but ignored. Now properly rejected with validation error.

@pillai-ashwin pillai-ashwin requested review from a team as code owners July 25, 2025 05:55
@pillai-ashwin pillai-ashwin requested a review from squeed July 25, 2025 05:55
@maintainer-s-little-helper
Copy link
Copy Markdown

Commit 2974519 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 25, 2025
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jul 25, 2025
@pillai-ashwin pillai-ashwin force-pushed the cnp-nodeselector-bug branch from 2974519 to da4d09f Compare July 25, 2025 05:59
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 25, 2025
Copy link
Copy Markdown
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

nice catch!

@squeed
Copy link
Copy Markdown
Contributor

squeed commented Jul 29, 2025

/test

@squeed squeed added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 29, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 29, 2025
@squeed squeed added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 29, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 29, 2025
@squeed squeed added the needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch label Jul 29, 2025
@pillai-ashwin pillai-ashwin force-pushed the cnp-nodeselector-bug branch from da4d09f to f019bcc Compare July 29, 2025 17:09
@maintainer-s-little-helper
Copy link
Copy Markdown

Commit 81831c5 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 30, 2025
@pillai-ashwin pillai-ashwin force-pushed the cnp-nodeselector-bug branch from 81831c5 to a951dd6 Compare July 30, 2025 18:42
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 30, 2025
Signed-off-by: Ashwin Pillai <pillaiashwin96@gmail.com>
@joestringer
Copy link
Copy Markdown
Member

/test

@joestringer joestringer enabled auto-merge August 5, 2025 15:30
@parlakisik
Copy link
Copy Markdown
Contributor

/test

@parlakisik
Copy link
Copy Markdown
Contributor

@pillai-ashwin Can you rebase the cr ? This may help to pass the failed test.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 21, 2025
@joestringer joestringer added this pull request to the merge queue Aug 21, 2025
Merged via the queue into cilium:main with commit a5242c5 Aug 21, 2025
67 of 68 checks passed
@pippolo84 pippolo84 mentioned this pull request Aug 25, 2025
17 tasks
@pippolo84 pippolo84 added backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. and removed needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch labels Aug 25, 2025
@github-actions github-actions bot added backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. and removed backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. labels Sep 1, 2025
@cilium-release-bot cilium-release-bot bot moved this to Released in cilium v1.19.0 Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Projects

No open projects
Status: Released

Development

Successfully merging this pull request may close these issues.

Cilium accepts namespaced CiliumNetworkPolicy with nodeSelector, but doesn't do anything

6 participants