gistfile1.txt

diff --git a/test/e2e/network/network_policy.go b/test/e2e/network/network_policy.go
index 676e6b25e2c..1b6924711d1 100644
--- a/test/e2e/network/network_policy.go
+++ b/test/e2e/network/network_policy.go
@@ -18,6 +18,7 @@ package network
 
 import (
 	"encoding/json"
+
 	v1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -56,6 +57,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 	ginkgo.Context("NetworkPolicy between server and client", func() {
 		ginkgo.BeforeEach(func() {
 			ginkgo.By("Creating a simple server that serves on port 80 and 81.")
+			// ports 80 and 81, because
 			podServer, service = createServerPodAndService(f, f.Namespace, "server", []int{80, 81})
 
 			ginkgo.By("Waiting for pod ready", func() {
@@ -101,7 +103,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			nsBName := f.BaseName + "-b"
 			nsB, err := f.CreateNamespace(nsBName, map[string]string{
 				"ns-name": nsBName,
-			})
+			}):w:
 			framework.ExpectNoError(err, "Error occurred while creating namespace-b.")
 
 			// All communication should be possible before applying the policy.
@@ -144,6 +146,8 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// ginkgo.By("Creating client-a, in server's namespace, which should be able to any server other then. podServerLabelSelector"
+
 			ginkgo.By("Creating client-a, in server's namespace, which should be able to contact the server.", func() {
 				testCanConnect(f, nsA, "client-a", service, 80)
 			})
@@ -163,6 +167,9 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			})
 			framework.ExpectNoError(err)
 
+
+			// missing positive control , see above test
+
 			// Create Server with Service in NS-B
 			framework.Logf("Waiting for server to come up.")
 			err = e2epod.WaitForPodRunningInNamespace(f.ClientSet, podServer)
@@ -197,11 +204,18 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// Need By statements here
+
 			testCannotConnect(f, nsA, "client-a", service, 80)
+			testCannotConnect(f, nsA, "client-b", service, 80) // <-- also need a pod test for consistency
 			testCanConnect(f, nsB, "client-b", service, 80)
 		})
 
 		ginkgo.It("should enforce policy based on PodSelector with MatchExpressions[Feature:NetworkPolicy]", func() {
+
+			// not confirming connectivity before this policy is enacted means that the test is
+			// not equivalent to previous tests.
+
 			ginkgo.By("Creating a network policy for the server which allows traffic from the pod 'client-a'.")
 			policy := &networkingv1.NetworkPolicy{
 				ObjectMeta: metav1.ObjectMeta{
@@ -234,6 +248,8 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			ginkgo.By("Creating client-a which should be able to contact the server.", func() {
 				testCanConnect(f, f.Namespace, "client-a", service, 80)
 			})
+
+			// what about port 81
 			ginkgo.By("Creating client-b which should not be able to contact the server.", func() {
 				testCannotConnect(f, f.Namespace, "client-b", service, 80)
 			})
@@ -253,7 +269,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			})
 			framework.ExpectNoError(err, "Error creating namespace %v: %v", nsCName, err)
 
-			// Create Policy for the server that allows traffic from namespace different than namespace-a
+			// Create Policy for the server that allows traffic from namespace different than namespace-a (think we mean c here?)
 			ginkgo.By("Creating a network policy for the server which allows traffic from ns different than namespace-a.")
 			policy := &networkingv1.NetworkPolicy{
 				ObjectMeta: metav1.ObjectMeta{
@@ -271,6 +287,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 								MatchExpressions: []metav1.LabelSelectorRequirement{{
 									Key:      "ns-name",
 									Operator: metav1.LabelSelectorOpNotIn,
+									// see the -c above, i think thats what we mean here.
 									Values:   []string{nsCName},
 								}},
 							},
@@ -283,6 +300,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err, "Error creating Network Policy %v: %v", policy.ObjectMeta.Name, err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// the tests confirm that namespace C is the differentiator, not A.
 			testCannotConnect(f, nsC, "client-a", service, 80)
 			testCanConnect(f, nsB, "client-a", service, 80)
 		})
@@ -295,7 +313,6 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			})
 			framework.ExpectNoError(err, "Error creating namespace %v: %v", nsBName, err)
 
-			// Create Policy for the server that allows traffic only via client B or namespace B
 			ginkgo.By("Creating a network policy for the server which allows traffic from client-b or namespace-b.")
 			policy := &networkingv1.NetworkPolicy{
 				ObjectMeta: metav1.ObjectMeta{
@@ -308,19 +325,23 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 						},
 					},
 					Ingress: []networkingv1.NetworkPolicyIngressRule{{
-						From: []networkingv1.NetworkPolicyPeer{{
-							PodSelector: &metav1.LabelSelector{
-								MatchLabels: map[string]string{
-									"pod-name": "client-b",
-								},
-							},
-						}, {
-							NamespaceSelector: &metav1.LabelSelector{
-								MatchLabels: map[string]string{
-									"ns-name": nsBName,
-								},
-							},
-						}},
+						From: []networkingv1.NetworkPolicyPeer{
+							{
+								// TODO add these composably, so that can be disambugated from combo networkpolicypeer
+									PodSelector: &metav1.LabelSelector{
+										MatchLabels: map[string]string{
+											"pod-name": "client-b",
+										},
+									},
+							}, 
+							{
+									NamespaceSelector: &metav1.LabelSelector{
+										MatchLabels: map[string]string{
+											"ns-name": nsBName,
+										},
+									},
+							}
+						},
 					}},
 				},
 			}
@@ -355,18 +376,22 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 						},
 					},
 					Ingress: []networkingv1.NetworkPolicyIngressRule{{
-						From: []networkingv1.NetworkPolicyPeer{{
-							PodSelector: &metav1.LabelSelector{
-								MatchLabels: map[string]string{
-									"pod-name": "client-b",
+						From: []networkingv1.NetworkPolicyPeer{
+							{
+								PodSelector: &metav1.LabelSelector{
+									MatchLabels: map[string]string{
+										"pod-name": "client-b",
+									},
 								},
-							},
-							NamespaceSelector: &metav1.LabelSelector{
-								MatchLabels: map[string]string{
-									"ns-name": nsBName,
+								// similar to the above issue this is subtlel;'y different from above, we should
+								//have an easy way to read the difference
+								NamespaceSelector: &metav1.LabelSelector{
+									MatchLabels: map[string]string{
+										"ns-name": nsBName,
+									},
 								},
-							},
-						}},
+							}
+						},
 					}},
 				},
 			}
@@ -447,7 +472,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			})
 			ginkgo.By("Creating client-b, in server's namespace, which should not be able to contact the server.", func() {
 				testCannotConnect(f, nsA, "client-b", service, 80)
-			})
+			}) // this is the positive test, for consistency, it should be at the end, rather then in the middle ?
 			ginkgo.By("Creating client-a, not in server's namespace, which should be able to contact the server.", func() {
 				testCanConnect(f, nsB, "client-a", service, 80)
 			})
@@ -483,6 +508,8 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 
 			ginkgo.By("Testing pods can connect only to the port allowed by the policy.")
 			testCannotConnect(f, f.Namespace, "client-a", service, 80)
+
+			// also need to test that clients in *other* namespaces can connect to 81, as we do in other tests.
 			testCanConnect(f, f.Namespace, "client-b", service, 81)
 		})
 
@@ -536,6 +563,8 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			defer cleanupNetworkPolicy(f, policy2)
 
 			ginkgo.By("Testing pods can connect to both ports when both policies are present.")
+
+			// also need to confirm the negative cases here
 			testCanConnect(f, f.Namespace, "client-a", service, 80)
 			testCanConnect(f, f.Namespace, "client-b", service, 81)
 		})
@@ -559,6 +588,11 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			defer cleanupNetworkPolicy(f, policy)
 
 			ginkgo.By("Testing pods can connect to both ports when an 'allow-all' policy is present.")
+
+			// also need to confirm the negative cases here
+			
+			// also need to confirm the positive namespaces here
+			
 			testCanConnect(f, f.Namespace, "client-a", service, 80)
 			testCanConnect(f, f.Namespace, "client-b", service, 81)
 		})
@@ -588,6 +622,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// we should specify that it can contact the server specifically on 80 here.
 			ginkgo.By("Creating client-a which should be able to contact the server.", func() {
 				testCanConnect(f, f.Namespace, "client-a", service, 80)
 			})
@@ -596,6 +631,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			})
 		})
 
+		// i think we should qualify this as 'exactly'  vs 'one or more'
 		ginkgo.It("should allow ingress access from namespace on one named port [Feature:NetworkPolicy]", func() {
 			nsBName := f.BaseName + "-b"
 			nsB, err := f.CreateNamespace(nsBName, map[string]string{
@@ -635,10 +671,12 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err, "Error creating Network Policy %v: %v", policy.ObjectMeta.Name, err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// use client-b for a simple delta here
 			testCannotConnect(f, f.Namespace, "client-a", service, allowedPort)
 			testCanConnect(f, nsB, "client-b", service, allowedPort)
 		})
 
+		// we dont specify the namedport/dns dependency explicitly here, i think we should.
 		ginkgo.It("should allow egress access on one named port [Feature:NetworkPolicy]", func() {
 			clientPodName := "client-a"
 			protocolUDP := v1.ProtocolUDP
@@ -673,6 +711,9 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// mention dns lookup here
+
+			// also we should test an external namespace, since that is done in other tests.
 			ginkgo.By("Creating client-a which should be able to contact the server.", func() {
 				testCanConnect(f, f.Namespace, clientPodName, service, 80)
 			})
@@ -686,6 +727,10 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 				clientAAllowedPort    = 80
 				clientANotAllowedPort = 81
 			)
+
+			// should we have one testCanConnect up here, so that the Cannot connect clearely represents a delta?
+
+			// i wonder wether the tests here should be exported as YAML and placed in kubernetes/examples somehow...
 			ginkgo.By("Creating a network policy for the Service which allows traffic from pod at a port")
 			policy := &networkingv1.NetworkPolicy{
 				ObjectMeta: metav1.ObjectMeta{
@@ -809,6 +854,7 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err, "Error creating Network Policy %v: %v", policy.ObjectMeta.Name, err)
 			defer cleanupNetworkPolicy(f, policy)
 
+			// #t1 will reference this later.
 			testCannotConnect(f, nsB, "client-a", service, allowedPort)
 
 			nsB, err = f.ClientSet.CoreV1().Namespaces().Get(nsB.Name, metav1.GetOptions{})
@@ -818,6 +864,9 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			nsB, err = f.ClientSet.CoreV1().Namespaces().Update(nsB)
 			framework.ExpectNoError(err, "Error updating Namespace %v: %v", nsB.ObjectMeta.Name, err)
 
+
+			// for testCanConnect to be meaningful, it should be identical to #t1 above
+			// also, we probably should be verifying here that other namespaces can still *not* connect.
 			testCanConnect(f, nsB, "client-b", service, allowedPort)
 		})
 
@@ -866,7 +915,8 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			checkConnectivity(f, f.Namespace, podClient, service)
 		})
 
