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

Allow including both podSelector and namespaceSelector in a NetworkPolicyPeer #60452

Merged
merged 3 commits into from Mar 24, 2018

Conversation

@danwinship
Copy link
Contributor

danwinship commented Feb 26, 2018

Updates NetworkPolicy API docs and validation to allow podSelector and namespaceSelector to be specified together in a NetworkPolicyPeer

Fixes #58637

Release note:

NetworkPolicies can now target specific pods in other namespaces by including both a namespaceSelector and a podSelector in the same peer element.
@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Feb 26, 2018

/sig network

Is there anything else I missed in kubernetes itself? We'll also need a docs PR and a kubectl PR...

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Feb 26, 2018

Having actually written the docs now, I'm maybe more convinceable that we should go with podAndNamespaceSelector instead; it would work more cleanly with the idea of separately documenting each field in NetworkPolicyPeer like we do...

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Feb 26, 2018

Having actually written the docs now, I'm maybe more convinceable that we should go with podAndNamespaceSelector instead; it would work more cleanly with the idea of separately documenting each field in NetworkPolicyPeer like we do...

I'd prefer to not introduce a new field such as podAndNamespaceSelector

As a user of the API, I think the AND of podSelector and namespaceSelector is exactly how I'd expect those two to behave when both specified. In fact, even a generic AND would make sense to me, though I don't see what use-case I'd have for it.

As an implementor, it's almost no work on my side to simply support the AND combination of pod / namespace, whereas there's more for me to do to support a new selector field.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Feb 26, 2018

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Feb 26, 2018

My concern is that the AND isn't generic (unless we make it so) and is
therefore vastly more confusing.

That may be, though with a well written out validation error I'd guess it would be fairly clear.

I understand it might be extra work for other implementors to do the generic AND, but from a Calico perspective it's trivial. So personally I'd rather have us implement the generic AND than add an extra type for people to reason with.

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Feb 26, 2018

That may be, though with a well written out validation error I'd guess it would be fairly clear.

"may not specify both ipBlock and another peer"

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Feb 27, 2018

@Lion-Wei

This comment has been minimized.

Copy link
Contributor

Lion-Wei commented Feb 27, 2018

I like this, is this anything I can help? Maybe the kubectl pr? @danwinship

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Feb 27, 2018

@Lion-Wei: sure. "kubectl describe" needs to be extended to be able to describe policies where a NetworkPolicyPeer has both podSelector and namespaceSelector set (describing it according to the semantics seen in the API docs here).

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Feb 27, 2018

/retest

@caseydavenport
Copy link
Member

caseydavenport left a comment

This LGTM - one minor comment.

I'm in favor of merging as-is, and further relaxing the validation if we get feedback that it's confusing.

@@ -47,7 +47,7 @@ if ! ${ALL} ; then
echo "Running in short-circuit mode; run with FORCE_ALL=true to force all scripts to run."
fi

"${KUBE_ROOT}/hack/godep-restore.sh" ${V}
#"${KUBE_ROOT}/hack/godep-restore.sh" ${V}

This comment has been minimized.

@caseydavenport

caseydavenport Feb 28, 2018

Member

Need to add this back?

@danwinship danwinship force-pushed the danwinship:networkpolicy-pod-plus-ns branch from 136f147 to 8bc6eda Feb 28, 2018

@Lion-Wei

This comment has been minimized.

Copy link
Contributor

Lion-Wei commented Mar 2, 2018

@danwinship I will push that pr as soon as this got merged. : )

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Mar 2, 2018

@caseydavenport repushed with the stray update.sh change removed if you want to /lgtm it

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Mar 2, 2018

/lgtm

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 3, 2018

/approve

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 5, 2018

This change is Reviewable

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 5, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 5, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 9, 2018

@danwinship danwinship force-pushed the danwinship:networkpolicy-pod-plus-ns branch from 5e25d12 to 8bc6eda Mar 9, 2018

@oivindoh

This comment has been minimized.

Copy link

oivindoh commented Mar 21, 2018

What's holding this thing of outstanding natural beauty back? Just re-applying /lgtm?

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented Mar 21, 2018

It's waiting for 1.10 to go out and master to reopen for 1.11. I'm not sure where the /lgtm went.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 23, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 23, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 24, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 24, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 24, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 24, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 24, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 0b062e1 into kubernetes:master Mar 24, 2018

14 of 15 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation danwinship authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-unit Context retired. Status moved to "pull-kubernetes-integration".
pull-kubernetes-verify Job succeeded.
Details

@danwinship danwinship deleted the danwinship:networkpolicy-pod-plus-ns branch Mar 26, 2018

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented May 8, 2018

Is this technically a breaking change in the behavior? (or was it impossible to specify both namespaceSelector and podSelector on the same peer spec?)

@oivindoh

This comment has been minimized.

Copy link

oivindoh commented May 8, 2018

@caseydavenport does this require a change in calico to work?

@danwinship

This comment has been minimized.

Copy link
Contributor Author

danwinship commented May 11, 2018

@ahmetb It was previously impossible to specify both; validation would reject the resource in that case

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented May 15, 2018

@oivindoh yeah, we'll need to make a minor change to Calico in order to make this work. Should be really straightforward.

Calico supports this already within its policy model, but the code which converts kubernetes policies doesn't yet expect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.