auto egress IP fixes

[danwinship]

Some fixes to the auto egress IP code

[danwinship]

From @pravisankar 's too-late comments on #16561:

omitempty?

fixed

Do we expect egress IP to work for IPV6 as well? If not, may be we should add ipv4 check here and in netnamespace.EgressIPs validation.

No... but we aren't requiring IPv4 in most of the other places we only support IPv4 either. I'll file a bug about that.

We already know the nodeIP (map key), don't need to duplicate in the map value
nodesByNodeIP map[string]*sets.String ?

I guess technically it would work to have one map from node IP to (just) egress IP, and a second map from egress IP to (just) node IP, but I prefer it with the struct, which makes it more obviously parallel to the namespace maps, where we do need the structs

Since this method will not return an error may be this method can have no return value.

Yeah... I was going to fix that, but then I didn't. It seems like it could return an error potentially in the future...

If assignedIP is already set, then drop the traffic for this namespace?

Because of previous checks, it's guaranteed that it's either unset, or already this IP.

When namespace is deleted, where are we deleting vnid from namespacesByVNID map?

Fixed

Failed link may not have desired IP address, we could log an error and continue

Fixed

We should not call this for single-tenant/subnet plugin as it depends on NetNamespaces. I think, we should move this to node.policy.StartEgressIP?

Fixed

[danwinship]

Do we expect egress IP to work for IPV6 as well? If not, may be we should add ipv4 check here and in netnamespace.EgressIPs validation.

No... but we aren't requiring IPv4 in most of the other places we only support IPv4 either. I'll file a bug about that.

#16660

[danwinship]

@eparis need an approval from you for "API changes" (I just belatedly marked the new EgressIP fields +optional.)

[smarterclayton]

Is there an egress extended test?

[danwinship]

No, but there are some unit tests.

I guess it would be possible to test in the extended tests, given that pod-to-(public)-node-IP traffic is considered external, so we could run a server in a hostNetwork=true pod on one node and host an egress IP on another...

[pravisankar]

LGTM

[smarterclayton]

Something we can queue up for 3.8, but i'd like to have at least one extended test for all features that runs in reasonable time. GCE runs with subnet today.

…

On Tue, Oct 3, 2017 at 12:37 PM, Dan Winship ***@***.***> wrote: No, but there are some unit tests. I guess it would be possible to test in the extended tests, given that pod-to-(public)-node-IP traffic is considered external, so we could run a server in a hostNetwork=true pod on one node and host an egress IP on another... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#16659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4YdS-t1WEXhfFA0gD1-wEl73pyqks5somLHgaJpZM4PsT6w> .

[eparis]

/retest
/lgtm

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [eparis]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-merge-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[openshift-merge-robot]

Automatic merge from submit-queue (batch tested with PRs 16625, 16659).

[openshift-ci-robot]

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	e08e9eb	link	/test extended_conformance_install_update

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.