Open question: is it OK to not include InsecureSkipTLSVerify here? Need to think through what guidance to exec plugins would be.

Thinking about this more, I think we should include InsecureSkipTLSVerify.

If the kubeconfig has insecure-skip-tls-verify: true set, whatever credentials the auth plugin returns will be happily sent by client-go without verifying TLS.

If we omit this, I anticipate some auth plugins trying connections with insecure TLS as a fallback "just in case" that's how the user's kubeconfig was set up.

One upside of including InsecureSkipTLSVerify info is that it would allow credential providers to detect if replayable tokens they planned to return would be used in an insecure context, and to refuse.

ACK. @enj may slay me when he returns, but I'll add this into the struct. :)

One upside of including InsecureSkipTLSVerify info is that it would allow credential providers to detect if replayable tokens they planned to return would be used in an insecure context, and to refuse.

That's how I talked myself into this

You speak truth!

in the implementation, account for the fact that this envvar was already used in v1alpha1 and sent a serialized ExecCredential which did not include connection info

describe the behavior in the following scenarios (well-known error, etc?):

• the envvar is not set
• the envvar is set to an empty string
• the envvar cannot be decoded (because it is malformed)
• the envvar cannot be decoded (because it contains an unrecognized type)

or document that in LoadCluster and indicate this delegates to that

might consider names like LoadExecCredentialFromEnv / LoadExecCredential instead

ACK. I'll rev this description tonight.

well-known error

I am thinking here that a plugin that wants to use cluster info looks like this:

func main() {
  ec, rc, err := exec.LoadExecCredentialFromEnv()
  if err != nil {
    [0]
  }
  ...
}

The plugin will probably do something like fmt.Println(err); os.Exit(1); in [0]. I can't imagine that they would want to branch on what that error return is. If the exec plugin can't get the cluster information, I would hope that it doesn't sacrifice any security properties and just quits. Seems like it makes the package API surface easier to consume if we return opaque errors from these functions. If I am wrong about this assumption, then we should be able to update this package with those error helpers in a backwards compatible manner.

might consider names

I'll move forward with LoadExecCredential* to optimize for readability and speed of consensus, but I will mention that the original reason I went with Load is because the package name was exec and I had hoped that exec.Load() read as "load information necessary to do exec stuff".

nit: go

would it be simpler to do

func LoadCluster(data []byte) (execCredential runtime.Object, *rest.Config, error)

and let them type-assert the version they expect and get their exec config from execCredential.spec.cluster.config themselves?

Yes? Probably? You would have a better gut feel for this so I am tempted to go with the function signature that you proposed.

Your function signature gets rid of an error case where someone calls LoadCluster(data, &corev1.Pod{}) on my function signature. So I think I do like your's better.

I suppose injecting references for return values is nice in situations where you want the caller to be able to allocate the memory necessary for an object, but I won't be able to outsmart the Go compiler here, and it's 2020 so why even worry about memory...

nit: note this is only set if provideClusterInfo: true in the exec provider config