Add networkpolicies #2773

karampok · 2025-05-26T12:57:48Z

/kind feature

What this PR does / why we need it:

Special notes for your reviewer:
Adding networkpolicies support, and that is an opt-in feature.
E2E test are executed with network policies enabled.

Release note:

 Add network policy support to Helm charts and manifests

    - Network policies are disabled by default. User can enable them in Helm
      by setting the value `networkpolicies.enabled` true. When deploying
      using manifests the user should modify the config/native/kustomization.yaml
      to include the network policies.

    - User can install a default deny network policy rule if needed. In Helm
      by setting the chart value `networkpolicies.defaultDeny` to true. In
      manifest by modifying accordingly the config/networkpolicies/kustomization.yaml
      to include or exclude the deny_all policy.

oribon · 2025-06-08T09:44:59Z

charts/metallb/templates/networkpolicy.yaml

+      {{- include "metallb.selectorLabels" . | nindent 6 }}
+      app.kubernetes.io/component: controller
+  egress:
+    - ports:


can you add to the commit message why this exact ports (and same for ingress)?

Updated the commit message

oribon · 2025-06-08T09:48:50Z

config/networkpolicies/controller.yaml

+    app: metallb
+    component: controller
+ egress:
+  - ports:


why we don't need 443 here?

bz

oc get svc kubernetes -o yaml apiVersion: v1 kind: Service metadata: creationTimestamp: "2025-06-10T08:42:56Z" labels: component: apiserver provider: kubernetes name: kubernetes namespace: default resourceVersion: "195" uid: 8809a655-eee9-4e1d-863a-3147dbe44424 spec: clusterIP: 10.96.0.1 clusterIPs: - 10.96.0.1 internalTrafficPolicy: Cluster ipFamilies: - IPv4 ipFamilyPolicy: SingleStack ports: - name: https port: 443 protocol: TCP targetPort: 6443 sessionAffinity: None type: ClusterIP

how the controller speaks is through the svc and the networkpolicies seems to operate on the target port (at least in kind cluster and in OCP). I cleaned up the port egress port 443 from the helm chart .

Having said that I will search deeper if there is standardized documenation how the CNI network should work on the targetPort or not.

oribon · 2025-06-08T09:53:04Z

controller/main.go

I don't think this is necessary (nor visible/informative enough), I think we should just drop it. (if our networkpolicies caused an error then it is a bug, and if custom policies were used we can't shield users from a misconfig)

agree, it's not worth the extra complexity

okay, commit dropped

fedepaol · 2025-06-09T09:58:48Z

charts/metallb/templates/networkpolicy.yaml

+  ingress:
+    - ports:
+        {{- if .Values.controller.webhookMode }}
+        - protocol: TCP


I think it's gonna be more future-proof if we use named ports. IIUC the spec allows it https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#networkpolicyport-v1-networking-k8s-io

If we don't have these ports under a name, we can add them.

done
(nice tip)

fedepaol · 2025-06-09T10:00:23Z

charts/metallb/README.md

@@ -68,6 +68,7 @@ Kubernetes: `>= 1.19.0-0`
 | imagePullSecrets | list | `[]` |  |
 | loadBalancerClass | string | `""` |  |
 | nameOverride | string | `""` |  |
+| networkpolicies.defaultDeny | bool | `false` |  |


what's the use case for this vs disabling / enabling netpols at all?

For example if you install metallb in a different namespace, where other things might exist you want your policies but not a default-deny that will break the other things. In OCP, the plan is eventually external component to install default rules and not added by the components. If you like we can exclude it from the helm API (but in that case the metallb-operator will add it somehow)

makes sense, thanks!

fedepaol · 2025-06-09T10:02:52Z

controller/main.go

agree, it's not worth the extra complexity

fedepaol · 2025-06-09T10:05:35Z

charts/metallb/templates/networkpolicy.yaml

+  egress:
+    - ports:
+        - protocol: TCP
+          port: 6443


these should be available to override, as we don't have control over them. Also, the speaker reaches out the apiserver too, but I guess we are not adding it as it's host networked. Can you add an explicit commetn about it?

