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

Move NetworkPolicy to v1 #39164

Merged
merged 2 commits into from May 28, 2017

Conversation

@danwinship
Copy link
Contributor

danwinship commented Dec 22, 2016

Move NetworkPolicy to v1

@kubernetes/sig-network-misc

Release note:

NetworkPolicy has been moved from `extensions/v1beta1` to the new
`networking.k8s.io/v1` API group. The structure remains unchanged from
the beta1 API.

The `net.beta.kubernetes.io/network-policy` annotation on Namespaces
to opt in to isolation has been removed. Instead, isolation is now
determined at a per-pod level, with pods being isolated if there is
any NetworkPolicy whose spec.podSelector targets them. Pods that are
targeted by NetworkPolicies accept traffic that is accepted by any of
the NetworkPolicies (and nothing else), and pods that are not targeted
by any NetworkPolicy accept all traffic by default.

Action Required:

When upgrading to Kubernetes 1.7 (and a network plugin that supports
the new NetworkPolicy v1 semantics), to ensure full behavioral
compatibility with v1beta1:

    1. In Namespaces that previously had the "DefaultDeny" annotation,
       you can create equivalent v1 semantics by creating a
       NetworkPolicy that matches all pods but does not allow any
       traffic:

           kind: NetworkPolicy
           apiVersion: networking.k8s.io/v1
           metadata:
             name: default-deny
           spec:
             podSelector:

       This will ensure that pods that aren't matched by any other
       NetworkPolicy will continue to be fully-isolated, as they were
       before.

    2. In Namespaces that previously did not have the "DefaultDeny"
       annotation, you should delete any existing NetworkPolicy
       objects. These would have had no effect before, but with v1
       semantics they might cause some traffic to be blocked that you
       didn't intend to be blocked.
@k8s-reviewable

This comment has been minimized.

Copy link

k8s-reviewable commented Dec 22, 2016

This change is Reviewable

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented Jan 9, 2017

Here's an initial attempt at moving NetworkPolicy to v1... this still needs work.

(Just to clarify, that work is blocked on decisions being made about the questions I asked above)

@lavalamp lavalamp removed their assignment Jan 11, 2017

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jan 11, 2017

Sorry for radio silence I was OOO and am still catching up. asking in the api-machinery slack channel is probably your best bet if you have open api machinery questions.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 11, 2017

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented Jan 12, 2017

Before declaring v1 done we should probably land #35660 (e2e NetworkPolicy tests), extend it to cover all features that are going to be part of v1, and make sure existing implementations actually implement all those features. And if most/all of them don't implement a feature, maybe we should consider dropping it for v1.

eg, as far as I can tell, none of Calico, Romana, or Weave implement string-valued Ports (qv #39769)

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented Jan 19, 2017

How does migration between the v1beta1 net.beta.kubernetes.io/network-policy Namespace annotation and the v1 Namespace.NetworkPolicy field work

This message to kubernetes-dev says they're not maintaining any backward compatibility with alpha annotations for scheduling features so maybe we can do the same thing? (Although that's alpha->beta, not beta->v1...)

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Feb 8, 2017

Do the v1beta1 comment changes correctly describe the way that existing NetworkPolicy implementations work?

Yep, I think those comments are accurate now.

What should the unversioned→v1beta1 conversion do with policies containing an "allow no ports" or "allow no peers" rule? Currently it tries to represent that using a nil-vs-[] distinction, but that's obviously wrong since we're not documenting any such distinction any more (and the conversion to/from json won't respect that distinction). Should it return an error instead? (Presumably that would mean v1beta1 clients would get errors if they tried to fetch such policies?)

Well, a "match no ports" or a "match no peers" rule can be represented in the v1beta1 API by just removing that rule altogether, right? That rule is essentially useless I think.

Should NetworkPolicy stay in pkg/apis/extensions? If not, where should it move? (Does the infrastructure support moving APIs between versions? Eg, conversions.)

Not sure (and I don't think I have a very strong opinion).

How does migration between the v1beta1 net.beta.kubernetes.io/network-policy Namespace annotation and the v1 Namespace.NetworkPolicy field work, given that there is not actually a version number change for Namespace?

Can probably respect the annotation if it is present and the Namespace field is not, but prioritize the Namespace field if it is present? I don't think any special migration code is necessary - up to the implementations what they want to do, but I suspect that they'll want to support both for some period of time.

Do we want to simplify NamespaceNetworkPolicy at all? eg, it currently distinguishes between "default network policy" ({ "networkPolicy": nil }) "default ingress network policy (but possibly non-default egress policy)" ({ "networkPolicy": { "ingress": nil } }), and "default ingress isolation policy (but possibly non-default policy for other aspects of ingress)" ({ "networkPolicy": { "ingress": { "isolation": nil } } }). Do we still think NetworkPolicy will eventually describe things other than traffic isolation?

I'd opt for keeping it as is since we've seen no reason for changing it during it's time in beta. I'm really keen to not change things from beta -> GA unless we've seen a compelling reason to do so.

Where do we move docs/proposals/network-policy.md now that it's no longer a "proposal"? Also, presumably we update it to refer only to the v1 API? (Do we need to worry about documenting the v1beta1 Namespace annotation somewhere since it won't show up anywhere in the generated API docs?)

I imagine that can stay under proposals, right? I don't think the intention is that those evolve as the API evolves.

All-in-all, I'm still a little bit uneasy about the empty vs nil change. It feels like extra work for implementors that doesn't buy us anything. I keep asking myself "when did we ever get feedback from a user asking for this distinction? when did it confuse a user of the API? when did it make things harder for implementations?" and the answers always come back "never".

@bboreham

This comment has been minimized.

Copy link
Contributor

bboreham commented Feb 9, 2017

eg, as far as I can tell, none of Calico, Romana, or Weave implement string-valued Ports (qv #39769)

Speaking for Weave, that is indeed not presently implemented. It complicates matters, but does not seem insurmountable.

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Feb 9, 2017

eg, as far as I can tell, none of Calico, Romana, or Weave implement string-valued Ports

Yep, Calico doesn't implement this yet either. It's not impossible but it's not a one-liner to add support.

I've not heard any user demand for this.

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented Feb 10, 2017

Well, a "match no ports" or a "match no peers" rule can be represented in the v1beta1 API by just removing that rule altogether, right? That rule is essentially useless I think.

Ah, indeed

Where do we move docs/proposals/network-policy.md now that it's no longer a "proposal"?

I imagine that can stay under proposals, right? I don't think the intention is that those evolve as the API evolves.

Well, OK, but we need to document NetworkPolicy somewhere that isn't just a "proposal". But I guess that's an easier question: just add the new docs wherever they fit in best.

All-in-all, I'm still a little bit uneasy about the empty vs nil change. It feels like extra work for implementors that doesn't buy us anything. I keep asking myself "when did we ever get feedback from a user asking for this distinction? when did it confuse a user of the API? when did it make things harder for implementations?" and the answers always come back "never".

Yeah, I don't think anyone actually wanted the distinction, it's just that the way that v1beta1 ended up being implemented (empty Ports means "all ports", empty From means "from anywhere") makes us inconsistent with other APIs (and in particular, it makes Ports/From inconsistent with PodSelector/NamespaceSelector, meaning NetworkPolicy isn't even internally self-consistent).

But yeah, the "fix" is ugly. I'd be fine sticking with the v1beta1 behavior. (But there were objections to that when it was discussed in the SIG.)

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Feb 27, 2017

Yeah, I don't think anyone actually wanted the distinction, it's just that the way that v1beta1 ended up being implemented (empty Ports means "all ports", empty From means "from anywhere") makes us inconsistent with other APIs (and in particular, it makes Ports/From inconsistent with PodSelector/NamespaceSelector, meaning NetworkPolicy isn't even internally self-consistent).

I noticed while poking around the ClusterRole specification that it seems there are other APIs using an empty slice to indicate "all" rather than "none".

// ResourceNames is an optional white list of names that the rule applies to.  An empty set means that everything is allowed.
ResourceNames []string

@danwinship danwinship force-pushed the danwinship:networkpolicy-v1 branch from 214b51c to 0923f86 May 28, 2017

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 28, 2017

@danwinship: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-cross 0923f86 link @k8s-bot pull-kubernetes-cross test this

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.

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented May 28, 2017

@thockin: fixed defaults, rebased, regenerated

@thockin

This comment has been minimized.

Copy link
Member

thockin commented May 28, 2017

@k8s-ci-robot k8s-ci-robot added the lgtm label May 28, 2017

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented May 28, 2017

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented May 28, 2017

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit 382a170 into kubernetes:master May 28, 2017

9 of 11 checks passed

pull-kubernetes-cross Jenkins job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce-etcd3
Details
cla/linuxfoundation danwinship authorized
Details
pull-kubernetes-bazel Job succeeded.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-federation-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details

@danwinship danwinship deleted the danwinship:networkpolicy-v1 branch May 29, 2017

@enj

This comment has been minimized.

Copy link
Member

enj commented May 29, 2017

If we delete the extensions.NetworkPolicy - who would register the v1beta1 endpoint? Ugh, I don't understand the apiserver anymore....

@thockin you could register the types from the other package. For example, pkg/apis/apps registers the pkg/apis/extensions types:

// Adds the list of known types to api.Scheme.
func addKnownTypes(scheme *runtime.Scheme) error {
	// TODO this will get cleaned up with the scheme types are fixed
	scheme.AddKnownTypes(SchemeGroupVersion,
		&extensions.Deployment{},
		&extensions.DeploymentList{},
		&extensions.DeploymentRollback{},
		&extensions.Scale{},
		&StatefulSet{},
		&StatefulSetList{},
	)
	return nil
}
@thockin

This comment has been minimized.

Copy link
Member

thockin commented May 30, 2017

@thockin

This comment has been minimized.

Copy link
Member

thockin commented May 30, 2017

@thockin

This comment has been minimized.

Copy link
Member

thockin commented May 30, 2017

@gyliu513

This comment has been minimized.

Copy link
Member

gyliu513 commented Jul 18, 2017

One question here is if I upgrade NetworkPolicy to v1, do I also need to upgrade the calico? Currently, I was using calico node image as 1.2.1 and calico policy controller image as 0.6.0. @danwinship

@gyliu513

This comment has been minimized.

Copy link
Member

gyliu513 commented Jul 24, 2017

I filed an issue for calico projectcalico/kube-controllers#108 , the NetworkPolicy of net.beta.kubernetes.io/network-policy still works in Kubernetes 1.7, but the CHANGELOG mentioned this has been removed in 1.7, is it a bug? @danwinship


NetworkPolicy has been promoted from extensions/v1beta1 to the new networking.k8s.io/v1 API group. The structure remains unchanged from the v1beta1 API. The net.beta.kubernetes.io/network-policy annotation on Namespaces (used to opt in to isolation) has been removed.

@caseydavenport caseydavenport referenced this pull request Jul 31, 2017

Merged

Release 2.4.0 #981

0 of 3 tasks complete

@mumoshu mumoshu referenced this pull request Aug 20, 2017

Merged

Update Calico to v2.4.1 #832

liggitt added a commit to liggitt/kubernetes that referenced this pull request Jan 25, 2018

Stop serving deprecated extensions/v1beta1 APIs by default
From the 1.7 release notes:
NetworkPolicy has been moved from extensions/v1beta1 to the new (kubernetes#39164, @danwinship) networking.k8s.io/v1 API group.

From the 1.8 release notes:
The Deployment, DaemonSet, and ReplicaSet kinds in the extensions/v1beta1 group version are now deprecated, as are the Deployment, StatefulSet, and ControllerRevision kinds in apps/v1beta1. As they will not be removed until after a GA version becomes available, you may continue to use these kinds in existing code. However, all new code should be developed against the apps/v1beta2 group version

This PR disables these extensions/v1beta1 resources from being served by default in 1.10, though they can be re-enabled via --runtime-config.

Objects already stored in etcd in extensions/v1beta1 format can still be read
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment