Add IPBlock to Network Policy #50033

cmluciano · 2017-08-02T18:34:50Z

What this PR does / why we need it:
Add ipBlockRule to NetworkPolicyPeer.

Which issue this PR fixes
fixes #49978

Special notes for your reviewer:

I added this directly as a field on the existing API per guidance from API-Machinery/lazy SIG-Network consensus.

Todo:

Documentation comments to mention this is beta, unless we want to go straight to GA
e2e tests

Release note:

Support ipBlock in NetworkPolicy

cmluciano · 2017-08-09T18:37:17Z

I'm not sure if I generated the code improperly. The build failures are a bit cryptic to me.

cmluciano · 2017-08-11T21:00:21Z

@kubernetes/sig-api-machinery-misc Can anyone help diagnose what I'm failing to run/add here?

thockin · 2017-08-11T22:39:25Z

What is the use case for the selector semantic here, vs simply "from cidr". Can you show concretely how this is to be used that can't be solved by simpler? This backdoor-introduces "deny" logic, and I worry that this turns into a firewall.

cmluciano · 2017-08-11T22:48:23Z

This was based on conversations in #49978 (comment) about how to implement the deny-type of wording. I went with this since it matched current styles for podSelectors.

cc @danwinship @caseydavenport

caseydavenport · 2017-08-11T22:49:57Z

@thockin I expressed similar concern. The reasoning is documented in this discussion: #49978 (comment)

The TL;DR is that some users want to allow traffic to/from a set of IPs that doesn't cleanly align with a single CIDR.

e.g. "allow from everywhere in 192.168.0.0/16 except for 192.168.8.0/24"

To reiterate my stance from the other thread:

I'd much much prefer a simple CIDR field
This is a bit more flexible (also more confusing), and it's easy for us to implement at least so I won't fight it tooth-and-nail.

thockin · 2017-08-11T22:53:06Z

I'll go read the context (sorry). I find this to be pretty obtuse, myself.

…

On Fri, Aug 11, 2017 at 3:50 PM, Casey Davenport ***@***.***> wrote: @thockin <https://github.com/thockin> I expressed similar concern. The reasoning is documented in this discussion: #49978 (comment) <#49978 (comment)> The TL;DR is that some users want to allow traffic to/from a set of IPs that doesn't cleanly align with a single CIDR. e.g. "allow from everywhere in 192.168.0.0/16 except for 192.168.8.0/24" To reiterate my stance from the other thread: - I'd much much prefer a simple CIDR field - This is a bit more flexible (also more confusing), and it's easy for us to implement at least so I won't fight it tooth-and-nail. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50033 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVP-aZrrSFO2eamyz9rGZVHo0T4lzks5sXNqqgaJpZM4OrfNl> .

thockin · 2017-08-11T22:57:05Z

Will comment there.

…

On Fri, Aug 11, 2017 at 3:52 PM, Tim Hockin ***@***.***> wrote: I'll go read the context (sorry). I find this to be pretty obtuse, myself. On Fri, Aug 11, 2017 at 3:50 PM, Casey Davenport ***@***.*** > wrote: > @thockin <https://github.com/thockin> I expressed similar concern. The > reasoning is documented in this discussion: #49978 (comment) > <#49978 (comment)> > > The TL;DR is that some users want to allow traffic to/from a set of IPs > that doesn't cleanly align with a single CIDR. > > e.g. "allow from everywhere in 192.168.0.0/16 except for 192.168.8.0/24" > > To reiterate my stance from the other thread: > > - I'd much much prefer a simple CIDR field > - This is a bit more flexible (also more confusing), and it's easy > for us to implement at least so I won't fight it tooth-and-nail. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#50033 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AFVgVP-aZrrSFO2eamyz9rGZVHo0T4lzks5sXNqqgaJpZM4OrfNl> > . >

cmluciano · 2017-08-16T20:31:50Z

I think my test failures are caused by the v1beta code not converting or dropping the new ipBlock field.

@thockin @sttts Does the conversion code under pkg/apis/extensions/conversion.go persist for one more release?

thockin · 2017-08-17T04:56:51Z

We have to keep v1b1 for another release.

…

On Wed, Aug 16, 2017 at 1:32 PM, cmluciano ***@***.***> wrote: I think my test failures are caused by the v1beta code not converting or dropping the new ipBlock field. @thockin <https://github.com/thockin> @sttts <https://github.com/sttts> Does the conversion code under pkg/apis/extensions/conversion.go <https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/v1beta1/conversion.go> persist for one more release? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50033 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVKER1t8WOHu4uRTQWxXFbBbkHC51ks5sY1HdgaJpZM4OrfNl> .

thockin · 2017-08-17T04:57:01Z

Ping me when ready for review.

…

On Wed, Aug 16, 2017 at 9:56 PM, Tim Hockin ***@***.***> wrote: We have to keep v1b1 for another release. On Wed, Aug 16, 2017 at 1:32 PM, cmluciano ***@***.***> wrote: > I think my test failures are caused by the v1beta code not converting or > dropping the new ipBlock field. > > @thockin <https://github.com/thockin> @sttts <https://github.com/sttts> > Does the conversion code under pkg/apis/extensions/conversion.go > <https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/v1beta1/conversion.go> > persist for one more release? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#50033 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AFVgVKER1t8WOHu4uRTQWxXFbBbkHC51ks5sY1HdgaJpZM4OrfNl> > . >

danwinship · 2017-08-18T18:18:15Z

pkg/apis/networking/types.go

+type CIDR string
+
+// Except is a slice of CIDRs to not include within an IpBlockRule
+type Except []string


You're not using either the CIDR type or the Except type. (IpBlockRule just uses string and []string directly. And I feel like that's correct.)

danwinship · 2017-08-18T18:21:26Z

pkg/apis/networking/types.go

+// IpBlockRule describes a particular CIDR (Ex. "192.168.1.1/24") that is allowed to the pods
+// matched by a NetworkPolicySpec's podSelector. The except entry describes CIDRs that should
+// not be included within this rule.
+type IpBlockRule struct {


Capital P. IPBlockRule. Acronyms are always either all-caps or all-lowercase. (So IPBlockRule here and ipBlockRule in the JSON.)

danwinship · 2017-08-18T18:24:01Z

pkg/apis/networking/types.go

+// matched by a NetworkPolicySpec's podSelector. The except entry describes CIDRs that should
+// not be included within this rule.
+type IpBlockRule struct {
+	// CIDR is a string representing a Classless inter-domain route


We don't explain what "CIDR" stands for anywhere else, so I'd just say "is a string representing the IP block". (If you do want to expand the acronym, be consistent with the capitalization.)

danwinship · 2017-08-18T18:25:29Z

pkg/apis/networking/types.go

+	// CIDR is a string representing a Classless inter-domain route
+	// Valid examples are "192.168.1.1/24"
+	CIDR string
+	// Except is a slice of CIDRs that should not be included within an IPBlock rule


There's inconsistency throughout about "IPBlockRule" vs "IPBlock"

You're right. I have no idea why I came up with rule and it becomes terribly confusing. I switched the the networking.IPBlock to represent the type for a IPBlockSelector.

Should we continue to use something like ipBlock in the json or ipBlockSelector?

danwinship · 2017-08-18T18:26:28Z

pkg/apis/networking/types.go

+
+	// IpBlockrule defines policy on a particular CIDR
+	// +optional
+	IpBlockRule *IpBlockRule


maybe call it IPBlockSelector or CIDRSelector for consistency with the other fields?

danwinship · 2017-08-18T18:27:27Z

pkg/apis/networking/validation/validation.go

+func ValidateIpBlock(br *networking.IpBlockRule, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if len(br.CIDR) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "ibBlock rules must specify a CIDR"))


beyond the continued inconsistency about "ip block" vs "ip block rule", there's also a typo here ("ib")

danwinship · 2017-08-18T18:28:04Z

pkg/apis/networking/validation/validation.go

+	for i, exceptIP := range br.Except {
+		exceptPath := fldPath.Child("except").Index(i)
+		if exceptIP == "" || exceptIP == " " {
+			allErrs = append(allErrs, field.Invalid(exceptPath.Child("except"), exceptIP, "value cannot be an empty string"))


should call IsValidCIDR

danwinship · 2017-08-18T18:29:51Z

staging/src/k8s.io/api/networking/v1/types.go

+	// Except is a slice of CIDRs that should not be included within an IPBlock rule
+	// Valid examples are "192.168.1.1/24"
+	// +optional
+	Except []string `json:"except,omitempty" protobuf:"bytes,2,opt,name=except"`


rep not opt in the protobuf spec (because it's an array)

cmluciano · 2017-08-22T11:44:47Z

/retest

cmluciano · 2017-08-22T12:44:16Z

@thockin Any ideas on why roundtrips fail only sometimes for me? I thought my fuzzer would help with this and I see the errors irregularly. I'm not sure if this is intentional or if my fuzzer/assumptions about compatibility are wrong.

cmluciano · 2017-08-22T15:16:30Z

Talked with Stefan a bit about this. Since we still have v1beta1 I need to encode the new field in an annotation. Also we are still using v1beta1 as the default storage backend. #50604 is open for this.

thockin · 2017-08-22T21:22:45Z

I'll review now. What is teh story with test fail? Do we understand it? ETA for fix?

thockin

small changes, pls. Otherwise LGTM

thockin · 2017-08-22T21:27:31Z

pkg/apis/networking/types.go

+	// CIDR is a string representing the IP Block
+	// Valid examples are "192.168.1.1/24"
+	CIDR string
+	// Except is a slice of CIDRs that should not be included within an IP Block


Comment (and validate) that the CIDRs in except must be subsets of cidr

thockin · 2017-08-22T21:28:47Z

pkg/apis/networking/types.go

+
+	// IPBlockSelector defines policy on a particular IPBlock
+	// +optional
+	IPBlockSelector *IPBlock


This should be IPBlock. It is not a Selector.

thockin · 2017-08-22T21:31:03Z

pkg/apis/networking/validation/validation.go

+func ValidateIPBlock(br *networking.IPBlock, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if len(br.CIDR) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "ipBlock must specify a CIDR"))


field.Required(fldPath.Child("cidr"), "") - the last error string is redundant here.

thockin · 2017-08-22T21:31:58Z

pkg/apis/networking/validation/validation.go

+		allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), "ipBlock must specify a CIDR"))
+	}
+	if !IsValidCIDR(br.CIDR) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("cidr"), br.CIDR, "value is not a valid CIDR"))


"not a valid CIDR"

thockin · 2017-08-22T21:32:22Z

pkg/apis/networking/validation/validation.go

+	for i, exceptIP := range br.Except {
+		exceptPath := fldPath.Child("except").Index(i)
+		if !IsValidCIDR(exceptIP) {
+			allErrs = append(allErrs, field.Invalid(exceptPath.Child("except"), exceptIP, "value cannot be an empty string"))


"not a valid CIDR"

validate that it is "in" the main CIDR

thockin · 2017-08-22T21:33:59Z

pkg/apis/networking/validation/validation.go

+}
+
+// IsValidCIDR validates whether a CIDR matches the conventions expected by net.ParseCIDR
+func IsValidCIDR(cidr string) bool {


does this need to be exposed or could it be isValidCIDR ?

thockin · 2017-08-24T05:05:04Z

ping me on slack when ready. I want to turn around PRs as fast as I can...

cmluciano · 2017-08-24T19:17:04Z

@thockin @caseydavenport @danwinship Ready for review

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

thockin · 2017-08-24T20:59:50Z

/lgtm
/approve
/retest

k8s-github-robot · 2017-08-24T21:00:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cmluciano, thockin

Associated issue: 49978

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [thockin]
~~docs/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

sttts · 2017-08-25T07:37:50Z

staging/src/k8s.io/api/networking/v1/types.go

+// matched by a NetworkPolicySpec's podSelector. The except entry describes CIDRs that should
+// not be included within this rule.
+type IPBlock struct {
+	// CIDR is a string representing the IP Block


Don't we usually avoid putting the field name into the description? For some doc generation reason I believe.

k8s-github-robot · 2017-08-25T17:10:40Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-25T18:07:07Z

Automatic merge from submit-queue (batch tested with PRs 50033, 49988, 51132, 49674, 51207)

k8s-ci-robot · 2017-08-25T18:29:38Z

@cmluciano: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-etcd3	`02735c3`	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-e2e-kops-aws	`02735c3`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

cmluciano added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Aug 2, 2017

cmluciano added this to the v1.8 milestone Aug 2, 2017

cmluciano assigned danwinship, caseydavenport and thockin Aug 2, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2017

cmluciano force-pushed the cml/addnpcidrselector branch from 7c5094e to 7ad35fb Compare August 9, 2017 17:57

cmluciano force-pushed the cml/addnpcidrselector branch from 7ad35fb to f3b9bc4 Compare August 11, 2017 17:54

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 11, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 11, 2017

cmluciano force-pushed the cml/addnpcidrselector branch from f3b9bc4 to 66d2516 Compare August 15, 2017 18:21

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 15, 2017

cmluciano force-pushed the cml/addnpcidrselector branch from 66d2516 to f1fe142 Compare August 15, 2017 19:57

cmluciano force-pushed the cml/addnpcidrselector branch from f1fe142 to cbbd2e6 Compare August 18, 2017 02:17

danwinship reviewed Aug 18, 2017

View reviewed changes

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 21, 2017

cmluciano force-pushed the cml/addnpcidrselector branch from 800f481 to 107c56a Compare August 22, 2017 01:53

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 22, 2017

thockin reviewed Aug 22, 2017

View reviewed changes

cmluciano force-pushed the cml/addnpcidrselector branch from 107c56a to a9528b6 Compare August 22, 2017 22:08

cmluciano force-pushed the cml/addnpcidrselector branch from a9528b6 to f89fd5a Compare August 24, 2017 19:10

cmluciano changed the title ~~WIP: Add CIDRSelector to Network Policy~~ Add IPBlock to Network Policy Aug 24, 2017

Christopher M. Luciano added 3 commits August 24, 2017 16:20

Add IPBlock to NetworkPolicy

b75dc93

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

Add networking fuzzer

54a058f

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

IPBlock generated code

02735c3

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

cmluciano force-pushed the cml/addnpcidrselector branch from f89fd5a to 02735c3 Compare August 24, 2017 20:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2017

sttts reviewed Aug 25, 2017

View reviewed changes

k8s-github-robot merged commit c04e516 into kubernetes:master Aug 25, 2017

cmluciano mentioned this pull request Aug 25, 2017

ipBlock for Network Policies kubernetes/enhancements#367

Closed

cmluciano mentioned this pull request Sep 11, 2017

Document ipBlock option in NetworkPolicy kubernetes/website#5392

Closed

Add IPBlock to Network Policy #50033

Add IPBlock to Network Policy #50033

Conversation

cmluciano commented Aug 2, 2017 • edited Loading

cmluciano commented Aug 9, 2017

cmluciano commented Aug 11, 2017

thockin commented Aug 11, 2017

cmluciano commented Aug 11, 2017

caseydavenport commented Aug 11, 2017

thockin commented Aug 11, 2017 via email

thockin commented Aug 11, 2017 via email

cmluciano commented Aug 16, 2017

thockin commented Aug 17, 2017 via email

thockin commented Aug 17, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmluciano commented Aug 22, 2017

cmluciano commented Aug 22, 2017

cmluciano commented Aug 22, 2017

thockin commented Aug 22, 2017

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 24, 2017

cmluciano commented Aug 24, 2017

thockin commented Aug 24, 2017

k8s-github-robot commented Aug 24, 2017

Choose a reason for hiding this comment

k8s-github-robot commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

k8s-ci-robot commented Aug 25, 2017

cmluciano commented Aug 2, 2017 •

edited

Loading