Openstack: Fix cluster floating ips

[mitch000001]

This fixes an issue when we have multiple floating IPs which have the same virtual IPs but point to different networks.

[k8s-ci-robot]

Hi @mitch000001. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[zetaab]

Could you please tell the use-case for this? This whole if loop should handle loadbalancer ip addresses, NOT instance addresses. What is the thing that is not working currently? I am just worrying that this PR will break things like using old neutron lbaas. Also I am not sure does this work with octavia either

[mitch000001]

@zetaab The problem is that the current logic is connecting Floating IPs with Loadbalancers by their virtual IP. Having multiple networks with the same IP ranges in one project can lead to Loadbalancers having the same private IP they point to but within different networks. This fix mitigates that by using the Loadbalancer id stored within the FloatingIP

[mitch000001]

Having said that, InstanceID refers to different things than you would expect, i.e. when connecting a server to a FloatingIP it refers to the server ID, when connecting a LoadBalancer to the FloatingIP, it refers to its ID.

[mitch000001]

Another solution than the used one in this PR is to filter all LoadBalancers by their subnet/network to use the appropriate ones. I am also fine with that solution. In either way the current solution leads to bad side effects which we have to fix.

[mitch000001]

@zetaab btw: we are using neutron loadbalancers. For them, this change is working fine. How it will behave on octavia I cannot tell.

[zetaab]

ok I will test this tomorrow

[zetaab]

/ok-to-test

[zetaab]

this does not work for octavia loadbalancers:

% ./kops export kubecfg --name xjxjjx.k8s.local -v 2

I1217 13:13:43.469803   59537 factory.go:68] state store do://kopstest
I1217 13:13:44.400841   59537 cloud.go:354] authenticating to keystone
I1217 13:13:44.883163   59537 cloud.go:458] Openstack using Octavia lbaasv2 api
I1217 13:13:44.883226   59537 cloud.go:632] Querying Openstack to find Loadbalancers for API ("xjxjjx.k8s.local")
W1217 13:13:47.152999   59537 create_kubecfg.go:76] Did not find API endpoint for gossip hostname; may not be able to reach cluster
kops has set your kubectl context to xjxjjx.k8s.local

without this change I can find the hostname and everything works like should

Added debug for that:

W1217 13:13:47.152801   59537 cloud.go:648] DEBUG3 dbab1a38-faa5-49df-98bc-f638d057a96f lb-dbab1a38-faa5-49df-98bc-f638d057a96f

So when lb.ID is dbab1a38-faa5-49df-98bc-f638d057a96f the fip.InstanceID is lb-dbab1a38-faa5-49df-98bc-f638d057a96f so the floatingip instanceid contains prefix lb- which should be ignored in case of octavia

[mitch000001]

this does not work for octavia loadbalancers:
...
So when lb.ID is dbab1a38-faa5-49df-98bc-f638d057a96f the fip.InstanceID is lb-dbab1a38-faa5-49df-98bc-f638d057a96f so the floatingip instanceid contains prefix lb- which should be ignored in case of octavia

Given that fact I would propose another way of implementing it. The problem currently for us is that there are IP clashes between LoadBalancers. So if we add a filter for the loadbalancer to make sure it is originating from the proper subnet/network we should be able to leave the implementation with IP addresses as is.

[mitch000001]

Given that fact I would propose another way of implementing it. The problem currently for us is that there are IP clashes between LoadBalancers. So if we add a filter for the loadbalancer to make sure it is originating from the proper subnet/network we should be able to leave the implementation with IP addresses as is.

Thinking more about it the just proposed solution does not work, because we iterate over FloatingIPs and there is the problem of identification, not at the LoadBalancer side. So in oder to get it working we have to use the prefix strip approach, although it is not pleasing to me and feels brittle.

[mitch000001]

I will try to use parts of the implementation of https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/openstacktasks/floatingip.go#L85-L121 where we already connect LoadBalancers and FloatingIPs. Possibly a good idea to reuse knowledge.

[mitch000001]

/retest

[zetaab]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mitch000001, zetaab

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

The pull request process is described here

Needs approval from an approver in each of these files:

• upup/pkg/fi/cloudup/openstack/OWNERS [zetaab]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mitch000001]

/test pull-kops-verify-govet
/test pull-kops-verify-gofmt