[feat] - add support for cilium-backed loadbalancers #208
Conversation
AshleyDumaine
force-pushed
the
shared-ip
branch
13 times, most recently
from
96cf0c0 to
269bca3
Compare
|
|
||
| ipHolder, err = l.client.CreateInstance(ctx, linodego.InstanceCreateOptions{ | ||
| Region: l.zone, | ||
| Type: "g6-nanode-1", |
There was a problem hiding this comment.
nit: can we have this configurable via a variable? Though plans don't change frequently, just wondering if it does get updated in future to g7, we can just set the variable than changing and building again. Same for image.
There was a problem hiding this comment.
This is a temporary workaround while there's no way to request IPs that are not attached to a Linode. I'm hoping that changes by the time there is a g7 plan or ubuntu 22.04 goes EOL 😳
| # sharedIPLoadBalancing: | ||
| # defaultLoadBalancerClass: <linode-nodebalancer or io.cilium/bgp-control-plane> | ||
| # bgpNodeLabel: <node label for CiliumBGPPeeringPolicy> | ||
|
|
There was a problem hiding this comment.
nit: can we also add some documentation regarding these flags in readme?
| AddInstanceIPAddress(ctx context.Context, linodeID int, public bool) (*linodego.InstanceIP, error) | ||
| DeleteInstanceIPAddress(ctx context.Context, linodeID int, ipAddress string) error | ||
|
|
||
| ShareIPAddresses(ctx context.Context, opts linodego.IPAddressesShareOptions) error |
There was a problem hiding this comment.
nit: we can remove parameter names and just keep parameter types to keep it consistent with other lines in file. There are some lines which still have both parameter name and types, but in general I see folks trying to just keep parameter types.
There was a problem hiding this comment.
If we want to keep it consistent, I would prefer adding parameter names for clarity
There was a problem hiding this comment.
Make it consistent! By converging on my standard...
I love it :D
AshleyDumaine
force-pushed
the
shared-ip
branch
3 times, most recently
from
51e2f6e to
bd542af
Compare
AshleyDumaine
force-pushed
the
shared-ip
branch
2 times, most recently
from
c2a6c94 to
ecf72ad
Compare
| // These values come from https://www.linode.com/docs/products/compute/compute-instances/guides/failover/#ip-sharing-availability | ||
| var ( | ||
| regionIDMap = map[string]int{ | ||
| "us-southeast": 4, // Atlanta, GA (USA) |
There was a problem hiding this comment.
can we move this to a configmap as a followup?
There was a problem hiding this comment.
How would a configmap help? I think we'd still need to cut a new release if we did add/remove a DC, it would just be a difference of if the image version changes vs the k8s resources
There was a problem hiding this comment.
I guess i was thinking more from a internal customer pov - modifying a configmap in place vs picking up a new version - but you're right, doesn't give us much
README.md
Outdated
| - image: linode/linode-cloud-controller-manager:latest | ||
| name: ccm-linode | ||
| args: | ||
| - --leader-elect-resource-lock=endpoints |
There was a problem hiding this comment.
nit: example args needs an update as they have changed recently.
There was a problem hiding this comment.
updated to just include the args that need to be added for the feature
…ud-provider wanting no loadBalancerClass set
This PR is to support cases where the linode-CCM is installed but instead of backing a LoadBalancer Service via NodeBalancers, the user wants to leverage Cilium's BGP load balancing with automatically requested and assigned shared IPs.
General:
Pull Request Guidelines: