Introduce an easy way to limit access to k8s api when ManagedAPIServerLoadBalancer: true

nikParasyr commented 2 years ago

/kind feature

Hello,

I find having CAPO managing the ApiServerLB a very good feature that simplifies a lot of stuff in our case. That being said the k8s api is open from everywhere which is not ideal in our case. Currently the only way to have the ApiServerLB with limited access is to create it yourself, which means also creating network, subnet, router, pool etc etc which is a bit of extra work and adds an additional step in creation and deletion of clusters. I'd like to have a way to limit access to the ApiServer without having to build it myself. I'm not sure if this is something you think that CAPO should support but im mentioning some potential approaches below.

Describe the solution you'd like Possible approaches:

User can define a list of strings with allowed CIDRs on OpenStackCluster. CAPO creates an SG with these CIDRs and attaches it to the LB vip port after creation. This requires the least work from user perspective but adds quite extra complexity on CAPO.
User can define a list of strings with securityGroup IDs on OpenStackCluster. CAPO associates these SGs to the LB vip port after creation. A bit more work for the user, a bit less for CAPO.
User can define LBPortID on OpenstackCluster. CAPO creates an LB that uses that port as the vip. This is possible only for octavia (not sure if LBaaS is supported instead of octavia). This adds quite some work for the user as he needs to create network,subnet, router, a port and his own SGs on it but is much easier for CAPO.
User can define a list with allowed CIDRs on OpenStackCluster. CAPO adds the allowedCIDRs to the Listeners it creates on the LB. Only possible for octavia > 2.12 ( I think its Stein but i'll have to doublecheck).

Anything else you would like to add:

Let me know if this is something that should be added and if so which approach should be taken.
I'd like to work on this one

mdbooth commented 2 years ago

@iamemilio thoughts? (Although this doesn't currently affect OpenShift)

My initial thought is that option 2 above would be preferable. My concern about option 1 is that it's potentially too simplistic. Potential use cases I can think of:

Source CIDR
IPv4 vs IPv6
Target protocol and port (e.g. restricting external access to k8s by http version)

However, if we were confident that we could cover all reasonable use cases without too much complexity it might be worth considering.

jichenjc commented 2 years ago

User can define a list with allowed CIDRs on OpenStackCluster.

conceptually this seems reasonable as OpenStackCluster is the entity define the cluster itself which means API block/allow list belongs to it well suited. Stein is a pretty old version so I think we are good enough to add such requirement (maybe tolerance on Stein- version to say it's not supported but not block)

not sure if LBaaS is supported instead of octavia

Octavia is the future and I am not sure we even tested LBaas at all

hidekazuna commented 2 years ago

@nikParasyr Neutron LBaaS v2 support was already dropped: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/813

nikParasyr commented 2 years ago

Thanks for the feedback :)

I see that option 2 and 4 are mentioned, which are also the ones that make more sense. There are some differences between the two approaches. Both of them should be rather easy to implement and maintain. I'd like some more feedback in regards with their differences mentioned below, before i start implementing this.

case 4, "add a allowedCidrs to listener.CreateOpts": This requires octavia 2.12. From the feedback this doesn't seem to be an issue necessarily and it can be added in a way that when it's not defined it will not add it, so users with lower octavia versions can keep using CAPO, by not setting this option. Functionally, this translates to HAproxy white/blacklist on the default octavia provider(amphora). I'm not sure how and if it works with other octavia providers. I don't know how much CAPO is worried about different providers or it sticks with "vanilla" openstack.
case 2, "associate a list of user created SGs to LBs vip_port after creation": This should work regardless of octavia version and/or provider. Also functionally it is a bit different as packages are dropped by neutron before they even reach the LB, in some sense this can be viewed as "stronger"/"better".

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

nikParasyr commented 2 years ago

/remove-lifecycle stale

bavarianbidi commented 2 years ago

@nikParasyr as the current occm implementation is doing case 4 (https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/openstack/loadbalancer.go#L1467-L1470) i would prefer this implementation as the underlying openstack objects (CAPO managed LB and OCCM managed LB) behave/look more or less the same. WDYT?

are you still interested in the implementation or is this something we could take over from you?

nikParasyr commented 2 years ago

@bavarianbidi hello.

@nikParasyr as the current occm implementation is doing case 4 (https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/openstack/loadbalancer.go#L1467-L1470) i would prefer this implementation as the underlying openstack objects (CAPO managed LB and OCCM managed LB) behave/look more or less the same. WDYT?

i also think that case 4 is the way to go forward. it's the official vanilla openstack way of doing it.

Couple of notes/thoughts i had:

we should document that the cidr of the management cluster should be included in the list, as well as the cidr of the external network that the workload cluster runs on.
not sure if having this as a immutable field will be "restrictive" during the lifetime of a cluster

are you still interested in the implementation or is this something we could take over from you?

Unfortunately I haven't had much time to implement it and i don't see this changing the coming couple of months, so it would be really nice if you can pick it up :+1:

bavarianbidi commented 2 years ago

hey @nikParasyr i will work on it.

This are my first thoughts about it.

`spec`

adding a List to the APIServerLoadBalancer struct

type APIServerLoadBalancer struct {
    // Enabled defines whether a LoadBalancer should be created.
    Enabled bool `json:"enabled,omitempty"`
    // AdditionalPorts adds additional tcp ports to the Loadbalacner
    AdditionalPorts []int `json:"additionalPorts,omitempty"`
+++ // add allowed CIDRs
+++ AllowedCIDRs []string `json:"allowedCidrs,omitempty"`
}

implementation

questions/thoughts

beside `6443` it's possible to define multiple listeners via `additionalPorts`

should we apply the firewall policy to all listeners for now or only to the api server? Is it a valid use-case to have different allowedCirds per listener (future impl.)

discovery

How much "auto discovery" is needed/valid? During cluster-creation a user only knows a few IPs (e.g. IP of a central bastion host). Is it safe to get the router ip from the external_gateway_info field of a router (CAPO cluster and target cluster) if exists or should a admin/user know/define these IPs by it's own (as there are many OS deployments where the router-ip can't be found in the external_gateway_info field)

If we have the IP auto discovery, should we write the additional IPs back into the Spec or does a new status field make sense

Update 2022-04-25:

For now i will continue by adding a new status field which holds the information about the router/NAT IP of the target cluster (could be easily fetched). Discovering the IP of the management Cluster is much more trickier than i initially thought:

management cluster can run somewhere else, where it's not that easy to detect the outgoing router/NAT IP address
- other cloud provider
- running on a dedicate installation
even if the management cluster is self managed by CAPO, we have to define somewhere, what's the cluster name of the management cluster to either talk to the OpenstackAPI for this specifc tenant or going down the resources of this cluster to get the outgoing IP address.

immutable

I would recommend to have this field not immutable as the IP restriction might change over the lifetime of an openstack cluster.

cc: @chrischdi / @tobiasgiese / @seanschneeweiss as you might also have an opinion about this

chrischdi commented 2 years ago

One thing about wether writing back to spec or status: IMHO writing back to spec is a bad pattern and would break/not work when using ClusterClass. CAPA already encountered an issue there if I remember correctly.

Having a field in status too may be good to prevent unnecessary API calls so the controller would be able to do the diff and only do calls to Openstack if there had been a change to the Cr itself.

kubernetes-sigs / cluster-api-provider-openstack