Require a label to indicate ip-masq-agent readiness. #47764
Conversation
a daemonset running on nodes where the master is 1.7 and has this enabled by default, however, the nodes are still < 1.7.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/XS
|
/assign bowei |
|
/assign @bowei |
|
/assign @dchen1107 |
|
After this pr, shouldn't some tests in v1.7 requiring ip-masq-agent running by default will fail? Otherwise, I am fine with this change. |
|
Good point. There is a no-snat test. It looks like it doesn't use the yaml. It hard codes the image. Running that test now to make sure this doesn't break it. |
|
/lgtm I am approving this to stop the bleeding edge first. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, dnardo Associated issue: 47752 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
Adding do-not-merge because the release note process has not been followed. |
k8s-github-robot
added
the
do-not-merge
dchen1107
added
release-note-none
|
I am manually merging this one to fix upgrade tests. |
|
I suspect this broke all GKE test runs. |
|
It looks like this broke pod egress to any public IP in both GCE and GKE. Should a e2e test run in our prepull suite have caught this? |
| @@ -33,6 +33,8 @@ spec: | |||
| volumeMounts: | |||
| - name: config | |||
| mountPath: /etc/config | |||
| nodeSelector: | |||
| beta.kubernetes.io/masq-agent-ds-ready: "true" | |||
There was a problem hiding this comment.
should this label be added to 1.7 nodes?
There was a problem hiding this comment.
But I don't see that it was added to https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/config-test.sh
There was a problem hiding this comment.
Right but if those env vars are set in GKE that would also handle this.
Yesterday a PR that set NON_MASQUERADE_CIDR=0.0.0.0/0 in the gce scripts was reverted. Since that was reverted the non-masq-cidr will get set to 10/8. If that is the case the the ip-masq-agent won't even run and the kubelet would take care of the masquerading. So I think that whether those labels are set here doesn't matter.
I think what is needed now is that we set gce back to using NON_MASQUERADE_CIDR=0.0.0.0/0 and we add the labels.
There was a problem hiding this comment.
yeah, I asked #47764 (comment) which assume ip-masq-agent won't run since there is no node marking with the newly introduced label. But on my side, I was confused that NON_MASQUERADE_CIDR=0.0.0.0/0 is another label on Kubelet which tells Kubelet to stop masq-ing. :-( @mikedanese Thanks for spot the issue.
This prevents a DaemonSet from running on nodes where the master is 1.7 and has this
enabled by default, however, the nodes are still < 1.7.
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes ##47752
Special notes for your reviewer:
Release note: