search

digitalocean / digitalocean-cloud-controller-manager

Kubernetes cloud-controller-manager for DigitalOcean (beta)
Apache License 2.0
527 stars 149 forks source link

Support specifying custom load balancer names #298

Closed timoreimann closed 4 years ago

timoreimann commented 4 years ago

Load balancer names are based on Service object UIDs which consist of hard-to-read alphanumeric characters.

Some time ago, upstream provided the ability to specify custom names. We should take advantage of this and enable users to set their own names (through, say, an annotation).

One caveat to watch out for is that load balancer names must be unique. Consequently, we either need to surface any errors returning from the DO LoadBalancer API or ensure uniqueness on our end. The latter could be enforced by appending some kind of random hash (or one based off of the Service object UID).

The final piece to take into consideration are possible name length constraints, especially when thinking about adding a hash suffix.

grzesiek commented 4 years ago

+1

I started working on that in https://github.com/digitalocean/digitalocean-cloud-controller-manager/pull/307

/cc @timoreimann

timoreimann commented 4 years ago

Hey @grzesiek , thanks for your PR! 🎉

Just wanted to let you know that I'm busy today but will try to find some time tomorrow to look into the change. Will get back to you as soon as I can.

grzesiek commented 4 years ago

Thanks for letting me know @timoreimann and no worries, this is not urgent :)

grzesiek commented 4 years ago

Okay, so we had a discussion in the pull request :arrow_right: https://github.com/digitalocean/digitalocean-cloud-controller-manager/pull/307 and more annotations seems to be not an option. For more details see https://github.com/digitalocean/digitalocean-cloud-controller-manager/pull/307#pullrequestreview-361093236.

I would like to keep the ball rolling, so I have a different proposal here. But before I delve into the proposal I would like to explicitly name problems I would like to solve here:

Problems

Mutating LoadBalancer resource

When a user creates a LoadBalancer resource, digitalocean-cloud-controller-manager mutates it by adding an annotation like load-balancer-id.

This is not perfect, because if you are using a tool that has configuration drift detection built in, like Terraform Kubernetes provider, there is always some amount of noise involved and sometimes this also might lead to problems with reconciliation when Terraform tries to remove annotations it does not recognize on every run. kubectl diff may detect this.

Using Load Balancers API

Creating a LoadBalancer k8s resource will result in a DigitalOcean Load Balancer being provisioned, but it is difficult to use DO's API to get details of this resource, because a name of the load balancer being provisioned is random and the load balancer ID is assigned when the LoadBalancer service gets created.

Using Terraform data "digitalocean_loadbalancer" can't be done without knowing the load balancer name.

Proposal

What do you think about that @timoreimann?

timoreimann commented 4 years ago

hey @grzesiek. First of all, thanks for your follow up proposal.

We used to rely on LB names exclusively to link LB resources to Services. This posed two issues:

  1. LBs can be renamed, so any user doing so via the cloud control panel or API breaks the link to its cluster. This has been a frequent source of confusion and troubles in the past, which motivated the addition of the LB ID in the first place.
  2. LBs can only be looked up by ID directly; for name-based lookups, the list of LBs must be enumerated (possibly exhaustively). Admittedly, this isn't as problematic as the first point but it adds complexity.

So a good question would be what a better place for the LB ID could be. We have been thinking about moving CCM to a CRD-based architecture so that tracking metadata such as the LB ID would not have to be maintained on the Service object. This could possibly address the concerns you mentioned, but the idea needs to be fleshed out more. And just to complete the picture, there's an ongoing upstream effort to change the way LBs are managed in Kubernetes.

I'm not super happy with the configuration drift we added by introducing the LB ID annotation, so I'd be happy to follow up on any of my thoughts above or new ones to address that matter. To me though, that's somewhat distinct from making LB names configurable: while they could serve to also eliminate configuration drift (though my concerns raised above pose a set of challenges), I'm inclined to treat that as a separate issue if we cannot leverage the LB name for that. The name still serves a more dedicated purpose, which is to make it easier for customers to find their LBs based on a human-readable name.

Happy to hear your thoughts.

grzesiek commented 4 years ago

@timoreimann Thanks for the reply!

We used to rely on LB names exclusively to link LB resources to Services

I've never said that we should use load balancer name to track a service / LB resource relationship. I merely think that mutating the service to do this might not be the best solution. I agree that using a LB ID is presumably still the best solution for now.

So a good question would be what a better place for the LB ID could be. We have been thinking about moving CCM to a CRD-based architecture so that tracking metadata such as the LB ID would not have to be maintained on the Service object.

This sounds interesting indeed! Please let me know when you know more about that, I would be happy to help here!

For the time being though, I wonder how can we proceed here. What would be the most reasonable iteration?

Do you think the way forward is introducing load-balancer-name annotation and working on surfacing possible errors to users?

timoreimann commented 4 years ago

@grzesiek

Do you think the way forward is introducing load-balancer-name annotation and working on surfacing possible errors to users?

Yep, I think that's what we should do. There isn't much needed to do with regards to surfacing the errors because the DO LB API will already reject create/update requests if a different LB in the account has the same name already. Feel free to update your existing PR accordingly.

timoreimann commented 4 years ago

And re: "finding a better place for the LB ID": I'll do some upstream digging to see where SIG Networking stands today with regards to the Ingress v2 effort as this would determine if moving to CRDs still made sense.

grzesiek commented 4 years ago

Thanks for a really productive discussion @timoreimann :pray: ! I will try to allocate some time this week to update my PR and test it :+1:

flyte commented 4 years ago

Using Terraform data "digitalocean_loadbalancer" can't be done without knowing the load balancer name.

Since having to use the service.beta.kubernetes.io/do-loadbalancer-hostname annotation to work around the bug in kube-proxy, the external IP of the load balancer is no longer provided to Kubernetes (kubernetes.digitalocean.com/load-balancer-ext-ip perhaps??). This means that Terraform isn't able to use that IP to create the DNS record for the domain. I thought a possible workaround for that would be to use data "digitalocean_loadbalancer" but indeed as mentioned here, there's no way to look up the DO LB since we don't have any programmatic access to its name from the data within Kubernetes. We're a little stuck at this point.

For me and other users coming across this issue, is there a current way of looking up the LB in Terraform using the information from the Kubernetes Service, since we can't set the LB name directly?

grzesiek commented 4 years ago

@flyte using kubernetes_service.my_load_balancer.load_balancer_ingress[0].ip worked for me when I tried it the last time :thinking:

flyte commented 4 years ago

Sadly that's necessarily no longer populated when using the service.beta.kubernetes.io/do-loadbalancer-hostname annotation.

timoreimann commented 4 years ago

@flyte the scheme to create an LB name today is very simple: it consists of the letter a concatenated with the Service UID. The latter is a field on the Service object.

Strictly speaking, the naming scheme is an implementation detail of Kubernetes. I don't think it would be changed easily though, at least not without a proper deprecation process.