fedepaol · 2025-06-09T10:06:37Z

Also, please enrich the release notes a bit saying what we are going to block. Also also, the network policies are enabled if you deploy with manifests / kustomize

karampok · 2025-06-11T07:00:56Z

@fedepaol @oribon I think I have addressed all your comments, ptal thank you

fedepaol · 2025-06-12T06:42:08Z

Looks good! Would it make sense to add a test ensuring that the network policies are effectively working?
I am thinking about curling to / from a different pod.

karampok · 2025-06-12T08:49:33Z

Looks good! Would it make sense to add a test ensuring that the network policies are effectively working? I am thinking about curling to / from a different pod.

That is a bit tricky.
There is a default-deny, if our network-policy are not correct then all test are failling because operator never healthy. If the kind cluster does not support netpol (which it does), we could have not-related metallb-test to verify it (but I think it not very meaningful).

We could have have one other pod (kubectl run pod ) in the namespace to try to reach the metrics port to verify that ingress traffic is blocked (but that mean we should have add a check if netpol enable and default deny exist, and we should logic to grub the pod IP and such)
I cannot thing of good way to test the egress, it should have like kubectl debug pod to have a shell in the controller network namespace

Is that something you have in mind?

(update maybe we could have that test in the operator,)

IMO it's not worth the extra complexity but we can do it

fedepaol · 2025-06-12T08:59:45Z

Looks good! Would it make sense to add a test ensuring that the network policies are effectively working? I am thinking about curling to / from a different pod.

That is a bit tricky. There is a default-deny, if our network-policy are not correct then all test are failling because operator never healthy. If the kind cluster does not support netpol (which it does), we could have not-related metallb-test to verify it (but I think it not very meaningful).

Not sure I understood this part.

1. We could have have one other pod (`kubectl run pod `) in the namespace to try to reach the metrics port to verify that ingress traffic is blocked (but that mean we should have add a check `if netpol enable and default deny exist`, and we should logic to grub the pod IP and such)

2. I cannot thing of good way to test the egress, it should have like `kubectl debug pod` to have a shell in the controller network namespace

Is that something you have in mind?

(update maybe we could have that test in the operator,)

IMO it's not worth the extra complexity but we can do it

I am not comfortable in saying "it works" if we don't have a way to verify it.
What I was suggesting was to spin up a pod that we try to reach from the controller and that tries to curl the controller, to ensure that the netpol we are configuring are really effective.

karampok · 2025-06-12T09:34:07Z

we try to reach from the controller

how do you suggest to do that given we have distroless container without a shell?

fedepaol · 2025-06-12T10:19:06Z

we try to reach from the controller

how do you suggest to do that given we have distroless container without a shell?

https://metallb.io/troubleshooting/#how-to-debug-the-speaker-and-the-controller-containers

oribon · 2025-06-18T12:51:05Z

e2etest/pkg/executor/executor.go

not sure I understand the rationale of this change (is it to be able to run the same debug twice?)?
if that's the reason, it seems a bit overcomplex (vs what we had before), is it possible (if reasonable) to just randomize the name of the ephemeral container instead when Execing?

Debug container is a container attached to the pod, and we can do from
there normal kubectl exec commands. In that way we avoid running any
-it argument neither adding more than one debug container per target
container.

Having the go client instead of kubectl is more flexible and is required
to catch errors as well as to wait for the debug pod to be ready

(from the commit message).
I think that is more recommended implementation and not so complex imo.

Having an -interactive shell when doing not interactive call wrong (if I remove it it fails)
Return values of the command is more straightforward.
Speed also is reason ( exec is faster) if we want to extend the number of debug command
Debug container cannot be removed so kind the dynamic name will leak as many as calls as many containers
Having a go-client implementation is better (we still use kubectl but we can start not using it)

what I mean is why specifically we need this change, what are we doing now that we couldn't do before? is it the if we want to extend the number of debug command part?

So if I revert and have in test code