-		ginkgo.It("should enforce egress policy allowing traffic to a server in a different namespace based on PodSelector and NamespaceSelector [Feature:NetworkPolicy]", func() {
+		// we should say "matching BOTH podSelector AND Namespaceselector"
+		ginkgo.It("should enforce egress policy allowing traffic to a server in a different namespace based PodSelector and NamespaceSelector [Feature:NetworkPolicy]", func() {
 			var nsBserviceA, nsBserviceB *v1.Service
 			var nsBpodServerA, nsBpodServerB *v1.Pod
 
@@ -878,11 +928,16 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err, "Error occurred while creating namespace-b.")
 
 			// Creating pods and services in namespace-b
+
+			//call these services "foo" and "bar" to differentiate namespace titles "a/b", i.e. nsBpodFoo, nsBpodBar
 			nsBpodServerA, nsBserviceA = createServerPodAndService(f, nsB, "ns-b-server-a", []int{80})
 			nsBpodServerB, nsBserviceB = createServerPodAndService(f, nsB, "ns-b-server-b", []int{80})
 
+
 			// Wait for Server with Service in NS-A to be ready
 			framework.Logf("Waiting for servers to come up.")
+
+			// shouldnt this be part of the Giinkgo.Before() ? podServer is part of the bootstrapped setup in BeforeEach()
 			err = e2epod.WaitForPodRunningInNamespace(f.ClientSet, podServer)
 			framework.ExpectNoError(err, "Error occurred while waiting for pod status in namespace: Running.")
 
@@ -946,6 +1001,9 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err, "Error occurred while creating policy: policyAllowToServerInNSB.")
 			defer cleanupNetworkPolicy(f, policyAllowToServerInNSB)
 
+			// minor:
+			// Mentioned this earlier, but i think maybe we want to test both ports
+			// Also, i think foo and bar make more sense for these pod servers.
 			ginkgo.By("Creating client-a, in 'namespace-a', which should be able to contact the server-a in namespace-b.", func() {
 				testCanConnect(f, nsA, "client-a", nsBserviceA, 80)
 			})
@@ -1188,6 +1246,8 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 
 		})
 
+		// we should update this description to clarify that we only test one pod with a /32, and btw, 
+		// is this something we can run on ipv6 clusters ?
 		ginkgo.It("should allow egress access to server in CIDR block [Feature:NetworkPolicy]", func() {
 			var serviceB *v1.Service
 			var podServerB *v1.Pod
@@ -1259,14 +1319,19 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			framework.ExpectNoError(err, "Error occurred while creating policy: policyAllowCIDR.")
 			defer cleanupNetworkPolicy(f, policyAllowCIDR)
 
+			// for parity with other tests, should we have multiple 'cannotConnect' verifications here?
 			ginkgo.By("Creating client-a which should not be able to contact the server-b.", func() {
 				testCannotConnect(f, f.Namespace, "client-a", serviceB, 80)
 			})
+			
 			ginkgo.By("Creating client-a which should be able to contact the server.", func() {
 				testCanConnect(f, f.Namespace, "client-a", service, 80)
 			})
 		})
 
+		// i think we also need to make sure there is a test for enforcing multiple policies with a NamespaceSelector.
+
+		// i think we mean "should enforce multiple separate policies against the same PodSelector"
 		ginkgo.It("should enforce policies to check ingress and egress policies can be controlled independently based on PodSelector [Feature:NetworkPolicy]", func() {
 			var serviceA, serviceB *v1.Service
 			var podA, podB *v1.Pod
@@ -1505,6 +1570,7 @@ func createServerPodAndService(f *framework.Framework, namespace *v1.Namespace,
 			Ports: []v1.ContainerPort{
 				{
 					ContainerPort: int32(port),
+					// the serve-80 is referenced in tests specifically, so dont change the name!
 					Name:          fmt.Sprintf("serve-%d", port),
 				},
 			},