Refactor Hubble as a cell #35206

kaworu · 2024-10-03T14:06:42Z

Part of #34501, Better reviewed commit by commit.

Refactor Hubble configuration and initialization using hive/cell. The rational and goal / non-goal are explained in the referenced issue as well as in the second commit:

Currently, all the Hubble configuration and initialization is
implemented in the Cilium daemon. Hubble related flags (e.g.
--enable-hubble) are member of the DaemonConfig struct, and the Hubble
startup code is under daemon/cmd/hubble.go. Most notably the
launchHubble() func which handle all the Hubble component startup is
called from cmd.newDaemon().

Over time, launchHubble() has become increasingly complex, initializing
more and more Hubble subsystems while bailing out if any of them
encounter a configuration or startup error. This has lead to a situation
where a misconfiguration of one part of Hubble (e.g. metrics) could
effectively disable other Hubble components without crashing the Cilium
daemon.

The recent addition of many new Hubble feature / subsystems, for example
monitor event filtering1, L7 data redaction2, and k8s Events
integration on drop notifications3 introducing new Hubble related
flags and logic both increase the potential of startup failure and the
surface of affected features.

Furthermore, most of the new feature increase Hubble's reliance on the
Cilium daemon for either flag setup/parsing and/or dependencies
injection, making it harder to decouple Hubble from the Daemon.

Fortunately, many part of Cilium already use the hive/cell framework for
configuration, startup, and dependency injection. Most notably, all
Hubble dependencies are now available as cell, which open the
possibility to convert Hubble components as cell themselves.

The goal of this patch series is to refactor Hubble into its own new
cell and decouple it from the Cilium daemon. Concretely, all the code
under daemon/cmd/hubble.go will be moved away into the new cell, along
with the flags and Hubble startup, until the point where it would be
possible to create a hive with the Hubble cell but without the Cilium
daemon.

Splitting the Hubble components into smaller cells is left for a later
time.

I tried my best to split into reviewable chunk, each commit focusing on moving one well defined part. As a result, there are a lot of commits (sorry). I considered opening multiples PRs for each refactoring stages, but OTHO it would increase the likelihood of conflicting with other PRs and I'm not looking forward to handle merge conflicts related to such a huge code shuffle, but if reviewers feels it is necessary then I'll do it.

Conceptually, there are four stages implemented:

remove all references of the Cilium daemon from launchHubble(). This stage is a prerequisite to be able to move Hubble startup into its own new cell,
move the launchHubble() code into the new Hubble cell,
Move all Hubble configuration flags, defaults, and logic into the Hubble cell,
use hive lifecycle to start and shutdown Hubble components instead of relying on the Cilium daemon's context.

rolinh

That's a massive cleanup and improvement! I initially feared reviewing this PR given the lines diff and the number of commits but how you broke the changes into multiple commits with detailed explanation of why each change is required is brilliant and made my work as a reviewer surprisingly easy!

With that said, the changes look good to me overall. It's a massive step forward in terms of modularizing the Hubble subsystem and highlights where we could still improve. The only minor concern I have is related to Validate() error (see comment below).

pkg/hubble/cell/hubble.go

pkg/hubble/cell/config.go

pkg/hubble/cell/hubble.go

qmonnet

Impressive work, and like Robin I loved the way you broke it down into small commits. Looks good to me! 🚀

mhofstetter

Great work - looks very good! Thanks a lot for tackling this! 😍 🎉

I left some small comments / suggestions inline.

daemon/cmd/hubble.go

pkg/hubble/cell/hubble.go

pkg/hubble/cell/cell.go

pkg/hubble/cell/hubble.go

pkg/hubble/cell/cell.go

pkg/hubble/cell/config.go

pkg/hubble/cell/hubble.go

ovidiutirla

Thanks for the contribution and time spent to split the commits 🚀

pkg/hubble/cell/cell.go

pkg/hubble/cell/hubble.go

pkg/hubble/cell/cell.go

pkg/hubble/cell/config.go

Validation of HTTP header redaction has moved from DaemonConfig.Populate() to the Hubble's Config Validate() func. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

The --hubble-drop-events-reasons flag is now registered through pflags as a string slice (previously registered as a string). This change has two minor implications: it is now possible to provide the flag multiple times, and each flag value are treated as comma-separated values (previously white spaces separated). For the latter, it should not create issue since the reasons values don't contains commas, and a backward compatible logic has been implemented. The viper-check.sh change allow viper.{Get,Set} in go comments lines. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

Explicit the contract between the Hubble integration and the Cilium agent. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

Now that all Hubble related code has been extracted from the Cilium daemon, we can hide away the Hubble integration implementation details from other packages. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

Before this patch, the Cilium daemon was responsible to "launch hubble", i.e. handle Hubble components startup. Now that Hubble is fully integration into the hive/cell framework, we can plug into its life cycle management and free the Cilium daemon from starting Hubble. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

Instead of using the global logging.DefaultLogger. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

joamaki

Nice!

rolinh · 2024-10-07T09:23:47Z

/test