ctrlExec := executor.ForPodDebug(controller.Namespace, controller.Name, "controller", agnhostImage) out, err = ctrlExec.Exec("./agnhost", "connect", net.JoinHostPort(probeIP, "8080"), "--timeout", "5s") Expect(err).NotTo(HaveOccurred()) # this error is nil, which in the refactor case is not nil as expected Expect(out).To(ContainSubstring("TIMEOUT")) // If no netpol then REFUSED

then the test is failing

SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS ------------------------------ Networkpolicies only allowed traffic /home/kka/workspace/github.com/metallb/wts/up-np-metallb/e2etest/netpoltests/networkpolicies.go:89 STEP: checking ingress - probe pod to any port on the controller other than webhook should be timeout @ 06/19/25 12:56:10.11 STEP: checking egress - controller pod to any port on the probe other port than API should be timeout @ 06/19/25 12:56:15.324 [FAILED] in [It] - /home/kka/workspace/github.com/metallb/wts/up-np-metallb/e2etest/netpoltests/networkpolicies.go:104 @ 06/19/25 12:56:15.88 • [FAILED] [19.888 seconds] Networkpolicies [It] only allowed traffic /home/kka/workspace/github.com/metallb/wts/up-np-metallb/e2etest/netpoltests/networkpolicies.go:89 [FAILED] Expected <string>: Targeting container "controller". If you don't see processes from this container it may be because the container runtime doesn't support this feature. Unable to use a TTY - input is not a terminal or the right kind of file If you don't see a command prompt, try pressing enter. warning: couldn't attach to pod/controller-765bbcb6fc-dzcv4, falling back to streaming logs: error stream protocol error: unknown error to contain substring <string>: TIMEOUT In [It] at: /home/kka/workspace/github.com/metallb/wts/up-np-metallb/e2etest/netpoltests/networkpolicies.go:104 @ 06/19/25 12:56:15.88 ------------------------------ SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS --

which is hiding the real stdout from the process, and me trying to fix it I refactor, did not dig deeper (as refactor seemed the right approach)

/home/kka/workspace/github.com/metallb/wts/up-np-metallb/build/kubectl debug -n metallb-system -c controller-debugger-x --target=controller --image=registry.k8s.io/e2e-test-images/agnhost:2.47 controller-765bbcb6fc-dzcv4 -- ./agnhost connect 10.244.1.7:8080 --timeout 5s Targeting container "controller". If you don't see processes from this container it may be because the container runtime doesn't support this feature.

just running the command I do not see the stdout

with -it (which we should not have in theory) I can see but there is some time waiting (kind the go code does not wait but it returns)

/home/kka/workspace/github.com/metallb/wts/up-np-metallb/build/kubectl debug -it -n metallb-system -c controller-debugger-xxx --target=controller --image=registry.k8s.io/e2e-test-images/agnhost:2.47 controller-765bbcb6fc-dzcv4 -- ./agnhost connect 10.244.1.7:8080 --timeout 5s Targeting container "controller". If you don't see processes from this container it may be because the container runtime doesn't support this feature. If you don't see a command prompt, try pressing enter. --- here is hanging for a bit --- TIMEOUT Session ended, the ephemeral container will not be restarted but may be reattached using 'kubectl attach controller-765bbcb6fc-dzcv4 -c controller-debugger-xxx -i -t' if it is still running

Re-word the commit description with our chat findings

only thing I still don't understand from the discussion is why the current test using it is failing. If the command was never returning the right value, I am not able to justify the mac assertion here

metallb/e2etest/l2tests/interface_selector.go

Line 160 in c759397

selectorMac, err := mac.GetIfaceMac(NodeNics[0], executor.ForPodDebug(speakerPod.Namespace, speakerPod.Name, "speaker", agnostImage))

You probably chatted about this, but the conclusion in the commit message seem either not accurate or the current test is volkswagening

therefore it returns neither the correct output (= only the output from the command we want to run), neither the error (if the command fails the kubectl debug actually returns ok).

the observation is that for the command that returns instantly (whoami/ifconfig), it works just fine. It
does not work fine for the commands that will timeout/exit. I re-wrote the commit so it is clear that the above sentence is only valid for the specific type of commans (which is agnhost connect --timeout).

oribon · 2025-06-18T12:56:27Z

e2etest/pkg/metallb/metallb.go

+func CreatePod(cs clientset.Interface, p *corev1.Pod) (*corev1.Pod, error) {
+	pod, err := cs.CoreV1().Pods("default").Create(context.TODO(), p, metav1.CreateOptions{})
+	if err != nil {
+		return nil, err
+	}
+	return WaitForPodReady(cs, pod.Namespace, pod.Name)
+}


this seems a bit opinionated (the "default" part at least, I guess it should be p.Namespace), I think it makes sense to omit this function and the next, creating/deleting directly where needed

yes, I fixed it, it was mistake. I could skip the function but I should skip also the delete function and then the main code will look more packed ( I think I also got feedback to hide those under function in the past)

fair enough (though I guess we can now omit the Delete part since deleting the namespace takes care of that)

oribon · 2025-06-18T12:57:01Z

e2etest/netpoltests/networkpolicies.go

+		tpod := &corev1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "netpol-test",
+				Namespace: "default", // probe pod must not be in the metallb-system namespace


can we create a new ns for this test (and remove it in the defer)?

Sure I can move into a random new namespace, I see not argument for/against though

I updated to use test-namespace

I see not argument for/against though

we should avoid polluting namespaces (even if not very probable)

tasks.py

oribon

few nits, lgtm otherwise

oribon · 2025-06-19T14:20:55Z

e2etest/pkg/executor/executor.go

 }

