K8s client as reusable cell

[joamaki]

First commit drops the cnp_test.go test that was used to benchmark protobuf vs json serialization for K8s objects. It is no longer needed and would have complicated implementation of the k8s client cell.

Second commit implements K8s client as a reusable cell that provides the Clientset interface to the application, which is a composition of the different client interfaces used by Cilium (Kubernetes, APIExt, Cilium and Slim version of Kubernetes).

The configuration needed by the client is moved from DaemonConfig into client.Config in pkg/k8s/client/config.go and it implements CellFlags to register the command-line flags. This allows easy embedding of the client cell into other applications besides cilium-agent.

Rest of the commits convert uses of the k8s client over to this new abstraction out from k8s.Configure/k8s.Init. There are still some holdouts that use the global accessors (k8s.Client() etc.) that will be addressed in a follow-up.

To make this more concrete, here is an abbreviated example application using the client cell:

func main() {
  hive := hive.New(
    pflag.CommandLine, // Add cell flags to the default flagset
    viper.New(), // Use a new viper instance to bind flags to and read settings from

    client.Cell,
    exampleCell,
    hive.Require[*example](), // Require that *example is constructed.
  )
  pflag.Parse()
  hive.Run()
}

var exampleCell = hive.NewCell("example",  fx.Provide(newExample))

type example struct {
  clientset client.Clientset
}

func newExample(lc fx.Lifecycle, clientset client.Clientset) *example {
  ex := &example{clientset}
  lc.Append(fx.Hook{OnStart: ex.onStart, OnStop: ex.onStop})
  return ex
}

func (ex *example) onStart(context.Context) error {
  node, err := ex.clientset.CoreV1().Nodes().Get(/* ... */)
  ...
}

The application now has the k8s configuration options included, so to run it with kubeconfig file:

$ ./example --k8s-kubeconfig-path=$HOME/.kube/config

[joamaki]

/test

[joamaki]

Here is how cilium-agent objects looks like when K8s client is in use:

[cilium:~/go/src/github.com/cilium/cilium/daemon]$ ./cilium-agent objects --k8s-kubeconfig-path=$HOME/.kube/config
Supplied objects:

  🎁️ *logrus.Entry

  🎁️ cmd.DaemonCellConfig from daemon

  🎁️ gops.GopsConfig from gops

  🎁️ client.Config from k8s-client

Constructors:

  🛠️  go.uber.org/fx.(*App).dotGraph-fm():
    • fx.DotGraph

  🛠️  go.uber.org/fx.(*App).shutdowner-fm():
    • fx.Shutdowner

  🛠️  go.uber.org/fx.New.func1():
    • fx.Lifecycle

  🛠️  k8s-client (github.com/cilium/cilium/pkg/k8s/client.newClientset()):
    • client.Clientset

Start hooks:

  • github.com/cilium/cilium/pkg/gops.registerGopsHooks.func1 (.../gops/cell.go:41)
  • github.com/cilium/cilium/pkg/k8s/client.(*compositeClientset).onStart-fm (..././<autogenerated>:1)
  • github.com/cilium/cilium/daemon/cmd.registerDaemonHooks.func1 (.../cmd/daemon_main.go:1551)

Stop hooks:

  • github.com/cilium/cilium/daemon/cmd.registerDaemonHooks.func2 (.../cmd/daemon_main.go:1557)
  • github.com/cilium/cilium/pkg/k8s/client.(*compositeClientset).onStop-fm (..././<autogenerated>:1)
  • github.com/cilium/cilium/pkg/gops.registerGopsHooks.func2 (.../gops/cell.go:48)

Configurations:

  ⚙ gops: gops.GopsConfig{GopsPort:0x26a2}
  ⚙ k8s-client: client.Config{K8sAPIServer:"", K8sKubeConfigPath:"/home/jussi/.kube/config", K8sClientQPS:0, K8sClientBurst:0, K8sHeartbeatTimeout:30000000000, EnableK8sAPIDiscovery:false}
  ⚙ daemon: cmd.DaemonCellConfig{SkipDaemon:false}

From the above we see that the client.Clientset is available to use in the application, and
we can see from "Start hooks" that gops is started first, followed by the k8s client and finally the
daemon start hook.

We can use for example go doc to explore what it contains:

$ go doc client.Clientset
package client // import "github.com/cilium/cilium/pkg/k8s/client"

type Clientset interface {
        kubernetes.Interface
        apiext_clientset.Interface
        cilium_clientset.Interface

        // Slim returns the slim client, which contains some of the same APIs as the
        // normal kubernetes client, but with slimmed down messages to reduce memory
        // usage. Prefer the slim version when caching messages.
        Slim() slim_clientset.Interface

        // IsEnabled returns true if Kubernetes support is enabled and the
        // clientset can be used.
        IsEnabled() bool

        // Config returns the configuration used to create this client.
        Config() Config
}
    Clientset is a composition of the different client sets used by Cilium.

$ go doc client.Config
package client // import "github.com/cilium/cilium/pkg/k8s/client"

type Config struct {
        // K8sAPIServer is the kubernetes api address server (for https use --k8s-kubeconfig-path instead)
        K8sAPIServer string

        // K8sKubeConfigPath is the absolute path of the kubernetes kubeconfig file
        K8sKubeConfigPath string

        // K8sClientQPSLimit is the queries per second limit for the K8s client. Defaults to k8s client defaults.
        K8sClientQPS float32

        // K8sClientBurst is the burst value allowed for the K8s client. Defaults to k8s client defaults.
        K8sClientBurst int

        // K8sHeartbeatTimeout configures the timeout for apiserver heartbeat
        K8sHeartbeatTimeout time.Duration

        // K8sEnableAPIDiscovery enables Kubernetes API discovery
        EnableK8sAPIDiscovery bool
}

func (cfg Config) CellFlags(flags *pflag.FlagSet)
func (cfg Config) K8sAPIDiscoveryEnabled() bool
func (cfg Config) K8sLeasesFallbackDiscoveryEnabled() bool

[joamaki]

Closing and reopening to rerun CI.

[joamaki]

/test

[joamaki]

/test

[pippolo84]

Great! 🎉
A couple of things left inline (one naming preference and a possible missing initialization).

[christarazi]

LGTM besides a few minor comments. Awesome PR! 🚀

[pippolo84]

Nice PR! LGTM 💯

[squeed]

This all is a bit confusing. there's initializeFlags() which takes no arguments, then there's the flags var, then there's RootCmd.Flags()? And commands

Is there any way to make this simpler? I would expect that creating the Hive would make the flags Just Work.

[joamaki]

The reason for this is that the root, "dot-graph" and "objects" commands in cilium-agent want all the same flags, but then we have "cmdref" which shouldn't. But now that you pointed this out I realized I can just made cmdref a hidden command and then we don't have Documentation/cmdref/cilium-agenc_cmdref.md and it doesn't matter what flags are there (unless we start adding mandatory flags at some point etc.). Will push the fix in a sec...

[squeed]

I get it now. I realized as I was walking for coffee that a Cell is different from a subcommand. Nevertheless, any way you can make flags simpler would be good; I've personally had poor experiences with other "do-it-all" cmd frameworks making it unergonomic to set flags the way you want.

[squeed]

Hey look, it's generics (not really) :-p.

[squeed]

Okay but really, this is a bit too much black magic for me. So, this is basically a decorator -- by doing evil reflection magic to ensure args are passed through. Cute... but...

For the sake of simplicity, what if this just took a func() and left it at that? Yes, it means a closure, but that is certainly more idiomatic.

[squeed]

And if there's a reason I'm missing that a closure wouldn't work, can you at least document the magic?

[joamaki]

Yeah I got annoyed by having to do fx.Invoke(func(lc fx.Lifecycle) { lc.Append(fx.Hook{OnStart: func(context.Context) error { thisOneThing() } } ) }) :)

[joamaki]

Ah the reason why this can’t just take “func()” is that the input arguments actually matter. E.g. if you give this a “func(a A)” then fx will go and construct “A” and pass it in. The trade-off here is between having to do this start hook the long way via fx.Invoke etc. or this magic. I’m fine either way, but though that for the command-line utility use-cases where we just want to construct and start few things and then run a function that uses them it makes sense to provide a simple helper.

I’ll document this better though!

[joamaki]

Documented and changed it to take a function that returns an error so it's more general. Does this seem reasonable now? What we could also do, if you think it'd be preferred, is to write this as func OnStart[T any](func(T) error) *Cell. It'd constrain the function to a single input argument, but one could then use fx.In: type Inputs struct { fx.In; Foo Foo; ...}.

[squeed]

Documenting the magic is fine for now. If it winds up needing changes later, we can do that.

It is cute :-)

[nbusseneau]

Reviewed test changes, LGTM, thanks!

[joamaki]

/test

[squeed]

One minor question, then lgtm.

[joamaki]

/test-1.16-4.9

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

---

Doh, moving the option.Config.Populate() before SetMapElementSizes screwed up the dynamic map size calculation. Fixed.

[joamaki]

/test

[joamaki]

ci-l4lb failure fixed by just merged #21302. Earlier run of ci-l4lb passed. Marking ready-to-merge.