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

Added support of Namespace and Pod selector for networkpolicy #21196

Merged

Conversation

JacobTanenbaum
Copy link
Contributor

currently Openshift networkpolicy only allows for using either peer.PodSelector or peer.NamespaceSelector. This brings the operation of Openshift's networkpolicy support closer to that of upstream by allowing the use of both.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 8, 2018
@JacobTanenbaum
Copy link
Contributor Author

/test e2e-gcp

@JacobTanenbaum
Copy link
Contributor Author

@danwinship PTAL

for _, pod := range np.pods {
vnid, exists := namespaces[pod.Namespace]
if exists && podSel.Matches(labels.Set(pod.Labels)) {
vnidsIPs[vnid] = pod.Status.PodIP
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only work for a single matching pod per namespace. I think you need to return an array of {vnid, podIP}. (Or else just do the fmt.Sprintf here and return an array of strings; in that case you should probably change selectPods and selectNamespaces to match, for consistency.)


testCannotConnect(f, nsA, "client-a", service, 80)
testCannotConnect(f, nsB, "client-b1", service, 80)
testCanConnect(f, nsB, "client-b2", service, 80)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's missing one of the 4 possible cases (matching podSelector, non-matching namespaceSelector):

testCannotConnect(f, nsA, "client-b2", service, 80)

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 12, 2018
@JacobTanenbaum
Copy link
Contributor Author

/test integration

Expect(err).NotTo(HaveOccurred())

// Create Policy for that service that allows traffic only via namespace B
By("Creating a network policy for the server which allows traffic from namespace-b.")
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, didn't notice this before, but a bunch of the comments / log messages / object names here seem to have been copied from the namespace-only test, and need to be updated to clarify that this is testing combined namespaceSelector+podSelector

for _, pod := range np.pods {
vnid, exists := namespaces[pod.Namespace]
if exists && podSel.Matches(labels.Set(pod.Labels)) {
//vnidsIPs[vnid] = pod.Status.PodIP
Copy link
Contributor

Choose a reason for hiding this comment

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

kill that

@JacobTanenbaum
Copy link
Contributor Author

/retest

@JacobTanenbaum
Copy link
Contributor Author

/test e2e-gcp

…d peer.NamespaceSelector

currently openshift networkpolicy only allows for using either peer.PodSelector or peer.NamespaceSelector.
This brings the operation of Openshift's networkpolicy support closer to that of upstream.

Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
@JacobTanenbaum
Copy link
Contributor Author

/retest

@danwinship
Copy link
Contributor

What happened to the change to selectPods()? Oh, had you accidentally dropped the "ip, " from the flow before and that's what broke things? You could just include that in the return values from selectPods too. (Also, maybe rename the arrays inside selectPods() and selectNamespaces() to "flows" or "peerFlows" or something?)

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@JacobTanenbaum
Copy link
Contributor Author

/test extended_clusterup

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@JacobTanenbaum
Copy link
Contributor Author

/test extended_clusterup

@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 19, 2018

@JacobTanenbaum: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_clusterup ebca3b6 link /test extended_clusterup

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.

@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit a2d3362 into openshift:master Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants