[octavia-ingress-controller] Add annotations to keep floating IP and/or specify an existing floating IP

[ccleouf66]

What this PR does / why we need it:

Add features:

• allow to keep floating IP when delete ingress
• allow to use an existing available floating IP

Which issue this PR fixes(if applicable):
none

Special notes for reviewers:
The doc is also updated to indicated how to.
During the ingress creation only add two annotations inspired from openstack-cloud-controller:

annotations:
  octavia.ingress.kubernetes.io/keep-floatingip: "true"        # floating ip will not be deleted when ingress is deleted
  octavia.ingress.kubernetes.io/floatingip: "122.112.219.229"  # define the floating to use 

keep-floatingip: default value false
If octavia.ingress.kubernetes.io/floatingip is sepcified and an error occurred (fip not available, not exist, not valid) fallback to create new fip.

Release note:

NONE

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ccleouf66 / name: Cyril Connan (abb3f2b, 66ee273, 2a19622, a7c4dfe, 4780e2e, 63fdc55, 78a4b18, 9b9048b, 51a06c5, 26fbe60, 7d4f5f0)

[k8s-ci-robot]

Welcome @ccleouf66!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @ccleouf66. 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.

[ccleouf66]

/easycla

[jichenjc]

/ok-to-test

[dulek]

Some tiny remarks inline.

I wonder if we could have approached this as we do in the controller for Services. There we don't need the keep-ip annotation because we simply check the description of the FIP and in case it's not created by us, we do not delete it.

This would make the cloud provider more consistent, question is - isn't using implicit annotation better?

[dulek]

Some tiny remarks inline.

I wonder if we could have approached this as we do in the controller for Services. There we don't need the keep-ip annotation because we simply check the description of the FIP and in case it's not created by us, we do not delete it.

This would make the cloud provider more consistent, question is - isn't using implicit annotation better?

Thinking about this more now - if we have two separate annotations now (floatingip and keep-floatingip), we have to consider if it makes sense to use them separately. An Ingress with just floatingip annotation would expect the FIP to exist beforehand and then it will remove that FIP on deletion. Then before it's recreated (K8s reconciliation pattern can trigger that automatically) it would need the FIP to be recreated by the user. This just doesn't fit K8s model.

Then using just keep-floatingip annotation would make the FIPs orphaned on every Ingress recreation. This again doesn't fit K8s model well, when resources might be recreated fairly often and without consequences.

IMO that's a strong argument to just implement the floatingip annotation and instead of relying on keep-floatingip annotation, just check the FIPs description to see if it was created by the Ingress controller or not. See a similar thing in CCM [1].

I now see that CCM has a keep-floatingip annotation support too. I don't think that's a good idea in general, unless there's a use case I don't see.

[1]

cloud-provider-openstack/pkg/openstack/loadbalancer.go

Lines 2335 to 2339 in 2231e34

	klog.InfoS("Matching floating IP", "floatingIP", fip.FloatingIP, "description", fip.Description)
	matched, err := regexp.Match("Floating IP for Kubernetes external service", []byte(fip.Description))
	if err != nil {
	return err
	}

[ccleouf66]

Okay thanks for precision ! The use case I have in mind is: when you are running a kube cluster on publuci cloud provider based on openstack like OVH or t-system, the first time when you create your ingress, you don't have yet an existing FIP (except if you created it manually before) so you probably want to let your Cloud provider to create it for you.

Now, when you delete your ingress, maybe you want to keep the FIP to not have to recreate DNS record for example and having place the keep annotations can allow you to do that.

You probably right, their is a better implementation possible, juste for the moment I don't have it.

[dulek]

I think that at a minimum I'd add that part about checking if description indicates if FIP was created by Ingress controller before deleting it like it's done in CCM. Then keep-floatingip annotation has a precedent in CCM, so I guess it can be added here too. Thing is - it's much easier to add a new functional annotation than to remove it later, so I'm trying to consider all option.

[dulek]

I added a bunch of comments regarding the logic around this. I do not think this should attempt to create a random FIP if user-specified one doesn't exist. If one is attempted to be created it has to request a user-requested address too (even if it's admin API).