-func ForPodDebug(namespace, name, container, image string) Executor {
-	return &podDebugExecutor{
+func ForPodDebug(cs clientset.Interface, namespace, podName, targetContainer string) (Executor, error) {


can we pass the image as input like before instead of hardcoding agnhost?

oribon · 2025-06-19T14:21:30Z

e2etest/pkg/executor/executor.go

+		cs:        cs,
+	}
+
+	ephemeralName := "debugger-" + targetContainer


nit: drop this and use ret.ephemeral (or drop references to ret.ephemeral, put this in ret and always use it)

e2etest/pkg/executor/executor.go

oribon · 2025-06-19T14:23:31Z

e2etest/pkg/executor/executor.go

 	}
+
+	return ret, wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 20*time.Second, false, func(context.Context) (bool, error) {


nit: can the wait be before the return, and the usual if err != nil after?

oribon · 2025-06-19T14:25:47Z

e2etest/netpoltests/networkpolicies.go

+	})
+})
+
+func expectedPolicies() ([]networkingv1.NetworkPolicy, error) {


I think this is unnecessary (checking that the objs are there), e.g for the same reasons we don't check the webhook objects are there before running the webhook e2es. I guess we can keep it if you see there's value in this

I have mixed feeling, I will wait @fedepaol feedback

Agree on this being redundant, we want to ensure the behavior, not the configuration that leads to the behavior.

oribon · 2025-06-19T14:27:37Z

e2etest/netpoltests/networkpolicies.go

+		probe, err = metallb.CreatePod(cs, tpod)
+		Expect(err).NotTo(HaveOccurred())
+		ginkgo.DeferCleanup(func() {
+			err = metallb.DeletePod(cs, probe)


nit: this is redundant (taken care by deleting the namespace

oribon · 2025-06-19T14:32:01Z

config/networkpolicies/controller.yaml

+      - protocol: TCP
+        port: 6443
+  ingress:
+    - ports:


can we also have here monitoring?

why? I added as a commented, in all case this is not enabled by default, t user must actively change the manifest (and we should not recommend use the non https port)

- Network policies are disabled by default. User can enable them in Helm by setting the value `networkpolicies.enabled` true. When deploying using manifests the user should modify the config/native/kustomization.yaml to include the network policies. - User can install a default deny network policy rule if needed. In Helm by setting the chart value `networkpolicies.defaultDeny` to true. In manifest by modifying accordingly the config/networkpolicies/kustomization.yaml to include or exclude the deny_all policy. E2E tests are adding the network policies and the default deny rule deny_all as part of the setup in the tasks.py file. The Egress network policy port for the k8s API is 6443 because the connection takes place through the k8s service, (as shown below). This should be safe but in Helm charts there is a value to override it if needed. ``` kubectl get svc kubernetes -o yaml apiVersion: v1 kind: Service metadata: name: kubernetes namespace: default spec: ... ports: - name: https port: 443 protocol: TCP targetPort: 6443 ... ``` Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>

fedepaol · 2025-06-20T13:36:19Z

e2etest/l2tests/interface_selector.go

@@ -25,7 +25,7 @@ import (
 	"go.universe.tf/e2etest/pkg/status"
 )

-const agnostImage = "registry.k8s.io/e2e-test-images/agnhost:2.45"
+const agnhostImage = "registry.k8s.io/e2e-test-images/agnhost:2.47"


nit: do we need this change? In any case, not belongs to this commit

removed it from the commit

fedepaol · 2025-06-20T13:40:15Z

e2etest/pkg/executor/executor.go

 	}
+
+	if !containerExists {
+		pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, ephemeralContainer)


I'd move this to a "addEphemeralContainerToPod" function

Done (if I understood your ask correctly)

fedepaol · 2025-06-20T13:46:03Z

e2etest/pkg/executor/executor.go

only thing I still don't understand from the discussion is why the current test using it is failing. If the command was never returning the right value, I am not able to justify the mac assertion here

metallb/e2etest/l2tests/interface_selector.go

Line 160 in c759397

selectorMac, err := mac.GetIfaceMac(NodeNics[0], executor.ForPodDebug(speakerPod.Namespace, speakerPod.Name, "speaker", agnostImage))

You probably chatted about this, but the conclusion in the commit message seem either not accurate or the current test is volkswagening

therefore it returns neither the correct output (= only the output from the command we want to run), neither the error (if the command fails the kubectl debug actually returns ok).

fedepaol · 2025-06-20T13:52:08Z

e2etest/pkg/metallb/metallb.go

@@ -150,3 +149,26 @@ func FRRK8SPods(cs clientset.Interface, namespace string) ([]*corev1.Pod, error)
 	}
 	return res, nil
 }
+
+func CreatePod(cs clientset.Interface, p *corev1.Pod) (*corev1.Pod, error) {


Let's move these under pkg/k8s

fedepaol · 2025-06-20T13:53:17Z

e2etest/netpoltests/networkpolicies.go

+	})
+})
+
+func expectedPolicies() ([]networkingv1.NetworkPolicy, error) {


Agree on this being redundant, we want to ensure the behavior, not the configuration that leads to the behavior.

I observed that the existing implementation works correctly for commands that return instantly like whoami, ifconfig but no for commands that take time to return and that they fail e.g. ``` kubectl debug -it -n metallb-system -c controller-debugger-xxx --target=controller --image=registry.k8s.io/e2e-test-images/agnhost:2.47 controller-765bbcb6fc-dzcv4 -- ./agnhost connect 10.244.1.7:8080 --timeout 5s Targeting container "controller". If you don't see processes from this container it may be because the container runtime doesn't support this feature. If you don't see a command prompt, try pressing enter. --- hanging for a bit --- TIMEOUT Session ended, the ephemeral container will not be restarted but may be reattached using 'kubectl attach controller-765bbcb6fc-dzcv4 -c controller-debugger-xxx -i -t' if it is still running ``` For the commands that take time to return, current implementation is not waiting and returns before the actual command finishes (probably due to how TTY works), and therefore it returns neither the correct output (= only the output from the command we want to run), neither the error (if the command fails the kubectl debug actually returns ok). So this commit refactors the code and follows the logic where the debug container is a container attached to the pod, and we can do from there normal `kubectl exec` commands without the `-it` argument and that enables the e2etest for networkpolicies. https://kubernetes.io/docs/tasks/debug/debug-application/debug-running-pod/#ephemeral-container Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>

Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>

karampok

@fedepaol I updated according to your comments, let me know I missed something

karampok · 2025-06-23T07:50:31Z

e2etest/pkg/executor/executor.go

 	}
