Skip to content

policy: Add config for enabling Cilium NetworkPolicy #35049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2024
Merged

policy: Add config for enabling Cilium NetworkPolicy #35049

merged 1 commit into from
Oct 14, 2024

Conversation

dlapcevic
Copy link
Contributor

Related to modularizing network policies: #33360

Signed-off-by: Dorde Lapcevic <dordel@google.com>

@dlapcevic dlapcevic requested review from a team as code owners September 26, 2024 08:54
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 26, 2024
@github-actions github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Sep 26, 2024
@dlapcevic dlapcevic force-pushed the cilium-np branch 2 times, most recently from cab057e to 00d578b Compare September 30, 2024 10:13
@dlapcevic dlapcevic added the release-note/misc This PR makes changes that have no direct user impact. label Sep 30, 2024
@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 Sep 30, 2024
sypakine
sypakine reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@sypakine sypakine left a comment

Choose a reason for hiding this comment

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

  • Should CCNP have separate enablement?
  • Should this short-circuit validation? operator/pkg/networkpolicy/cell.go
  • Should this disable the operator watchers? operator/cmd/root.go

@dlapcevic dlapcevic force-pushed the cilium-np branch 2 times, most recently from 7f4528e to 79fc6cf Compare October 1, 2024 16:03
@dlapcevic dlapcevic requested a review from a team as a code owner October 1, 2024 16:03
@dlapcevic dlapcevic requested a review from joamaki October 1, 2024 16:03
@dlapcevic
Copy link
Contributor Author

dlapcevic commented Oct 1, 2024 •
edited
Loading

Great questions @sypakine!

  • Should CCNP have separate enablement?

It would be good to differentiate CNP and CCNP. I propose to do it as a follow up. For now one flag will stand for both CNP and CCNP.

  • Should this short-circuit validation? operator/pkg/networkpolicy/cell.go

Yes, it should. I added it, and also for cilium-operator to read the same flag.

  • Should this disable the operator watchers? operator/cmd/root.go

Yes, it should. I added it. It reuses the same cilium-operator from the previous point.

Watchers in cilium-operator need to be migrated to resource.Resource[], which will make it easier to ensure that there aren't multiple watchers (informers) instantiated for the same resource, and that they don't run when no feature uses them. However, from the scalability perspective, this is negligible, since cilium-operator is running in one pod (leader from a deployment), while it would be problematic if it was a daemonset like cilium-agent.

@dlapcevic dlapcevic requested a review from sypakine October 4, 2024 10:43
sypakine
sypakine reviewed Oct 4, 2024
View reviewed changes
sypakine
sypakine approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@sypakine sypakine left a comment

Choose a reason for hiding this comment

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

LGTM with nit about the flag/comment lacking clarity that this applies to both CNP and CCNP.

@dlapcevic dlapcevic force-pushed the cilium-np branch 3 times, most recently from 159f04c to 4307577 Compare October 4, 2024 14:58
ovidiutirla
ovidiutirla approved these changes Oct 5, 2024
View reviewed changes
Copy link
Contributor

@ovidiutirla ovidiutirla left a comment

Choose a reason for hiding this comment

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

🚀

@dlapcevic
policy: Add config for enabling Cilium NetworkPolicy
fc06685 
Signed-off-by: Dorde Lapcevic <dordel@google.com>
@dlapcevic
Copy link
Contributor Author

/test

@pchaigno pchaigno enabled auto-merge October 7, 2024 12:28
@doniacld doniacld requested a review from squeed October 14, 2024 12:56
@doniacld
Copy link
Contributor

Adding @squeed to reviewers to double check for sig-policy group.

@pchaigno pchaigno added this pull request to the merge queue Oct 14, 2024
Merged via the queue into cilium:main with commit 253a05b Oct 14, 2024
65 checks passed
@dlapcevic dlapcevic mentioned this pull request Oct 15, 2024
Merged
@pippolo84 pippolo84 mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@asauber asauber asauber approved these changes

@ovidiutirla ovidiutirla ovidiutirla approved these changes

@youngnick youngnick Awaiting requested review from youngnick youngnick is a code owner automatically assigned from cilium/sig-k8s

@squeed squeed Awaiting requested review from squeed

+2 more reviewers

@sypakine sypakine sypakine approved these changes

@doniacld doniacld doniacld approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dlapcevic @doniacld @joamaki @asauber @sypakine @ovidiutirla @pchaigno