Skip to content

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Nov 15, 2023

As part of the "Cilium Ingress to controller-runtime library" migration (#28911), this PR migrates the TLS secret synchronization logic. This gets achieved by re-using functionality of the new secret-synchronization cell that is already using the controller-runtime library and is used by the Gateway API controller too (follow up of #29100).

For more information, please refer to the individual commits.

  • Adding support for default-secret to the secret syncer
  • Adding support for addtional watcher to the secret syncer (watch on IngressClass)
  • Actual Migration

@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 15, 2023
@mhofstetter
Copy link
Member Author

/test

@mhofstetter mhofstetter marked this pull request as ready for review November 15, 2023 16:51
@mhofstetter mhofstetter requested a review from a team as a code owner November 15, 2023 16:51
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/ingress-use-secretsyncer branch from 09a6236 to 3155530 Compare November 20, 2023 12:29
@mhofstetter mhofstetter requested a review from meyskens November 20, 2023 12:31
@mhofstetter
Copy link
Member Author

I addressed the input from @meyskens . Thanks a lot!

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, the patch is simpler that I have expected 👍

@mhofstetter mhofstetter force-pushed the pr/mhofstetter/ingress-use-secretsyncer branch from 3155530 to ea42dca Compare November 21, 2023 11:37
This commit adds the possibility to define additional watches in addition
to watching the object that is referencing the TLS Secret directly.

This provides the possibility to trigger reconciliation based on other events.

E.g. Default IngressClass changes

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit introduces the possibility to define the default Secret.

Default Secrets aren't referenced explicitly. Therefore, this Secrets
are referenced regardless of whether they are referenced or not.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit migrates the secret-sync for Ingress related TLS secrets
to use the controller-runtime based secret syncer, that is already
used by Ciliums Gateway API implementation.

By reusing the same secret-syncer we avoid potential problems of
fighting two syncers with each other. Especially if Gateway API &
Ingress can potentially have two different secret namespaces!

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit removes the old, now unused, implementation of the
Ingress Secret Sync based on informers.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/ingress-use-secretsyncer branch from ea42dca to c19a148 Compare November 21, 2023 11:47
@mhofstetter
Copy link
Member Author

rebased to main (without further changes)

@mhofstetter
Copy link
Member Author

/test

Copy link
Contributor

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM

@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 22, 2023
@lmb lmb merged commit 067a9c6 into cilium:main Nov 22, 2023
@mhofstetter mhofstetter deleted the pr/mhofstetter/ingress-use-secretsyncer branch November 22, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants