Skip to content

policy: No-op Identity Allocator #35973

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.

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

Merged
merged 1 commit into from
Nov 21, 2024
Merged

policy: No-op Identity Allocator #35973

merged 1 commit into from
Nov 21, 2024

Conversation

dlapcevic
Copy link
Contributor

Ref: #33360

Description
As part of the network policy modularization and decoupling of the network policy enforcement system from the endpoint, the no-op allocator aims to provide a possibility of disabling the identity allocation system when network policies are disabled.

Motivation
The scalability of network policy feature is limited when the scalability dimensions, # of nodes and pod churn rate, are stretched. Cilium has many networking features that scale better, or are unaffected by these scalability dimensions. Currently, disabling network policies to reach higher scale for other features is the only feasible solution.

Note: The plan is to improve scalability of network policies, so that it can reach higher scales that are required.

Follow-up
Subsequently, the plan is to agree on a configuration set that would determine if the identity allocation system should be disabled, and put the no-op allocator to use.

Proposed config:

  • EnablePolicy=never
  • DisableCiliumEndpointCRD=true
  • EnableK8sNetworkPolicy=false
  • EnableCiliumNetworkPolicy=false
  • EnableCiliumClusterwideNetworkPolicy=false

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

@dlapcevic dlapcevic requested review from a team as code owners November 13, 2024 16:52
@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 Nov 13, 2024
@github-actions github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Nov 13, 2024
@dlapcevic dlapcevic added the release-note/misc This PR makes changes that have no direct user impact. label Nov 14, 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 Nov 14, 2024
@dlapcevic dlapcevic force-pushed the np-1 branch 2 times, most recently from 341bd73 to db8efe6 Compare November 15, 2024 11:36
@dlapcevic
Copy link
Contributor Author

/test

@marseel
Copy link
Contributor

marseel commented Nov 18, 2024

Clustermesh failures seems like bug introduced in this PR:

panic: runtime error: invalid memory address or nil pointer dereference
github.com/cilium/cilium/pkg/clustermesh.(*remoteCluster).Status(0xc00026a5a0)
    /go/src/github.com/cilium/cilium/pkg/clustermesh/remote_cluster.go:159 +0xd4

corresponds to:

status.NumIdentities = int64(rc.remoteIdentityCache.NumEntries())

@dlapcevic
Copy link
Contributor Author

Clustermesh failures seems like bug introduced in this PR:

panic: runtime error: invalid memory address or nil pointer dereference
github.com/cilium/cilium/pkg/clustermesh.(*remoteCluster).Status(0xc00026a5a0)
    /go/src/github.com/cilium/cilium/pkg/clustermesh/remote_cluster.go:159 +0xd4

corresponds to:

status.NumIdentities = int64(rc.remoteIdentityCache.NumEntries())

I see. Interesting. I only changed from struct pointer (*RemoteCache) to interface (RemoteIDCache).
@marseel Do you have any ideas why it would affect its behavior?

@marseel
Copy link
Contributor

marseel commented Nov 18, 2024

Marcel Zięba Do you have any ideas why it would affect its behavior?

Yes, you can call method on nil pointer, but not on nil interface. On nil interface it causes panic.

@dlapcevic dlapcevic force-pushed the np-1 branch 2 times, most recently from ce39b1d to edd01a8 Compare November 18, 2024 13:38
Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

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

thanks, lgtm for clustermesh

@dlapcevic
Copy link
Contributor Author

Hi @tklauser, could you please take a look for sig-policy?

cc @squeed

@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 Nov 20, 2024
@tklauser tklauser removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 20, 2024
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

(requesting changes instead of comment, so this doesn't automatically get merged without comments being addressed)

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks! One more comment re. (*Allocator).WatchRemoteKVStore, otherwise LGTM.

@tklauser tklauser enabled auto-merge November 20, 2024 14:31
@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 Nov 21, 2024
**Description**
As part of the network policy modularization and decoupling of the network policy enforcement system from the endpoint, the no-op allocator aims to provide a possibility of disabling the identity allocation system when network policies are disabled.

**Motivation**
The scalability of network policy feature is limited when the scalability dimensions, # of nodes and pod churn rate, are stretched. Cilium has many networking features that scale better, or are unaffected by these scalability dimensions. Currently, disabling network policies to reach higher scale for other features is the only feasible solution.

Note: The plan is to improve scalability of network policies, so that it can reach higher scales that are required.

**Follow-up**
Subsequently, the plan is to agree on a configuration set that would determine if the identity allocation system should be disabled, and put the no-op allocator to use.

Proposed config:
- EnablePolicy=never
- DisableCiliumEndpointCRD=true
- EnableK8sNetworkPolicy=false
- EnableCiliumNetworkPolicy=false
- EnableCiliumClusterwideNetworkPolicy=false

Signed-off-by: Dorde Lapcevic <dordel@google.com>
@cilium cilium deleted a comment from maintainer-s-little-helper bot Nov 21, 2024
@cilium cilium deleted a comment from maintainer-s-little-helper bot Nov 21, 2024
@dlapcevic dlapcevic removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 21, 2024
@dlapcevic
Copy link
Contributor Author

/test

@tklauser tklauser added this pull request to the merge queue Nov 21, 2024
@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 Nov 21, 2024
Merged via the queue into cilium:main with commit a250775 Nov 21, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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
Development

Successfully merging this pull request may close these issues.

3 participants