[ccleouf66]

Thanks @dulek, I updated following your comments, let me know if it's ok for you.

[dulek]

The logic seems to make sense now, but the code of EnsureFloatingIP() is structured weirdly. IMO it should:

1. Get list of FIPs attached to the VIP port.
2. if needDelete.
   1. Delete them and return.
3. Check if len(fips) > 1, return error if so.
4. if existingFloatingIP == "":
   1. if len(fips) == 1 return success.
   2. else
      1. Create new FIP and save it into fip
5. else:
   1. if len(fips) == 1 check if attached FIP has that address and network. If so return success. If not, return error. We might consider if here we shouldn't detach that FIP instead of returning error.
   2. else find FIP by address, check if it's free. If not return error. Save it to fip.
6. Attach fip to the VIP port.

[ccleouf66]

The logic seems to make sense now, but the code of EnsureFloatingIP() is structured weirdly. IMO it should:

1. Get list of FIPs attached to the VIP port.

2. if needDelete.

   1. Delete them and return.

3. Check if len(fips) > 1, return error if so.

4. if existingFloatingIP == "":

   1. if len(fips) == 1 return success.

   2. else

      1. Create new FIP and save it into fip

5. else:

   1. if len(fips) == 1 check if attached FIP has that address and network. If so return success. If not, return error. We might consider if here we shouldn't detach that FIP instead of returning error.

   2. else find FIP by address, check if it's free. If not return error. Save it to fip.

6. Attach fip to the VIP port.

I let you validate and test or need something else ?

[kayrus]

I'm working on #2377 and I think it makes sense to wait until it's merged and then try to sync/reuse the logic in this PR.

[ccleouf66]

I'm working on #2377 and I think it makes sense to wait until it's merged and then try to sync/reuse the logic in this PR.

Ok, thanks for update :)

[AlexonOliveiraRH]

May we get this merged? I have a scenario where this could be very suitable.

[dulek]

I think this looks good now. @kayrus, do you think we can merge this now? Please note that this modifies how Octavia Ingress Controller handles LBs, so #2377 shouldn't affect this?

/lgtm

[jichenjc]

is this true? maybe the create floating ip can fail?

[ccleouf66]

You right, the error message is not correct the error don't cover the creation of a new fip.
That is what you are pointing ?

[jichenjc]

cloud-provider-openstack/pkg/openstack/loadbalancer.go

Lines 561 to 581 in 35ff405

	func (lbaas *LbaasV2) createFloatingIP(msg string, floatIPOpts floatingips.CreateOpts) (*floatingips.FloatingIP, error) {
	klog.V(4).Infof("%s floating ip with opts %+v", msg, floatIPOpts)
	mc := metrics.NewMetricContext("floating_ip", "create")
	floatIP, err := floatingips.Create(lbaas.network, floatIPOpts).Extract()
	err = PreserveGopherError(err)
	if mc.ObserveRequest(err) != nil {
	return floatIP, fmt.Errorf("error creating LB floatingip: %v", err)
	}
	return floatIP, err
	}
	func (lbaas *LbaasV2) updateFloatingIP(floatingip *floatingips.FloatingIP, portID *string) (*floatingips.FloatingIP, error) {
	floatUpdateOpts := floatingips.UpdateOpts{
	PortID: portID,
	}
	if portID != nil {
	klog.V(4).Infof("Attaching floating ip %q to loadbalancer port %q", floatingip.FloatingIP, *portID)
	} else {
	klog.V(4).Infof("Detaching floating ip %q from port %q", floatingip.FloatingIP, floatingip.PortID)
	}
	mc := metrics.NewMetricContext("floating_ip", "update")

should we reuse those functions?

[dulek]

I'd vote to not reuse them, just because these are separate controllers with separate codepaths and maintaining them through two LB controllers might be cumbersome.

[ccleouf66]

So we duplicate this function and add it in this project ?

[jouvin]

I just started with Octavia ingress controller and faced the problem adressed by this issue: use/reuse an externally defined FIP to ensure that a service is exposed with a static address. Any chance to see this feature released soon?

[dulek]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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:

• OWNERS [dulek]

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