-
Notifications
You must be signed in to change notification settings - Fork 3.4k
ingress: migrate Cilium Ingress controller to use the controller-runtime library #29327
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
ingress: migrate Cilium Ingress controller to use the controller-runtime library #29327
Conversation
4fe0214
to
502ed14
Compare
/test |
This commit introduces and registers a new Cilium Ingress reconciler based on the controller-runtime library. It replaces the current k8s informer based Cilium Ingress controller. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit deletes the k8s informer based Cilium Ingress controller. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Currently, the slim version of the k8s golang model gets used for the Cilium Ingress controller. The main goal of the slim model is to keep the memory footprint at a minimal level. This is especially important for big resources that occur often in a cluster. There's no actual reason to use the slim model in Cilium Ingress reconciler. Therefore, all the Ingress related helper functions are migrated from the slim to the "normal" model. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit restores the functions `regenerate` & `isEffectiveLoadBalancerModeDedicated` from the old Cilium Ingress controller. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit lays out the basic reconcile skeleton that should be implemented with the new Cilium Ingress reconciler. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit implements the creation and update of dependent k8s resources during a reconciliation. Shared loadbalancing mode: * Creation & update of the shared CiliumEnvoyConfig in the Cilium installation namespace The shared CiliumEnvoyConfig contains the aggregated config of all shared Ingress resources in the cluster. Dedicated loadbalancing mode: * Dedicated loadbalancer Service in the same namespace as the Ingress * Dedicated Endpoints in the same namespace as the Ingress * Deidcated CiliumEnvoyConfig in the same namespace as the Ingress Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit implements the cleanup of dependent resources that were created during previous reconciliation runs. Resources need to be cleaned-up in various scenarios: * Shared Ingress gets deleted * Loadbalancer Mode of Ingress gets changed (dedicated <-> shared) * Change of responsible ingress controller (IngressClass) In these cases the reconciler checks whether some resources need to be cleaned up or udpated (shared CEC). Note: resources (Service, Endpoints & CiliumEnvoyConfig) of dedicated Ingresses are deleted via K8s Garbage Collection (via OwnerReferences). Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit implements the update of the Ingress' loadbalancer status during reconciliation. It checks whether the related loadbalancer Service already has an IP assigned. If it's present, it writes the status of the Ingress accordingly. In case of a change in the Ingress controller ownership, the reconciler tries to reset the loadbalancer status of the reconciler. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit fixes the usage of ownerreferences. Shared loadbalancing mode: K8s OwnerReferences are only meant to reference resources in the same namespace. Therefore, the shared CiliumEnvoyConfig in the namespace of Cilium should not contain any OwnerReferences, as it would make use of non-supported cross-namespace references. In these cases the reconciler is responsible to cleanup/update the shared CiliumEnvoyConfig - and can not rely on K8s GC functionality. Dedicated loadbalancing mode: The related resources Service, Endpoints & CiliumEnvoyConfig should contain only one OwnerReference to the Ingress resource. This reference should be marked as controlled by the Cilium Ingress controller. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
502ed14
to
a016ead
Compare
rebased to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work ✔️.
I have manually verified for scenarios, which are not covered in CI:
- Toggle load balancer mode between dedicated and shared
- Toggle default Ingress class
- TLS Ingress Passthrough
- Non-default ingress controller params (e.g. enforceHTTPs as false, useProxyProtocol, etc)
All cases seem behaved as what we expect. So just a couple of comments as per below.
Currently, the presence of an OwnerReference referencing an Ingress or Gateway (Gateway API) resource is the indicator whether Ciliums BPF metadata listener filter in Envoy should be configured to use the original source address or not. Referencing an Ingress or Gateway: don't use original source address Not referencing an Ingress or Gateway: use original source address With the removal of the OwnerReferences on the shared CiliumEnvoyConfig in case of a shared Cilium Ingress, this check no longer works as expected. Therefore, this commit introduces an explicit internal annotation `cilium.io/use-original-source-address` on the CiliumEnvoyConfig. Usages like the Cilium Ingress controller can set this annotation accordingly. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit replaces the package logger with an injected logger by the Hive framework. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit renames and moves the Ingress related constants to the corresponding annotation package. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Ingress reconciliation needs to be triggered whenever there's a change on the shared Ingress CiliumEnvoyConfig in the namespace of the Cilium installation. Therefore, this commit configures a respective watch. To prevent an unnecessaray reconciliation of all shared Ingresses (as they all would try to re-write the same shared CiliumEnvoyConfig), only a pseudo ingress gets enqueued. This way all cases should be covered (e.g. manual deletion of the shared CilumEnvoyConfig). Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Whenever there's a change on the default ingress-controller annotation on the cilium IngressClass (`ingressclass.kubernetes.io/is-default-class`), the relevant Ingresses need to be reconciled. There might be the possibility that Cilium is no longer the Ingress controller for the Ingresses that don't have an explicit controller set via (legacy) annotation or `ingressClassName`. Therefore, this commit registers a corresponding watch on the cilium IngressClass. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit optimizes the watch on the Ingress resources to only watch for Ingresses that are managed by Cilium. This includes the Ingresses that were managed by Cilium before the change (e.g. change of the field `ingressClassName`). This is necessary to cleanup potentially existing resources from previous reconciliations. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit refactores the existing regeneration logic by introducing explicit functions to build the model for the shared and dedicated loadbalancing case. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit adds unit tests for the Ingress helper & reconciler. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
a016ead
to
45f56df
Compare
@sayboras Addressed and answered your feedback! Thanks a lot! PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work ✔️
/test |
@mhofstetter Technically, Cilium Ingress does not use the original IP address, but the Ingress IP address. So the annotation might need to be renamed as |
I named the label after the property in the Cilium BPFMetadata Listener filter and it's usage in the CiliumEnvoyConfig - and not after the usage by Ingress & Gateway API. Therefore, in case of Ingress it gets set to |
OK, I missed the inversion of the logic here :-) |
This is to align with Ingress implementation Relates: cilium#29327 Fixes: cilium#28949 Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is to align with Ingress implementation Relates: cilium#29327 Fixes: cilium#28949 Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is to align with Ingress implementation Relates: cilium#29327 Fixes: cilium#28949 Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Tam Mach <tam.mach@cilium.io>
This PR migrates the Cilium Ingress controller to use the controller-runtime library instead of bare k8s informers.
Please review the individual commits.
Follow up of: #29198
Fixes: #28911