k8s.io/kms: drop direct dependency on klog

[enj]

Signed-off-by: Monis Khan mok@microsoft.com

Builds on #120896

This will make #120225 drop klog altogether.

/kind cleanup
/assign aramase

NONE

[aramase]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f881f8443265e26c1ce9c5578278b1c335cd9077

[aramase]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 76dafbdf43303b1704ffbfa1227c63420b099b3b

[dims]

/hold

[dims]

/assign @pohly

@enj @aramase we don't yet have guidance from folks working on logging/klog about allowing slog in our code base

[dims]

also build failure:

vendor/k8s.io/kms/pkg/service/grpc_service.go:21:2: package log/slog is not in GOROOT (/usr/local/go/src/log/slog)

[enj]

/assign @pohly

@enj @aramase we don't yet have guidance from folks working on logging/klog about allowing slog in our code base

FWIW the kms staging repo is for external consumption, hence I was trying to limit its dep tree.

also build failure:

vendor/k8s.io/kms/pkg/service/grpc_service.go:21:2: package log/slog is not in GOROOT (/usr/local/go/src/log/slog)

I figured it would fail if we didn't support go1.21 yet :)

[pohly]

Using slog.Debug means that logging will use the global default slog handler. That's okay(is), that handler can be set by a binary such that the output goes to klog, if that is what the binary is using. See:

• docs: interoperability with slog go-logr/logr#222
• slog support #120696

However, it means that the code doesn't support contextual logging. My proposal is that we continue to use klog for handling access to a logger and the go-logr API for log calls, i.e. not switch to slog:

• KEP-3077: contextual logging: promotion to beta in 1.30 enhancements#4219

How important is it to remove the klog dependency? This is doable, but requires more changes than planned right now. For example, there are helpers in klog like klog.KObj which are needed for structured logging. We would need a separate package with all the calls that we intend to keep using.

[pohly]

For the sake of simplicity, that other package could be klog/v3... then all it takes in code with full conversion to structured, contextual logging would be a s;klog/v2;klog/v3. But a bit more thought will be needed regarding interoperability of the two versions...

[enj]

How important is it to remove the klog dependency?

Quite high for the kms staging repo because the code in there has no use in k/k itself, it is just a thin wrapper around the proto APIs to make it easy for someone to build a plugin. The reason for using slog is so that a plugin can easily override the logger and doesn't have to deal with any of the weirdness associated with klog. I can certainly make this code just use the log package instead if we really want to. Alternatively I can delete the log statements here and move them into the mock implementation.

[enj]

Note also that this code doesn't need contextual logging because it is never going to be used in-tree. Basically this change should have no bearing on what we decide to do as a whole for the Kube project (where I agree using klog is the correct choice).

[pohly]

If you want to get rid of klog, then I'd prefer to have no log calls in that package at all. That's the only short-term solution anyway, or are you ready to make Go 1.21 the required version for it?

[pohly]

Note also that this code doesn't need contextual logging because it is never going to be used in-tree.

Who says that contextual logging is only something that is done in-tree? 😄

The concept also applies out-of-tree. For example, Cluster API is using it and the controller-runtime has support for it.

[enj]

/hold cancel

I removed all usage of slog from this PR.

[aramase]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 35a7cd8278402fa8e92a72fdf841aa58914e871e