Skip to content

Conversation

mhofstetter
Copy link
Member

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

@mhofstetter mhofstetter added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-ingress labels Nov 22, 2023
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/ingress-controller-runtime branch 3 times, most recently from 4fe0214 to 502ed14 Compare November 22, 2023 15:03
@mhofstetter
Copy link
Member Author

/test

@mhofstetter mhofstetter marked this pull request as ready for review November 22, 2023 16:39
@mhofstetter mhofstetter requested review from a team as code owners November 22, 2023 16:39
@mhofstetter mhofstetter requested a review from sayboras November 22, 2023 16:39
@mhofstetter mhofstetter added area/operator Impacts the cilium-operator component dont-merge/blocked Another PR must be merged before this one. labels Nov 22, 2023
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>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/ingress-controller-runtime branch from 502ed14 to a016ead Compare November 27, 2023 07:30
@mhofstetter mhofstetter removed the dont-merge/blocked Another PR must be merged before this one. label Nov 27, 2023
@mhofstetter
Copy link
Member Author

rebased to main to resolve conflicts

Copy link
Member

@sayboras sayboras left a 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>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/ingress-controller-runtime branch from a016ead to 45f56df Compare November 27, 2023 13:38
@mhofstetter
Copy link
Member Author

@sayboras Addressed and answered your feedback! Thanks a lot! PTAL

@mhofstetter mhofstetter requested a review from sayboras November 27, 2023 13:40
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work ✔️

@mhofstetter
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 28, 2023
@sayboras sayboras added this pull request to the merge queue Nov 28, 2023
Merged via the queue into cilium:main with commit 8a1e067 Nov 28, 2023
@mhofstetter mhofstetter deleted the pr/mhofstetter/ingress-controller-runtime branch November 28, 2023 13:11
@jrajahalme
Copy link
Member

@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 cilium.io/use-ingress-source-address?

@mhofstetter
Copy link
Member Author

@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 cilium.io/use-ingress-source-address?

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 false. (Invertion of some logic was necessary) But IMO i think this is more logical - especially if other use-cases would make use of it.

@jrajahalme
Copy link
Member

@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 cilium.io/use-ingress-source-address?

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 false. (Invertion of some logic was necessary) But IMO i think this is more logical - especially if other use-cases would make use of it.

OK, I missed the inversion of the logic here :-)

sayboras added a commit to sayboras/cilium that referenced this pull request Dec 1, 2023
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>
sayboras added a commit to sayboras/cilium that referenced this pull request Dec 1, 2023
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>
github-merge-queue bot pushed a commit that referenced this pull request Dec 4, 2023
This is to align with Ingress implementation

Relates: #29327
Fixes: #28949

Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
nbusseneau pushed a commit that referenced this pull request Dec 5, 2023
[ upstream commit 4f515fa ]

This is to align with Ingress implementation

Relates: #29327
Fixes: #28949

Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
nbusseneau pushed a commit that referenced this pull request Dec 5, 2023
[ upstream commit 4f515fa ]

This is to align with Ingress implementation

Relates: #29327
Fixes: #28949

Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
nbusseneau pushed a commit that referenced this pull request Dec 6, 2023
[ upstream commit 4f515fa ]

This is to align with Ingress implementation

Relates: #29327
Fixes: #28949

Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
pjablonski123 pushed a commit to pjablonski123/cilium that referenced this pull request Dec 15, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operator Impacts the cilium-operator component area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-ingress kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFP: Migrate Operator Ingress controller code to controller-runtime
3 participants