As part of cilium#35206, we refactored Hubble as a Hive cell, and we inadvertedly changed the default EnableHubble to true. Since the helm chart defaults to not setting EnableHubble at all if the option is false in the values file, it is currently not possible to disable Hubble. Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>

As part of cilium#35206, we refactored Hubble as a Hive cell, and we inadvertently changed the default EnableHubble to true. Since the helm chart defaults to not setting EnableHubble at all if the option is false in the values file, it is currently not possible to disable Hubble. Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>

As part of #35206, we refactored Hubble as a Hive cell, and we inadvertently changed the default EnableHubble to true. Since the helm chart defaults to not setting EnableHubble at all if the option is false in the values file, it is currently not possible to disable Hubble. Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>

[ upstream commit 97aeb9e ] As part of #35206, we refactored Hubble as a Hive cell, and we inadvertently changed the default EnableHubble to true. Since the helm chart defaults to not setting EnableHubble at all if the option is false in the values file, it is currently not possible to disable Hubble. Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

kaworu requested review from rolinh and mhofstetter October 3, 2024 14:06

kaworu self-assigned this Oct 3, 2024

kaworu requested review from a team as code owners October 3, 2024 14:06

kaworu requested a review from ovidiutirla October 3, 2024 14:06

github-actions bot added the hubble-cli PRs or GitHub issues related with hubble-cli label Oct 3, 2024

rolinh requested changes Oct 3, 2024

View reviewed changes

pkg/hubble/cell/hubble.go Outdated Show resolved Hide resolved

pkg/hubble/cell/config.go Outdated Show resolved Hide resolved

pkg/hubble/cell/hubble.go Outdated Show resolved Hide resolved

kaworu force-pushed the pr/kaworu/hubble-cell branch from c530ac9 to ab8cbc2 Compare October 4, 2024 07:59

kaworu requested a review from a team as a code owner October 4, 2024 07:59

kaworu requested a review from qmonnet October 4, 2024 07:59

kaworu force-pushed the pr/kaworu/hubble-cell branch from ab8cbc2 to c56bee9 Compare October 4, 2024 08:31

kaworu requested a review from rolinh October 4, 2024 08:33

rolinh approved these changes Oct 4, 2024

View reviewed changes

qmonnet approved these changes Oct 4, 2024

View reviewed changes

kaworu force-pushed the pr/kaworu/hubble-cell branch 2 times, most recently from 7fa7696 to ee4487b Compare October 4, 2024 09:50

mhofstetter reviewed Oct 4, 2024

View reviewed changes

ovidiutirla reviewed Oct 4, 2024

View reviewed changes

ovidiutirla requested a review from bimmlerd October 4, 2024 10:49

kaworu force-pushed the pr/kaworu/hubble-cell branch 2 times, most recently from 34c2eed to 3c25145 Compare October 4, 2024 12:46

kaworu added 6 commits October 7, 2024 09:46

hubble: [09/10] l7 redact configuration flags migration

6dbae1f

Validation of HTTP header redaction has moved from DaemonConfig.Populate() to the Hubble's Config Validate() func. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

hubble: expose the Hubble integration as an interface

74a51cb

Explicit the contract between the Hubble integration and the Cilium agent. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

hubble: unexport Hubble integration struct and its member

5715255

Now that all Hubble related code has been extracted from the Cilium daemon, we can hide away the Hubble integration implementation details from other packages. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

hubble: depends on logrus through hive

22984e0

Instead of using the global logging.DefaultLogger. Signed-off-by: Alexandre Perrin <alex@isovalent.com>

kaworu force-pushed the pr/kaworu/hubble-cell branch from 3cf6e3a to 22984e0 Compare October 7, 2024 07:49

joamaki approved these changes Oct 7, 2024

View reviewed changes

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 Oct 9, 2024

kaworu enabled auto-merge October 9, 2024 12:31

kaworu added this pull request to the merge queue Oct 9, 2024

Merged via the queue into cilium:main with commit 1229149 Oct 9, 2024
63 checks passed

kaworu deleted the pr/kaworu/hubble-cell branch October 9, 2024 13:14

devodev mentioned this pull request Oct 28, 2024

daemon: refactor Hubble Exporters as a cell #35596

Merged

This was referenced Oct 28, 2024

StringSlice no longer works for values splitted by spaces on hive cells #29210

Closed

Hubble metrics config parsing broken in main #35619

Closed

kaworu mentioned this pull request Jan 24, 2025

Hubble export deny/allow list flag parsing broken #37233

Closed

3 tasks

devodev mentioned this pull request Feb 12, 2025

daemon: fix hubble can't be disabled #37587

Merged

kaworu mentioned this pull request Feb 18, 2025

hubble is always enabled #37702

Closed

3 tasks

kaworu mentioned this pull request May 23, 2025

CFP: Convert Hubble into a hive/cell based architecture #34501

Open

Refactor Hubble as a cell #35206

Refactor Hubble as a cell #35206

Uh oh!

Conversation

kaworu commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rolinh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

mhofstetter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ovidiutirla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joamaki left a comment

Choose a reason for hiding this comment

Uh oh!

rolinh commented Oct 7, 2024

Uh oh!

Uh oh!

Uh oh!

kaworu commented Oct 3, 2024 •

edited

Loading