-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[RFC] Network policies #3611
[RFC] Network policies #3611
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @BenTheElder |
privileged: true | ||
capabilities: | ||
add: ["NET_ADMIN"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't privileged=true make the NET_ADMIN cap redundant, i think it should be privileged=false and only the required caps should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this is from the original repo manifest;
https://github.com/kubernetes-sigs/kube-network-policies/blob/main/install.yaml#L85-L88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think I started to get granularity and gave up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think, it's fine to leave it privileged and remove the NET_ADMIN cap. maybe leave a TODO comment if someone wishes to experiment with caps and limit the access scope.
@@ -0,0 +1,75 @@ | |||
/* | |||
Copyright 2019 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright 2019 The Kubernetes Authors. | |
Copyright 2024 The Kubernetes Authors. |
// get the target node for this task | ||
controlPlanes, err := nodeutils.ControlPlaneNodes(allNodes) | ||
if err != nil { | ||
return err | ||
} | ||
node := controlPlanes[0] // kind expects at least one always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// get the target node for this task | |
controlPlanes, err := nodeutils.ControlPlaneNodes(allNodes) | |
if err != nil { | |
return err | |
} | |
node := controlPlanes[0] // kind expects at least one always | |
// get the target node for this task | |
node, err := nodeutils.BootstrapControlPlaneNode(allNodes) | |
if err != nil { | |
return err | |
} |
@@ -186,6 +186,8 @@ type Networking struct { | |||
// If DisableDefaultCNI is true, kind will not install the default CNI setup. | |||
// Instead the user should install their own CNI after creating the cluster. | |||
DisableDefaultCNI bool `yaml:"disableDefaultCNI,omitempty" json:"disableDefaultCNI,omitempty"` | |||
// If NetworkPolicies is true, kind will install the default Network Policy setup. | |||
NetworkPolicies bool `yaml:"networkPolicies,omitempty" json:"networkPolicies,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this is a bool i would put a verb in front of it, same as DisasbleDefaultCNI above.
e.g. InstallNetworkPolicies
@neolit123 the alternative is to ship it with kindnet #3612 and no API required |
i think making it not part of kindnet seems better to me. |
Making it part of kindnetd would make it easier for projects that are swapping out kindnet to exclude it, and for projects that aren't swapping out kindet ... I'm not sure why it would be a problem to include it in kindnet? I think that's the simpler approach, and consistent with e.g. ip masquerade just being built in to kindnet. In the future we may also look into if we can ship it in a single image / container by importing the controller or something like that (to share resources & patching better). |
Add support for network policies in KIND
/hold
Fixes: #842