Deprecate klog flags and add a deprecation message

[maelvls]

Many klog flags have been marked as deprecated in Kubernetes 1.23 (as per KEP-2845) and will go away in Kubernetes 1.26. I propose that we follow the same path for the same reasons.

Plan:

• cert-manager 1.12 (end of April 2023): start of the deprecation period for the flags --log_dir, --log_file, --log_flush_frequency, --logtostderr, --alsologtostderr, --one_output, --stderrthreshold, --log_file_max_size, --skip_log_headers, --add_dir_header, --skip_headers, --log_backtrace_at.
• cert-manager 1.15 (approx 6 month afterwards, Nov 2023): removal of the flags. We do not have a planned version in which we will remove the flags.

This PR adds a warning message that shows in two places:

• when using --help, the user will see the message DEPRECATED: this flag may be removed in the future.
• when using one of the deprecated flags, the user will see a line warning them (unfortunately, this line isn't JSON formatted):

  $ go run ./cmd/controller --log_dir=/tmp
  Flag --log_dir has been deprecated, this flag may be removed in the future
  

Before/after:

Commands

git checkout master && go run ./cmd/controller --help | cut -c-60 >/tmp/before 
gh pr checkout 5879 && go run ./cmd/controller --help | cut -c-60 >/tmp/after
diff -u /tmp/before /tmp/after

--- /tmp/before    2023-03-23 11:54:35.830525057 +0100
+++ /tmp/after     2023-03-23 11:54:35.830525057 +0100
empty diff

Deprecated flags:

$ go run ./cmd/controller --help | grep DEPRECATED | awk '{print $1}'
--add_dir_header
--alsologtostderr
--log_backtrace_at
--log_dir
--log_file
--log_file_max_size
--logtostderr
--one_output
--skip_headers
--skip_log_headers
--stderrthreshold

A subset of the klogs flags have been deprecated and will be removed in the future.

/kind cleanup

Notes to the reviewer:

• In order to display deprecation warnings, I swapped flag with github.com/spf13/pflag. We already import pflag, so I thought it would not be an issue.

[SgtCoDFish]

As mentioned in standup, the diff in the original doesn't have a corresponding line for this removal:

-      --log-flush-frequency duration                        Maximum number of seconds between log flushes (default 5s)

Also a couple of other things:

when using one of the deprecated flags, the user will see a line warning them (unfortunately, this line isn't JSON formatted):

question: Why is the deprecation notice not available in JSON? Is it too early to have parsed whether the user requested JSON logging or something?

cert-manager 1.15 (approx 6 month afterwards, Nov 2023): removal of the flags.

suggestion: I worry that this timeline is far too aggressive. If these flags become no-ops, then they don't provide a huge maintenance burden on us and they do no harm if the user sets them (other than the risk of a user expecting something to happen which won't happen)

I'd actually prefer to never remove these flags because it's (IMO) not a good enough reason to break people.

But if we must remove them, I think I'd rather give users (a lot) more time. That could potentially include a time.Sleep(n*5*time.Second) which runs if any of these flags were set, where n grows in each release to encourage users to remove the flags (since cert-manager will "hang" on startup if any of these flags were passed).

What do you think?

[maelvls]

This timeline is far too aggressive.

I agree, there is no good reason to remove these flags at the moment. Klog still supports them. The only long-term reason would be that one day klog 3.0 might be released, and we might want to migrate to it (I have only heard of klog 3.0 in the KEP-2845).

For context, the reason Kubernetes is migrating away from these flags is because they want to use a Go struct to configure logging (as opposed to using e.g. flag.Set("alsologtostderr", ...)).

For now, I will remove the warning about these flags being removed in cert-manager 1.15. I changed the warning to:

Flag --log_dir has been deprecated, this flag may be removed in the future

Why is the deprecation notice not available in JSON?

It looks like pflag uses fmt.Fprintf directly for the deprecation message in flag.go:

fmt.Fprintf(f.Output(), "Flag --%s has been deprecated, %s\n", flag.Name, flag.Deprecated)

It is most probably because this message is printed while the flags are being parsed: the logger isn't configured yet and can't be used. I wish it was handled better... but it seems like it is a good enough solution

--log-flush-frequency duration removed?

Ah, my mistake. I forgot to change one reference to flag to pflag.

[SgtCoDFish]

For now, I will remove the warning about these flags being removed in cert-manager 1.15

Thanks, that looks great!

It looks like pflag uses fmt.Fprintf directly for the deprecation message in flag.go...

Ah yeah... I can see why that happens. It's a shame but it's not the end of the world - especially if we don't remove the flags any time soon!

Thanks for working on this 😁

[maelvls]

I propose that we go ahead with this. Would you LGTM @SgtCoDFish?

Update 14 April 2023: I need to rework this after the changes made in #5828.

[inteon]

I propose that we go ahead with this. Would you LGTM @SgtCoDFish?

Update 14 April 2023: I need to rework this after the changes made in #5828.

I rebased the PR.

[SgtCoDFish]

/lgtm
/approve

We discussed this in the standup today (2023-08-25) and agreed this is a good improvement - thanks 😁

Only caveat from me (which doesn't directly affect this PR) is that I strongly believe we shouldn't remove these flags in any future release - cert-manager shouldn't break because of this. We should make them no-ops which just print that they're a no-op though.

(motivation: if I've been installing cert-manager with - excuse the incorrect syntax - helm install --set 'extraArgs=one_output for a while, I shouldn't see a break if I continue to use that parameter!)

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls, SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [SgtCoDFish,maelvls]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[inteon]

/lgtm /approve

We discussed this in the standup today (2023-08-25) and agreed this is a good improvement - thanks 😁

Only caveat from me (which doesn't directly affect this PR) is that I strongly believe we shouldn't remove these flags in any future release - cert-manager shouldn't break because of this. We should make them no-ops which just print that they're a no-op though.

(motivation: if I've been installing cert-manager with - excuse the incorrect syntax - helm install --set 'extraArgs=one_output for a while, I shouldn't see a break if I continue to use that parameter!)

The message says "DEPRECATED: this flag may be removed in the future", but we might just remove the implementation and still allow setting the flag. Or just keep the functionality but hide the flags.
The main goal is to indicate that ideally you should not use the flags & that if they are removed upstream, less of our users are affected.

[SgtCoDFish]

/test pull-cert-manager-master-e2e-v1-28