Conversation
Commitlint-CheckThanks for your contribution ❤️ commitlint has detected that all commit messages in this PR follow the conventional commit format 🎉 |
Terraform-Check (version: 1.8.5): ✅🖌 Terraform Format: ✅⚙️ Terraform Init: ✅🤖 Terraform Validate: ✅ |
Terraform-Check (version: 1.9.8): ✅🖌 Terraform Format: ✅⚙️ Terraform Init: ✅🤖 Terraform Validate: ✅ |
This diff uses the upstream [PR](hcloud-talos/terraform-hcloud-talos#281) and enables encryption in cilium
mrclrchtr
left a comment
mrclrchtr
left a comment
There was a problem hiding this comment.
Please ensure correct conventional commit messages are used.
| { name = "l7proxy.enabled" | ||
| value = "true" | ||
| }, |
There was a problem hiding this comment.
Why do we need to do this? What are the consequences? How is backward compatibility?
| data "http" "gateway_api_crds" { | ||
| url = "https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.3.0/experimental-install.yaml" | ||
| } |
There was a problem hiding this comment.
Is it possible to use helm, like I used with the prometheus_operator_crds? I really don't want to use GitHub sources if possible.
There was a problem hiding this comment.
It doesn't seem to be on Helm yet. I think the options are:
- Pull from github
- Download the file and commit to this repository.
What do you think?
| #### Cilium Gateway API | ||
| To use Cilium as [Gateway API](https://gateway-api.sigs.k8s.io/) implementation, set `cilium_enable_gateway_api` | ||
| variable. This will install the required dependencies. When you configure a gateway later on, the Cilium operator | ||
| will provision an hcloud Load Balancer using Hcloud Controller Manager. | ||
| > [!IMPORTANT] | ||
| > This step will fail if the load balancer | ||
| > is not configured with the right [annotations](https://github.com/hetznercloud/hcloud-cloud-controller-manager/blob/main/docs/guides/load-balancer/quickstart.md) | ||
| > and the gateway will not provision endpoints. |
There was a problem hiding this comment.
If we really want to do that, then we should also provision and properly configure the load balancers in this module. Otherwise, it's too much of a dependency for me.
There was a problem hiding this comment.
This would be ideal but I am not sure if it's possible? Currently, load balancers are created by the Gateway which goes uses the HCCM CR to allocate a physical load balancer and set the correct downstream hosts. I don't see a way to assign an existing load balancer in the documentation. It also seems standard across clouds rather than Hetzner-specific.
I would also point out some benefits by keeping it this way:
- The load balancer lifecycle will be bound (automatically) to the gateway's lifecycle. Consider a complex case with separate infra and application teams. If the load balancer is provisioned outside the gateway, and the applications team removed the gateway, the load balancer will be left stale until someone remembers to remove it as opposed to being fully managed by the gateway.
- We can provision Cilium once and create as many load balancers and gateways later rather than having to figure out how many gateways we will create when provisioning infrastructure.
| - Installing preqreuisite CRDs https://docs.cilium.io/en/stable/network/servicemesh/gateway-api/gateway-api/ | ||
| - Installing tlsroute CRD | ||
| - Enabling Gateway API | ||
| This variable is incompatible with `cilium_values`. If the latter is set, this variable isn't used |
There was a problem hiding this comment.
Unfortunately, I can't find the code that contains that... why would that be?
There was a problem hiding this comment.
So this is the pre-existing behaviour — I haven't changed this part. We do this here
3869e94 to
66bdc97
Compare
Abstracts away gateway API configuration with a variable and installs required dependencies. NOTE: We install the experimental yaml as it includes tlsroute CRDs. This is required for the Gateway API regardless of whether it uses TLS or not. Test Plan ``` terraform validate ``` \+ Use it on my cluster and validate that gateway is correctly configured.
This diff adds important information about configuring the cilium API gateway. This isn't directly related to provisioning the cloud in terraform. However, it's key to making cilium gateway work with Hetzner and isn't documented on the internet.
|
Hey @mrclrchtr following up on this PR. Would be nice to converge. Let me know your thoughts. |
|
Hey @mrclrchtr could you look at this and decide one way or the other please? I need to pull changes from upstream and will have to rebase my changes so it would be nice to merge it to cut the toil a bit. |
|
I would love this to be added; it would also be nice to make Hubble configurable. @EssamEmad do you have a workaround for now? Maybe it's also nice to have a preconfigured way to enable ingress. And would love a way to expose the gateway using hostport when you don't need/have a load balancer. (When using single node, to minimize costs) |
3 participants
Context
The high-level intent behind this issue was to make it easy for a user of this library to use Cilium as Gateway API implementation. The suggestion was to allow customising cilium configuration without having to add a lot of boilerplate.
On further experimenting, this approach wouldn't scale well as:
Proposition
I think we should either make cilium configuration easily editable (as proposed in the initial issue) or abstract away features through variables. Given the above, this PR proposes the latter option and uses that to support Cilium Gateway API.
NOTE: We install the experimental yaml and not the main yaml as the former includes tlsroute CRDs which are required for the Gateway API regardless of whether it uses TLS or not.