This change is 

Hi @wanghaoran1988. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

/cc @ericchiang I think maybe we can disable the authProvider wrap for transport if the beartokens or basic auth enabled ?

Why shouldn't the auth provider be able to wrap at his pleasure? For instance, couldn't you have an auth provider that used some native tooling to produce a bearertoken that was never stored inside of the kubeconfig file?

The way the current interface is written, as a transport wrapper, has nothing to do with bearer tokens. Doing this starts to make decisions based on the existing implementations, so the abstraction is starting to leak.

We should either change the interface to be about bearer tokens and declare a clear priority, or continue to modify auth providers to not override the authorization headers.

We should either change the interface to be about bearer tokens and declare a clear priority, or continue to modify auth providers to not override the authorization headers.

I think the transport wrapper should be allowed to wrap in any way it sees fit. What is the value in artificially restricting it?

I think the transport wrapper should be allowed to wrap in any way it sees fit. What is the value in artificially restricting it?

@deads2k My interface refactor point is that all or current (and proposed) client auth plugins are actually just bearer token sources, but the interface deals with wrapping transports.

If we want explicit bearer tokens/basic auth defined in the kubeconfig to override the client auth plugin's (what this PR is trying to do) we should probably refactor the client auth plugin interface into a bearer token source, instead of what this PR is doing and conditionally not wrapping the transports based on the Authorization header.

I'm not attached to a Authorization header priority, but the idea of the client auth plugin as a bearer token source instead of a transport wrapper has come up in other PRs #39587 (comment)

If you want to get this in instead of #46666, I'd be happy to close my PR.

@cjcullen @wanghaoran1988 maybe we should get this or #46666 in for 1.7 then work on the interface refactoring for 1.8? Add a TODO and open an issue?

@k8s-bot ok to test

Opened up #46728 to track refactoring the interface for 1.8. I marked #44476 on the 1.7 milestone.

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

/lgtm

@caesarxuchao @deads2k @lavalamp @smarterclayton for approval

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

/sig auth

I think the auth provider plugin should have the ability to wrap the transport regardless. The plugin can preserve the header information. Looks like the oidc one already did this: #45529

@deads2k Yeah, the oidc plugin already have, but for GCP plugin doesn't now, it use "golang.org.oauth2.Transport" to wrap the token, we need update the dependency to preserve the header if you like this way. but this pr is the simpler fix for now.

I think the auth provider plugin should have the ability to wrap the transport regardless.

I agree, I'd rather leave the power in the hands of the auth provider

to wrap the token, we need update the dependency to preserve the header if you like this way. but this pr is the simpler fix for now.

can choose whether to delegate to the oauth round tripper or the base round tripper:

func (g *gcpAuthProvider) WrapTransport(rt http.RoundTripper) http.RoundTripper {
	return &conditionalTransport{&oauth2.Transport{Source: g.tokenSource, Base: rt}}
}

type conditionalTransport struct {
	oauthTransport *oauth2.Transport
}

func (t *conditionalTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	if len(req.Header.Get("Authorization")) != 0 {
		return t.oauthTransport.Base.RoundTrip(req)
	}
	return t.oauthTransport.RoundTrip(req)
}

@liggitt Cool !, will update soon :)

@k8s-bot pull-kubernetes-kubemark-e2e-gce test this

@k8s-bot pull-kubernetes-unit test this

@wanghaoran1988 thanks! please update the release notes to say that the GCP auth plugin no longer overwrites existing Authorization headers.

This is a bug fix so I'm going to nominate this for 1.7.

@ericchiang Done

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, liggitt, wanghaoran1988

Associated issue: 44476

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

/retest

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

/retest

Automatic merge from submit-queue