Add type info to the AuthInfo object. #176
Conversation
k8s-ci-robot
added
sig/multicluster
|
Looks reasonable to me. Nothing controversial here. |
|
/lgtm |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
| // Config is a map of values that contains the information necessary for a | ||
| // client to determine how to authenticate to a Kubernetes API server. | ||
| // +optional | ||
| Config map[string]string | ||
| } | ||
|
|
||
| // AuthProviderType contains metadata about the auth provider. It should be used |
There was a problem hiding this comment.
nit: This says metadata but it just contains the name right now.
There was a problem hiding this comment.
That's true. However, I expect this object to potentially grow, and the name is technically "metadata", so I think I'd prefer to leave it like this, since it describes the of this object.
pkg/apis/clusterregistry/types.go
Outdated
| // Config is a map of values that contains the information necessary for a | ||
| // client to determine how to authenticate to a Kubernetes API server. | ||
| // +optional | ||
| Config map[string]string | ||
| } | ||
|
|
||
| // AuthProviderType contains metadata about the auth provider. It should be used | ||
| // by clients to differentiate between different kinds of auth providers, and so |
There was a problem hiding this comment.
Good catch! I actually meant "to", not "so". Fixed.
| type AuthProviderType struct { | ||
| // Name is the name of the auth provider. | ||
| // +optional | ||
| Name string |
There was a problem hiding this comment.
If we don't add anything else to this, it seems somewhat similar to the AuthProviderConfig:Name field. That is, couldn't one use the AuthProviderConfig:Name field to designate the auth provider's name? It seems the example of a controller vs a user could still use the AuthProviderConfig:Name to find the relevant auth config.
Is this AuthProviderType addition still relevant if we consider making cluster namespace scoped?
There was a problem hiding this comment.
Yes, I think so. The idea of the Type field is that it specifically allows for repeated AuthProviders of one type with different info, and makes this more obvious than having that info in the bag of data. For example, imagine that you have an AuthProvider type that is "GCP service account". You can now have two different AuthProvider instances with that type, but with different names and representing different service accounts.
Also, my intention here is that this may expand in the future in a backwards-compatible way. For example, I could see adding a version field. I'd also rather people not try to squeeze this information into the name (e.g., gcp-service-account_foo, gcp-service-account-bar for two different AuthProviders). I think this field makes writing a plugin a bit easier as well, and provides a standard way to encode this. It's something that every AuthProvider object will need to do somehow, so making it top-level and typed seems like a reasonable thing to do.
There was a problem hiding this comment.
Thanks! I was interested to hear your thoughts on this. I agree, I had thought that reusing an auth provider type in the top level config name would require overloading that name field quite a bit. I also think that explicitly specifying this AuthProviderType struct field makes it prescriptive so that users don't try to embed that sort of metadata in the Config map. Instead, this new field is where it should go.
| type AuthProviderType struct { | ||
| // Name is the name of the auth provider. | ||
| // +optional | ||
| Name string |
There was a problem hiding this comment.
Thanks! I was interested to hear your thoughts on this. I agree, I had thought that reusing an auth provider type in the top level config name would require overloading that name field quite a bit. I also think that explicitly specifying this AuthProviderType struct field makes it prescriptive so that users don't try to embed that sort of metadata in the Config map. Instead, this new field is where it should go.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: font, madhusudancs, perotinus The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/test pull-cluster-registry-verify-gosrc |
/sig multicluster
Fixes #169.
/cc @pmorie @madhusudancs @font
Please take a look at this; this is an API change and I want to make sure that we're happy with it, or that we have a chance to suggest more fields that should be in
AuthProviderType.