fix json log format panic, change the flag name in flagIsSet

[chenyw1990]

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubelet will panic with --logging-format=json

Which issue(s) this PR fixes:

Fixes #99239

Special notes for your reviewer:

@serathius
@ehashman
@knight42

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[lala123912]

/ok-to-test

[chenyw1990]

/retest

[chenyw1990]

/retest

[chenyw1990]

/retest

[lala123912]

/test pull-kubernetes-e2e-kind

[serathius]

/hold
This is not correct way to fix the issue. Json format doesn't support all klog flags, this logic is meant to protect against trying to enable JSON with one of the unsupported flags, so the logic is correct.

[serathius]

#98524 looks like a culprit.
It changed names of flags that are returned by unsupportedLoggingFlags without updating flagIsSet function.

[serathius]

Proposed fix, instead of adding json in Validate function, can you change flagIsSet function to

func flagIsSet(name string) bool {
	f := flag.Lookup(name)
	if f != nil {
		return f.DefValue != f.Value.String()
	}
	f = flag.Lookup(strings.Replace(name, "-", "_", -1))
	if f != nil {
		return f.DefValue != f.Value.String()
	}
	pf := pflag.Lookup(name)
	if pf != nil {
		return pf.DefValue != pf.Value.String()
	}
	panic(fmt.Sprintf("failed to lookup unsupported log flag: %s", name))
}

[chenyw1990]

Proposed fix, instead of adding json in Validate function, can you change flagIsSet function to

func flagIsSet(name string) bool {
	f := flag.Lookup(name)
	if f != nil {
		return f.DefValue != f.Value.String()
	}
	f = flag.Lookup(strings.Replace(name, "-", "_", -1))
	if f != nil {
		return f.DefValue != f.Value.String()
	}
	pf := pflag.Lookup(name)
	if pf != nil {
		return pf.DefValue != pf.Value.String()
	}
	panic(fmt.Sprintf("failed to lookup unsupported log flag: %s", name))
}

sure, I have modify the code.

[chenyw1990]

/retest

[serathius]

/unhold
/lgtm
/approve

[ehashman]

@chenyw1990 PTAL at the failing unit test, it looks related:

 === FAIL: cmd/kube-apiserver/app/options TestAddCustomGlobalFlags (0.00s)
    globalflags_test.go:59: [Default]: expected [add-dir-header alsologtostderr cloud-provider-gce-l7lb-src-cidrs cloud-provider-gce-lb-src-cidrs default-not-ready-toleration-seconds default-unreachable-toleration-seconds help log-backtrace-at log-dir log-file log-file-max-size log-flush-frequency logtostderr one-output skip-headers skip-log-headers stderrthreshold v vmodule], got [add_dir_header alsologtostderr cloud-provider-gce-l7lb-src-cidrs cloud-provider-gce-lb-src-cidrs default-not-ready-toleration-seconds default-unreachable-toleration-seconds help log-flush-frequency log_backtrace_at log_dir log_file log_file_max_size logtostderr one_output skip_headers skip_log_headers stderrthreshold v vmodule] 

[ehashman]

I think all comments should be addressed?

@liggitt would you PTAL?

[liggitt]

This looks a lot more coherent, thanks. Just had a couple suggestions about the unnormalize func

/approve
/hold for UnNormalize changes
instrumentation reviewer has lgtm once that is addressed

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenyw1990, liggitt, serathius

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:

• cmd/kube-apiserver/OWNERS [liggitt]
• pkg/kubelet/apis/config/OWNERS [liggitt]
• staging/src/k8s.io/component-base/OWNERS [liggitt]

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

[chenyw1990]

@liggitt Thanks for your suggestions, I have modified the code based on the review comments.

[liggitt]

/lgtm

[serathius]

/hold cancel
as UnNormalize changes were addressed

[chenyw1990]

/test pull-kubernetes-e2e-kind

[rifelpet]

@chenyw1990 We're seeing failures in the kOps e2e jobs using CI builds of k8s due to the --log-file flag changing to --log_file

$ docker run -it --rm k8s.gcr.io/kube-apiserver:v1.20.4 -- kube-apiserver --help
...
      --log-file string                  If non-empty, use this log file
      --log-file-max-size uint           Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)

however recent CI builds show

$ curl https://storage.googleapis.com/kubernetes-release-dev/ci/v1.21.0-beta.1.200+2eb32e930a30ec/bin/linux/amd64/kube-apiserver.tar | docker load
$ docker run -it --rm k8s.gcr.io/kube-apiserver-amd64:v1.21.0-beta.1.200_2eb32e930a30ec -- kube-apiserver --help
      --log_file string                  If non-empty, use this log file
      --log_file_max_size uint           Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)

Is this intentional?

[chenyw1990]

@rifelpet sorry, I have add a new PR to fixed this change. #100171