-
Notifications
You must be signed in to change notification settings - Fork 10
Define plugin interface and consumer library #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Where can I find the background of this? |
see the thread here https://kubernetes.slack.com/archives/C09R1PJR3/p1747786073248159 |
fb49e15
to
dab2b3b
Compare
Signed-off-by: Jian Qiu <[email protected]>
tools/plugins/spiffe/plugin.go
Outdated
return "spiffe" | ||
} | ||
|
||
func (p *Plugin) Credential(_ *v1alpha1.ClusterProfile) (*clientauthentication.ExecCredential, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't even use the clusterProfile?
tools/plugins/ocm/plugin.go
Outdated
} | ||
|
||
func (p *Plugin) Credential(cluster *v1alpha1.ClusterProfile) (*clientauthentication.ExecCredential, error) { | ||
tokenSecret, err := p.client.CoreV1().Secrets(cluster.Name).Get(context.Background(), "token", v1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this just use a secret of the same name as the clusterProfile...
Thanks, qiujian16! For the federated auth flow, the current interface could totally work; cluster-specific info. can be kept in the profile object, and application-specific info. would be discovered by the plugin itself in whichever means the plugin itself finds most appropriate. I am not 100% sure if we would like to enforce some form of linking mechanism that would allow the plugin to access application-specific info. in some way, e.g., via (implicitly) linked config maps, but that could be implemented in further PRs I assume. |
tools/plugins/interface.go
Outdated
Name() string | ||
|
||
// Credential returns the authentication information to connect to the cluster. | ||
Credential(cluster *v1alpha1.ClusterProfile) (*clientauthentication.ExecCredential, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed off line, we might need an extra field to contain application specific info so that different application get different identity and possibly different hub api URL too.
tools/plugins/spiffe/plugin.go
Outdated
|
||
func (p *Plugin) Credential(_ *v1alpha1.ClusterProfile) (*clientauthentication.ExecCredential, error) { | ||
svid, err := p.client.FetchJWTSVID(context.Background(), &workload.JWTSVIDRequest{ | ||
Audience: []string{"kube"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The audience should be passed via a param or some other ways.
Signed-off-by: Jian Qiu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you intend to keep the spiffee as a built-in provider? My feeling is that we should focus on the exec and not have any built-in to avoid having a two tiered approach for now.
We can definitely do it later, but I think it will be detrimental to the plugin ecosystem if some of considered built-in.
tools/config/config.go
Outdated
clusterEndpoint, err := getEndpointFromClusterProfile(cluster) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
config := &rest.Config{ | ||
Host: clusterEndpoint, | ||
TLSClientConfig: rest.TLSClientConfig{ | ||
CAData: getCABundleFromClusterProfile(cluster), | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a method for this entirely.
Build config from CP.
tools/config/config.go
Outdated
a := newAuthenticator(plugin, cluster) | ||
transportConfig, err := config.TransportConfig() | ||
if err := a.UpdateTransportConfig(transportConfig); err != nil { | ||
return nil, err | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd merge all this as "apply plugin for credentials" on the config (so that it's all put together)
tools/config/config.go
Outdated
func init() { | ||
spiffePlugin, err := spiffe.NewPlugin() | ||
if err == nil { | ||
pluginMap[spiffePlugin.Name()] = spiffePlugin | ||
} else { | ||
utilruntime.HandleError(err) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you isolate the "using library" part of obtaining credentials? (that way you would have a main that sets up the plugin, has a clusterprofile example and asks the library to construct credentials)
tools/plugins/spiffe/plugin.go
Outdated
@@ -0,0 +1,84 @@ | |||
package spiffe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you make this plugin exec'ed ? So his package would become a main and we can leverage the client-go code to call it.
tools/plugins/spiffe/plugin.go
Outdated
return pluginName | ||
} | ||
|
||
func (p *Plugin) Credential(cluster *v1alpha1.ClusterProfile) (*clientauthentication.ExecCredential, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the latest from the creds kep, you would actually get a "Cluster" object here.
https://github.com/kubernetes/enhancements/pull/5338/files#diff-6dcc0abd0aea71ca2e11e2a5b38ae81f50a227ae1c2c116d058b4de8abd35d20R236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot make Cluster in the CRD because of this line https://github.com/kubernetes/kubernetes/blob/a34c07971b610eb33908a743eadb4c61beeecc50/staging/src/k8s.io/client-go/pkg/apis/clientauthentication/v1/types.go#L92...
seems like a bug to me.
tools/config/config.go
Outdated
return res, nil | ||
} | ||
|
||
type Authenticator struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there no existing authenticator struct you can leverage and build on top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use https://github.com/kubernetes/kubernetes/blob/544e7a63e2cb6646d19e6a17bd665b07f39843d1/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go#L244 as long as we agree that exec is the only way.
tools/plugins/exec/plugin.go
Outdated
return p.name | ||
} | ||
|
||
func (p *Plugin) Credential(cluster *v1alpha1.ClusterProfile) (*clientauthentication.ExecCredential, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there code in client-go we can leverage to avoid having to implement this ourselves?
tools/config/config.go
Outdated
PluginPropertyKey = "plugin.multicluster.k8s.io" | ||
) | ||
|
||
var pluginMap = map[string]plugins.PluginInterface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi qiujian16! Since this variable is not exposed, for exec plugins I assume that people wouldn't be able to use the BuildConfigFromClusterProfile
, if I understand it correctly?
tools/config/config.go
Outdated
return []byte(caString) | ||
} | ||
|
||
func getPluginFromClusterProfile(cluster *v1alpha1.ClusterProfile) (plugins.PluginInterface, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi qiujian16! Just a quick Q on this part: it seems that the API alright includes a provider name part (and the exec plugin uses it) which seems to suggest multiple plugins available at a time, but the code here (including the Build
function) would query the plugin name property key (i.e., only one active plugin at a time) and retrieve the desirable plugin.
Signed-off-by: Jian Qiu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how simple it is; thank you for putting it together.
The only thing missing is provider configuration that would need to be done once for the controller on start and I think we can have a working prototype!
Config ProviderConfig `json:"config"` | ||
} | ||
|
||
type ProviderConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there a blocker to use clientauthenticationapi.Cluster ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question, they look the same?
tools/config/config.go
Outdated
} | ||
|
||
// BuildConfigFromClusterProfile is to build the rest.Config to init the client. | ||
func BuildConfigFromClusterProfile(cluster *v1alpha1.ClusterProfile, providerName string) (*rest.Config, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this proto!
I think we will need an additional method.
func GetOptimalPluginForCP(CP, pluginConfig) pluginName
Something that would allow a user to find the right plugin based on what they've configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also again, I do love how simple it made this function by reusing existing exec components. thats a win for maintainability :) (and the immense featureset we get on proxy, TLS config etc...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any example of what a "provider" is in real world case? I am a bit confused on what it actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A provider type name would be GKE
.
So for example, inside of the ClusterProfile, there will be a map with the key GKE
. Something like:
status:
credentials:
- name: GKE
config:
server: http://apiserver_endpoint:port
In the controller configuration (likely a flag or configmap?), there will be something like:
GKE: ExecConfig{
command:"/usr/bin/gke-gcloud-auth-plugin",
apiVersion: "client.authentication.k8s.io/v1beta1",
provideClusterInfo: true
}
MyProviderType: ExecConfig{
command:"/usr/bin/mybinary",
provideClusterInfo: true
}
You can easily see that the binary is /usr/bin/gke-gcloud-auth-plugin
and it will receive infor from the cluster thats in the config
part of the CP. That's it.
You can also easily imagine that if there were two providers to support, it's just a matter of adding configuration into the controller to support them, for example the "MyProviderType" above. And the ClusterProfile would have a credentials request for that kind of provider (my example only has GKE, but it technically could have a different one as well).
tools/config/config.go
Outdated
|
||
// If it is not exec Config, we have to find a way to fallback to a different path. | ||
execConfig := &clientcmdapi.ExecConfig{} | ||
err = json.Unmarshal(provider.Config.Raw, execConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exec config will likely not come from the CP; it will come from the configuration of the provider type for the controller.
when I look at this struct https://github.com/kubernetes/client-go/blob/master/tools/clientcmd/api/types.go#L208
type ExecConfig struct {
// Command to execute.
Command string `json:"command"`
// Arguments to pass to the command when executing it.
// +optional
Args []string `json:"args"`
// Env defines additional environment variables to expose to the process. These
// are unioned with the host's environment, as well as variables client-go uses
// to pass argument to the plugin.
// +optional
Env []ExecEnvVar `json:"env"`
// Preferred input version of the ExecInfo. The returned ExecCredentials MUST use
// the same encoding version as the input.
APIVersion string `json:"apiVersion,omitempty"`
// This text is shown to the user when the executable doesn't seem to be
// present. For example, `brew install foo-cli` might be a good InstallHint for
// foo-cli on Mac OS systems.
InstallHint string `json:"installHint,omitempty"`
// ProvideClusterInfo determines whether or not to provide cluster information,
// which could potentially contain very large CA data, to this exec plugin as a
// part of the KUBERNETES_EXEC_INFO environment variable. By default, it is set
// to false. Package k8s.io/client-go/tools/auth/exec provides helper methods for
// reading this environment variable.
ProvideClusterInfo bool `json:"provideClusterInfo"`
// Config holds additional config data that is specific to the exec
// plugin with regards to the cluster being authenticated to.
//
// This data is sourced from the clientcmd Cluster object's extensions[exec] field:
//
// clusters:
// - name: my-cluster
// cluster:
// ...
// extensions:
// - name: client.authentication.k8s.io/exec # reserved extension name for per cluster exec config
// extension:
// audience: 06e3fbd18de8 # arbitrary config
//
// In some environments, the user config may be exactly the same across many clusters
// (i.e. call this exec plugin) minus some details that are specific to each cluster
// such as the audience. This field allows the per cluster config to be directly
// specified with the cluster info. Using this field to store secret data is not
// recommended as one of the prime benefits of exec plugins is that no secrets need
// to be stored directly in the kubeconfig.
// +k8s:conversion-gen=false
Config runtime.Object `json:"-"`
// InteractiveMode determines this plugin's relationship with standard input. Valid
// values are "Never" (this exec plugin never uses standard input), "IfAvailable" (this
// exec plugin wants to use standard input if it is available), or "Always" (this exec
// plugin requires standard input to function). See ExecInteractiveMode values for more
// details.
//
// If APIVersion is client.authentication.k8s.io/v1alpha1 or
// client.authentication.k8s.io/v1beta1, then this field is optional and defaults
// to "IfAvailable" when unset. Otherwise, this field is required.
// +optional
InteractiveMode ExecInteractiveMode `json:"interactiveMode,omitempty"`
// StdinUnavailable indicates whether the exec authenticator can pass standard
// input through to this exec plugin. For example, a higher level entity might be using
// standard input for something else and therefore it would not be safe for the exec
// plugin to use standard input. This is kept here in order to keep all of the exec configuration
// together, but it is never serialized.
// +k8s:conversion-gen=false
StdinUnavailable bool `json:"-"`
// StdinUnavailableMessage is an optional message to be displayed when the exec authenticator
// cannot successfully run this exec plugin because it needs to use standard input and
// StdinUnavailable is true. For example, a process that is already using standard input to
// read user instructions might set this to "used by my-program to read user instructions".
// +k8s:conversion-gen=false
StdinUnavailableMessage string `json:"-"`
}
All those configs can be shared for all the CPs of a similar provider type.
instead of passing providerName, we should pass a struct with both the name and the exec config:
type Provider struct {
Name string
ExecConfig *clientcmdapi.ExecConfig
}
Update code following the KEP
Signed-off-by: Jian Qiu <[email protected]>
Signed-off-by: Jian Qiu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikeshng, qiujian16 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:
Approvers can indicate their approval by writing |
No description provided.