New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metallb does not deallocate a Service's IP on Service type change to non-LB #190

Closed
rsanders opened this Issue Mar 10, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@rsanders
Copy link

rsanders commented Mar 10, 2018

Is this a bug report or a feature request?:

Bug report.

What happened:

Changing the type of a Service with a metallb-assigned IP from LoadBalancer to NodePort does not appear to release the IP allocation to revoke access from outside the cluster. Instead, the Service remains reachable on the metallb-allocated IP address on the Service's port.

What you expected to happen:

I expected the IP address to be returned to the pool; reachability of the Service to be lost; and the status field of the Service to be updated to remove the ingress IP address.

How to reproduce it (as minimally and precisely as possible):

Setup:

metal v0.4.3 running a K8S 1.8.6 cluster. Create a NodePort service. Edit the type to be LoadBalancer. metallb should assign an IP. Test connectivity and check the status field for the assigned IP.

After that has been successful, edit the Service and change type back to NodePort. The allocation will not appear to change, and in my case I was still able to connect to the service on its defined port using the IP address allocated by metallb.

Anything else we need to know?:

No.

Environment:

  • MetalLB version: 0.4.3
  • Kubernetes version: 1.8.6
  • BGP router type/version: using ARP mode
  • OS (e.g. from /etc/os-release): CentOS 7.4 (except for kernel)
  • Kernel (e.g. uname -a): 4.14.12-1.el7.elrepo.x86_64

@danderson danderson added the bug label Mar 10, 2018

@danderson danderson self-assigned this Mar 10, 2018

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Mar 10, 2018

Thanks for the report!

I see where the bug is in the code, I'll cut 0.4.4 with a fix shortly.

I'm surprised this can happen, because when I was developing MetalLB on k8s 1.8, I think I was unable to change the type of a Service after creation... So the codepath just assumes that if something is not a LoadBalancer, then it never was a LoadBalancer and we don't need to run the deletion logic. Oops!

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Mar 10, 2018

And out of pure brilliance, I cut 0.4.4 before actually testing my change, and wouldn't you know it, it's broken!

Fixing (and testing before cutting the release this time...), 0.4.5 will have the right stuff.

danderson added a commit that referenced this issue Mar 10, 2018

Call deleteBalancer for services of the wrong type. #190
Turns out, you _can_ convert a service from type LoadBalancer to
something else, whereas I thought that the type was immutable after
creation. Oops!

@danderson danderson closed this in 0574519 Mar 10, 2018

danderson added a commit that referenced this issue Mar 10, 2018

Call deleteBalancer for services of the wrong type. #190
Turns out, you _can_ convert a service from type LoadBalancer to
something else, whereas I thought that the type was immutable after
creation. Oops!

(cherry picked from commit 886a19c)

danderson added a commit that referenced this issue Mar 10, 2018

Actually fix #190.
We can't just delete internal state when an actively managed object
becomes unmanaged, we have to run through the convergence logic to
push the cleared state back up into k8s.

(cherry picked from commit 0574519)

@danderson danderson reopened this Mar 10, 2018

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Mar 10, 2018

0.4.5 released, with a fix for this. Thank you again for the clear bug report!

@danderson danderson closed this Mar 10, 2018

@rsanders

This comment has been minimized.

Copy link

rsanders commented Mar 12, 2018

Thank you for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment