Skip to content
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

Add EndPort to Network Policy - Alpha #97058

Merged
merged 9 commits into from Feb 2, 2021
15 changes: 13 additions & 2 deletions pkg/apis/networking/types.go
Expand Up @@ -138,10 +138,21 @@ type NetworkPolicyPort struct {
// +optional
Protocol *api.Protocol

// The port on the given protocol. This can either be a numerical or named port on
// a pod. If this field is not provided, this matches all port names and numbers.
// The port on the given protocol. This can either be a numerical or named
// port on a pod. If this field is not provided, this matches all port names and
// numbers.
// If present, only traffic on the specified protocol AND port will be matched.
// +optional
Port *intstr.IntOrString

// If set, indicates that the range of ports from port to endPort, inclusive,
// should be allowed by the policy. This field cannot be defined if the port field
// is not defined or if the port field is defined as a named (string) port.
// The endPort must be equal or greater than port.
// This feature is in Alpha state and should be enabled using the Feature Gate
// "NetworkPolicyEndPort".
// +optional
EndPort *int32
rikatz marked this conversation as resolved.
Show resolved Hide resolved
}

// IPBlock describes a particular CIDR (Ex. "192.168.1.1/24","2001:db9::/64") that is allowed
Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/networking/validation/validation.go
Expand Up @@ -68,11 +68,21 @@ func ValidateNetworkPolicyPort(port *networking.NetworkPolicyPort, portPath *fie
for _, msg := range validation.IsValidPortNum(int(port.Port.IntVal)) {
allErrs = append(allErrs, field.Invalid(portPath.Child("port"), port.Port.IntVal, msg))
}
if port.EndPort != nil && *port.EndPort < port.Port.IntVal {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if port.EndPort != nil && *port.EndPort < port.Port.IntVal {
if port.EndPort != nil && *port.EndPort < port.Port.IntVal || *port.EndPort > 65535 {

Copy link
Contributor

Choose a reason for hiding this comment

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

rather use validation.IsValidPortNum ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can make this a follow up PR? For me no problem at all ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpanato will move with the follow up :D

allErrs = append(allErrs, field.Invalid(portPath.Child("endPort"), port.Port.IntVal, "must be greater than or equal to `port`"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
allErrs = append(allErrs, field.Invalid(portPath.Child("endPort"), port.Port.IntVal, "must be greater than or equal to `port`"))
allErrs = append(allErrs, field.Invalid(portPath.Child("endPort"), port.Port.IntVal, "must be greater than or equal to `port`, not higher then 65535"))

}
} else {
if port.EndPort != nil {
rikatz marked this conversation as resolved.
Show resolved Hide resolved
allErrs = append(allErrs, field.Invalid(portPath.Child("endPort"), *port.EndPort, "may not be specified when `port` is non-numeric"))
}
for _, msg := range validation.IsValidPortName(port.Port.StrVal) {
allErrs = append(allErrs, field.Invalid(portPath.Child("port"), port.Port.StrVal, msg))
}
}
} else {
if port.EndPort != nil {
allErrs = append(allErrs, field.Invalid(portPath.Child("endPort"), *port.EndPort, "may not be specified when `port` is not specified"))
}
}

return allErrs
Expand Down
222 changes: 221 additions & 1 deletion pkg/apis/networking/validation/validation_test.go
Expand Up @@ -38,7 +38,7 @@ func TestValidateNetworkPolicy(t *testing.T) {
protocolUDP := api.ProtocolUDP
protocolICMP := api.Protocol("ICMP")
protocolSCTP := api.ProtocolSCTP

endPort := int32(32768)
successCases := []networking.NetworkPolicy{
{
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Expand Down Expand Up @@ -377,6 +377,78 @@ func TestValidateNetworkPolicy(t *testing.T) {
PolicyTypes: []networking.PolicyType{networking.PolicyTypeIngress, networking.PolicyTypeEgress},
},
},
{
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
Ports: []networking.NetworkPolicyPort{
{
Protocol: nil,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 32000},
EndPort: &endPort,
},
{
Protocol: &protocolUDP,
Port: &intstr.IntOrString{Type: intstr.String, StrVal: "dns"},
},
},
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
To: []networking.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"c": "d"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: nil,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 30000},
EndPort: &endPort,
},
{
Protocol: nil,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 32000},
EndPort: &endPort,
},
},
},
},
Ingress: []networking.NetworkPolicyIngressRule{
Copy link
Member

Choose a reason for hiding this comment

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

also need one test for 65536?

{
From: []networking.NetworkPolicyPeer{
{
PodSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"e": "f"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: &protocolTCP,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 32768},
EndPort: &endPort,
},
},
},
},
},
},
}

// Success cases are expected to pass validation.
Expand Down Expand Up @@ -798,6 +870,154 @@ func TestValidateNetworkPolicy(t *testing.T) {
PolicyTypes: []networking.PolicyType{"foo", "bar", "baz"},
},
},
"multiple ports defined, one port range is invalid": {
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
To: []networking.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"c": "d"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: &protocolUDP,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 35000},
EndPort: &endPort,
},
{
Protocol: nil,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 32000},
EndPort: &endPort,
},
},
},
},
},
},
"endPort defined with named/string port": {
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
To: []networking.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"c": "d"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: &protocolUDP,
Port: &intstr.IntOrString{Type: intstr.String, StrVal: "dns"},
EndPort: &endPort,
},
{
Protocol: nil,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 32000},
EndPort: &endPort,
},
},
},
},
},
},
"endPort defined without port defined": {
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
To: []networking.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"c": "d"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: &protocolTCP,
EndPort: &endPort,
},
},
},
},
},
},
"port is greater than endPort": {
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
To: []networking.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"c": "d"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: &protocolSCTP,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 33000},
EndPort: &endPort,
},
},
},
},
},
},
"multiple invalid port ranges defined": {
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: networking.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{"a": "b"},
},
Egress: []networking.NetworkPolicyEgressRule{
{
To: []networking.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"c": "d"},
},
},
},
Ports: []networking.NetworkPolicyPort{
{
Protocol: &protocolUDP,
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 35000},
EndPort: &endPort,
},
{
Protocol: &protocolTCP,
EndPort: &endPort,
},
{
Protocol: &protocolTCP,
Port: &intstr.IntOrString{Type: intstr.String, StrVal: "https"},
EndPort: &endPort,
},
},
},
},
},
},
}

// Error cases are not expected to pass validation.
Expand Down
7 changes: 7 additions & 0 deletions pkg/features/kube_features.go
Expand Up @@ -297,6 +297,12 @@ const (
// Enables SCTP as new protocol for Service ports, NetworkPolicy, and ContainerPort in Pod/Containers definition
SCTPSupport featuregate.Feature = "SCTPSupport"

// owner: @rikatz
// alpha: v1.21
//
// Enables the endPort field in NetworkPolicy to enable a Port Range behavior in Network Policies.
NetworkPolicyEndPort featuregate.Feature = "NetworkPolicyEndPort"

// owner: @xing-yang
// alpha: v1.12
// beta: v1.17
Expand Down Expand Up @@ -728,6 +734,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
RuntimeClass: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23
NodeLease: {Default: true, PreRelease: featuregate.GA, LockToDefault: true},
SCTPSupport: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.22
NetworkPolicyEndPort: {Default: false, PreRelease: featuregate.Alpha},
VolumeSnapshotDataSource: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.21
ProcMountType: {Default: false, PreRelease: featuregate.Alpha},
TTLAfterFinished: {Default: false, PreRelease: featuregate.Alpha},
Expand Down
18 changes: 18 additions & 0 deletions pkg/registry/networking/networkpolicy/BUILD
Expand Up @@ -3,6 +3,7 @@ package(default_visibility = ["//visibility:public"])
load(
"@io_bazel_rules_go//go:def.bzl",
"go_library",
"go_test",
)

go_library(
Expand All @@ -16,9 +17,11 @@ go_library(
"//pkg/api/legacyscheme:go_default_library",
"//pkg/apis/networking:go_default_library",
"//pkg/apis/networking/validation:go_default_library",
"//pkg/features:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)

Expand All @@ -37,3 +40,18 @@ filegroup(
],
tags = ["automanaged"],
)

go_test(
name = "go_default_test",
srcs = ["strategy_test.go"],
embed = [":go_default_library"],
deps = [
"//pkg/apis/core:go_default_library",
"//pkg/apis/networking:go_default_library",
"//pkg/features:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
],
)