Handle iptables cleanup on uninstall #211
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 211 in repo Azure/aad-pod-identity |
| _, err := cmd.CombinedOutput() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| time.Sleep(90 * time.Second) |
There was a problem hiding this comment.
Added 90s wait time because it's possible nmi pods are just being created. It takes 60s after the pod startup before the iptables entries are added since the ticker is configured with 60s update duration. We might need to change this to create the ticker with instant first tick to prevent any race conditions.
There was a problem hiding this comment.
Could we add a check to ensure that the rules have been indeed added instead of the sleep to make it more deterministic. Perhaps in a future PR, but could make our tests more deterministic ?
There was a problem hiding this comment.
That makes sense. I'll open an issue to track that change.
| hostNetwork: true | ||
| containers: | ||
| - name: busybox | ||
| image: alpine:3.8 |
There was a problem hiding this comment.
just wanted to confirm that its the newest version ?
There was a problem hiding this comment.
The latest version is alpine:3.9.4. I kept it at 3.8 because that is the version we are using to build other images.
| image: alpine:3.8 | ||
| stdin: true | ||
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
do we need privileged for this ? Isn't hostNetwork: true enough to check the iptables existence ?
There was a problem hiding this comment.
Yeah, without privileged the iptables table won't initialize.
resolves #113
This will handle graceful shutdown of the pod, cleanup the ip table chain and rules that were inserted by NMI.