-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add "tsh kube" commands #4769
Add "tsh kube" commands #4769
Conversation
1e0e275
to
2bb09f6
Compare
lib/client/keystore.go
Outdated
kubeCerts := make(map[string][]byte) | ||
// Optionally, read kube TLS certs. | ||
// TODO(awly): unit test this. | ||
if clusterName != "" { |
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 is introducing a lot of change in the existing client code and adds another task for the function it should not ideally perform. Have you considered adding GetKubeClusterKey
as an alternative? It is an aggregation of GetKey
with kube cert lookup:
func GetKubeClusterKey(store *FSLocalKeyStore, proxyHost, username, clusterName string) (*Key, error) {
key, err := store.GetKey(proxyHost, username)
if err != nil {
return nil, trace.Wrap(err)
}
dirPath, err := store.dirFor(proxyHost, false)
if err != nil {
return nil, trace.Wrap(err)
}
kubeCerts, err := ReadKubeCerts(dirPath, clusterName)
if err != nil {
store.log.Error(err)
return nil, trace.Wrap(err)
}
key.ClusterName = clusterName
key.KubeTLSCerts = kubeCerts
return key, nil
}
ReadKubeCerts
could be tested separately and GetKubeClusterKey
needn't be tested at all since it solely calls into APIs which are tested.
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.
Valid point.
Since we'll be storing database and app certs in a similar way, I changed it to use pluggable options.
WDYT?
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.
Well, it is an improvement in that it will isolate the code so it becomes easier to test, yes.
You should consider doing this for other related APIs - e.g. DeleteKey
.
6abb52a
to
1e23046
Compare
lib/client/keystore.go
Outdated
kubeCerts := make(map[string][]byte) | ||
// Optionally, read kube TLS certs. | ||
// TODO(awly): unit test this. | ||
if clusterName != "" { |
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.
Well, it is an improvement in that it will isolate the code so it becomes easier to test, yes.
You should consider doing this for other related APIs - e.g. DeleteKey
.
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 with a few comments
APIVersion: "client.authentication.k8s.io/v1beta1", | ||
Command: v.Exec.TshBinaryPath, | ||
Args: []string{"kube", "credentials", | ||
fmt.Sprintf("--kube-cluster=%s", c), |
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 would consider quoting the flag values, =%q
, here and below.
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.
These args are passed directly to exec
, not to a shell.
Quotes won't be stripped (like they would be with a shell) and tsh
will see --kube-cluster
as "foo"
instead of foo
}, | ||
} | ||
if v.Exec.TshBinaryInsecure { | ||
authInfo.Exec.Args = append(authInfo.Exec.Args, "--insecure") |
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 --insecure
flag going to be recognized fine when appended? I've run into some issues with that before, that it had to go right after tsh
. I'd consider prepending it instead.
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.
it worked fine when I tested, but I can see it being a problem for tsh ssh
where it expects flags before arguments
b7598c0
to
af43459
Compare
1. `tsh kube clusters` - lists registered kubernetes clusters note: this only includes clusters connected via `kubernetes_service` 2. `tsh kube credentials` - returns TLS credentials for a specific kube cluster; this is a hidden command used as an exec plugin for kubectl 3. `tsh kube login` - switches the kubectl context to one of the registered clusters; roughly equivalent to `kubectl config use-context` When updating kubeconfigs, tsh now uses the exec plugin mode: https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins This means that on each kubectl run, kubectl will execute tsh with special arguments to get the TLS credentials. Using tsh as exec plugin allows us to put a login prompt when certs expire. It also lets us lazy-initialize TLS certs for kubernetes clusters.
af43459
to
efa9802
Compare
tsh kube clusters
- lists registered kubernetes clustersnote: this only includes clusters connected via
kubernetes_service
tsh kube credentials
- returns TLS credentials for a specific kubecluster; this is a hidden command used as an exec plugin for kubectl
tsh kube login
- switches the kubectl context to one of theregistered clusters; roughly equivalent to
kubectl config use-context
When updating kubeconfigs, tsh now uses the exec plugin mode:
https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins
This means that on each kubectl run, kubectl will execute tsh with
special arguments to get the TLS credentials.
Using tsh as exec plugin allows us to put a login prompt when certs
expire. It also lets us lazy-initialize TLS certs for kubernetes
clusters.
Updates #3952
Fixes #3691
As before, sending this out for review before all unit tests are finished. I'll be adding those shortly.