+
+	if !containerExists {
+		pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, ephemeralContainer)


Done (if I understood your ask correctly)

karampok · 2025-06-23T07:50:40Z

e2etest/pkg/metallb/metallb.go

@@ -150,3 +149,26 @@ func FRRK8SPods(cs clientset.Interface, namespace string) ([]*corev1.Pod, error)
 	}
 	return res, nil
 }
+
+func CreatePod(cs clientset.Interface, p *corev1.Pod) (*corev1.Pod, error) {


karampok · 2025-06-23T07:51:01Z

e2etest/pkg/executor/executor.go

the observation is that for the command that returns instantly (whoami/ifconfig), it works just fine. It
does not work fine for the commands that will timeout/exit. I re-wrote the commit so it is clear that the above sentence is only valid for the specific type of commans (which is agnhost connect --timeout).

karampok · 2025-06-23T07:51:13Z

e2etest/netpoltests/networkpolicies.go

+	})
+})
+
+func expectedPolicies() ([]networkingv1.NetworkPolicy, error) {


karampok · 2025-06-23T07:51:56Z

e2etest/l2tests/interface_selector.go

@@ -25,7 +25,7 @@ import (
 	"go.universe.tf/e2etest/pkg/status"
 )

-const agnostImage = "registry.k8s.io/e2e-test-images/agnhost:2.45"
+const agnhostImage = "registry.k8s.io/e2e-test-images/agnhost:2.47"


removed it from the commit

github-actions bot added the kind/feature label May 26, 2025

karampok force-pushed the networkpolicies branch 6 times, most recently from 8e246aa to b518762 Compare June 2, 2025 07:33

github-actions bot added kind/feature and removed kind/feature labels Jun 2, 2025

karampok force-pushed the networkpolicies branch from b518762 to 2aabc87 Compare June 2, 2025 10:16

karampok changed the title ~~Add networkpolicies in the helm chart~~ Add networkpolicies Jun 2, 2025

github-actions bot added kind/feature and removed kind/feature labels Jun 2, 2025

karampok marked this pull request as ready for review June 2, 2025 11:18

karampok requested review from fedepaol and oribon as code owners June 2, 2025 11:18

karampok force-pushed the networkpolicies branch from 2aabc87 to 784f844 Compare June 3, 2025 07:15

oribon reviewed Jun 8, 2025

View reviewed changes

fedepaol reviewed Jun 9, 2025

View reviewed changes

karampok force-pushed the networkpolicies branch 3 times, most recently from a373369 to 2fb7825 Compare June 10, 2025 14:29

github-actions bot added kind/feature and removed kind/feature labels Jun 10, 2025

karampok force-pushed the networkpolicies branch from 2fb7825 to 278d884 Compare June 10, 2025 14:37

karampok force-pushed the networkpolicies branch 9 times, most recently from 42963bb to 956e4e8 Compare June 17, 2025 12:54

oribon reviewed Jun 18, 2025

View reviewed changes

karampok force-pushed the networkpolicies branch 3 times, most recently from 04d7aa8 to 1143368 Compare June 19, 2025 12:12

oribon reviewed Jun 19, 2025

View reviewed changes

karampok force-pushed the networkpolicies branch from 1143368 to 3a07712 Compare June 20, 2025 07:27

fedepaol reviewed Jun 20, 2025

View reviewed changes

karampok added 4 commits June 23, 2025 09:46

E2E: add network policy tests

af93654

Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>

Bump go toolchain to 1.24.4

a8b5f21

Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>

fixup! E2E: add network policy tests

5f8b003

Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>

karampok force-pushed the networkpolicies branch from 3a07712 to 5f8b003 Compare June 23, 2025 08:22

karampok commented Jun 23, 2025

View reviewed changes

		}

		return ret, wait.PollUntilContextTimeout(context.Background(), 1time.Second, 20time.Second, false, func(context.Context) (bool, error) {

Add networkpolicies #2773

Are you sure you want to change the base?

Add networkpolicies #2773

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment