Teleterm: Add listing kube resources endpoint #46753

kimlisa · 2024-09-19T07:45:28Z

easier to review by commit

Adds a new grpc endpoint and related boilerplates for listing kube resources

if confused what is the difference between the other endpoint where we list kubes, kube resources refers to kube_cluster's sub resources:

teleport/api/types/constants.go

Line 1233 in 110b23a

// KubernetesResourcesKinds lists the supported Kubernetes resource kinds.

kimlisa · 2024-09-19T07:46:26Z

lib/teleterm/api/uri/uri.go

+// AppendKubeResource appends kube resource segment to the URI.
+// Modeled after how access request constructs the resource ID:
+// <teleport-cluster-name>/<subresource kind>/<kube_cluster name>/<subresource name>
+func (r ResourceURI) AppendKubeResource(kubeResourceKind string, kubeClusterName string, kubeResourceName string) ResourceURI {
+	r.path = fmt.Sprintf("%v/%v/%v/%v", r.path, kubeResourceKind, kubeClusterName, kubeResourceName)
+	return r
+}
+


i wasn't entirely sure what this was for, other than perhaps to uniquely identify resources?

Yeah, URIs are used for identifying resources.

fmt.Sprintf("%v/%v/%v/%v", r.path, kubeResourceKind, kubeClusterName, kubeResourceName)

I see some problems with this scheme. kubeResourceKind is an unknown string, so in theory, it could collide with other URIs. Additionally, we would not be able to identify if a given URI string is a kube resource URI without knowing kubeResourceKind.
A fix for this would be adding a known prefix in the URI, e.g:

"/clusters/teleport.sh/kube-resources/namespace/kube-cluster-name/namespace-name"

However, perhaps it would make more sense to treat a kube resource as a child of the kube cluster? I mean something like this:

var pathKubes = urlpath.New("/clusters/:cluster/kubes/:kubeName") var pathKubeResources = urlpath.New("/clusters/:cluster/kubes/:kubeName/:subResourceKind/:subResourceName")

cc @ravicious

i went ahead and applied your first suggestion instead:

"/clusters/teleport.sh/kube-resources/namespace/kube-cluster-name/namespace-name"

If it's correct that kube subresources always belong to a specific kube resource, and I believe it's correct, then I'd definitely go with the second option that Grzegorz proposed.

If we currently support only namespaces within the app, then for now I'd make a definition only for namespaces. That would be:

kubeResourceNamespace: '/clusters/:rootClusterId/(leaves)?/:leafClusterId?/kubes/:kubeId/namespaces/:kubeNamespaceId', kubeResourceNamespaceLeaf: '/clusters/:rootClusterId/leaves/:leafClusterId/kubes/:kubeId/namespaces/:kubeNameSpaceId',

This follows the existing conventions that we have while introducing a new one. Nested identifiers are prefixed with "parent's" name, e.g. it's :kubeNamespaceId, not :namespaceId.

This is mostly due to how matchPath works and how we implemented parseUri. You give it a bunch of possible identifiers and then pluck out what you need. So if we end up having two URIs, I think we'd rather have two different fields prefixed with the parent name, rather than reusing the same field name for two different purposes.

I wouldn't bother with KubeResourceNamespaceParam for now. If we wanted to have better types around this, we should refactor how routing.parseUri works and all the helper functions like routing.parseServerUri.

On the Go side, AppendKubeResourceNamespace as you have it is fine, we'd only have to change the actual URI it generates.

gzdunek · 2024-09-19T09:56:55Z

lib/teleterm/daemon/daemon.go

+	proxyGRPCClient, err := tc.NewKubernetesServiceClient(ctx, cluster.Name)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}


It seems to me this should be wrapped in clusters.AddMetadataToRetryableError too.

gzdunek · 2024-09-19T09:59:22Z

lib/teleterm/daemon/daemon.go

+		if err != nil {
+			return trace.Wrap(err)
+		}
+
+		return nil


Suggested change

if err != nil {

return trace.Wrap(err)

}

return nil

return trace.Wrap(err)

gzdunek · 2024-09-19T10:27:26Z

lib/teleterm/api/uri/uri.go

+// AppendKubeResource appends kube resource segment to the URI.
+// Modeled after how access request constructs the resource ID:
+// <teleport-cluster-name>/<subresource kind>/<kube_cluster name>/<subresource name>
+func (r ResourceURI) AppendKubeResource(kubeResourceKind string, kubeClusterName string, kubeResourceName string) ResourceURI {
+	r.path = fmt.Sprintf("%v/%v/%v/%v", r.path, kubeResourceKind, kubeClusterName, kubeResourceName)
+	return r
+}
+


Yeah, URIs are used for identifying resources.

fmt.Sprintf("%v/%v/%v/%v", r.path, kubeResourceKind, kubeClusterName, kubeResourceName)

I see some problems with this scheme. kubeResourceKind is an unknown string, so in theory, it could collide with other URIs. Additionally, we would not be able to identify if a given URI string is a kube resource URI without knowing kubeResourceKind.
A fix for this would be adding a known prefix in the URI, e.g:

"/clusters/teleport.sh/kube-resources/namespace/kube-cluster-name/namespace-name"

However, perhaps it would make more sense to treat a kube resource as a child of the kube cluster? I mean something like this:

var pathKubes = urlpath.New("/clusters/:cluster/kubes/:kubeName") var pathKubeResources = urlpath.New("/clusters/:cluster/kubes/:kubeName/:subResourceKind/:subResourceName")

cc @ravicious

web/packages/teleterm/src/ui/uri.ts

ravicious · 2024-09-20T13:30:54Z

lib/teleterm/api/uri/uri.go

+// AppendKubeResource appends kube resource segment to the URI.
+// Modeled after how access request constructs the resource ID:
+// <teleport-cluster-name>/<subresource kind>/<kube_cluster name>/<subresource name>
+func (r ResourceURI) AppendKubeResource(kubeResourceKind string, kubeClusterName string, kubeResourceName string) ResourceURI {
+	r.path = fmt.Sprintf("%v/%v/%v/%v", r.path, kubeResourceKind, kubeClusterName, kubeResourceName)
+	return r
+}
+


If it's correct that kube subresources always belong to a specific kube resource, and I believe it's correct, then I'd definitely go with the second option that Grzegorz proposed.

If we currently support only namespaces within the app, then for now I'd make a definition only for namespaces. That would be:

kubeResourceNamespace: '/clusters/:rootClusterId/(leaves)?/:leafClusterId?/kubes/:kubeId/namespaces/:kubeNamespaceId', kubeResourceNamespaceLeaf: '/clusters/:rootClusterId/leaves/:leafClusterId/kubes/:kubeId/namespaces/:kubeNameSpaceId',

This follows the existing conventions that we have while introducing a new one. Nested identifiers are prefixed with "parent's" name, e.g. it's :kubeNamespaceId, not :namespaceId.

This is mostly due to how matchPath works and how we implemented parseUri. You give it a bunch of possible identifiers and then pluck out what you need. So if we end up having two URIs, I think we'd rather have two different fields prefixed with the parent name, rather than reusing the same field name for two different purposes.

I wouldn't bother with KubeResourceNamespaceParam for now. If we wanted to have better types around this, we should refactor how routing.parseUri works and all the helper functions like routing.parseServerUri.

On the Go side, AppendKubeResourceNamespace as you have it is fine, we'd only have to change the actual URI it generates.

ravicious · 2024-09-20T13:32:42Z

proto/teleport/lib/teleterm/v1/service.proto

+}
+
+message ListKubernetesResourcesResponse {
+  repeated KubeResource items = 1;


Should we call it resources perhaps? That's what ListUnifiedResources already does. I don't see any other RPC within this service that would use items as the field name.

ravicious · 2024-09-20T13:34:00Z

proto/teleport/lib/teleterm/v1/service.proto

+
+message ListKubernetesResourcesResponse {
+  repeated KubeResource items = 1;
+  string start_key = 2;


This should probably be next_key, just to keep the convention started by unified resources.

ravicious · 2024-09-20T13:41:16Z

lib/teleterm/apiserver/handler/handler_kubes.go

 	"github.com/gravitational/teleport/lib/teleterm/clusters"
+	"github.com/gravitational/teleport/lib/web/ui"


I'd say let's avoid bringing lib/web into lib/teleterm. Grepping through lib/teleterm, it's not something that we do today.

If we need to reuse some functions between lib/web and lib/teleterm, I think a better option would to move them to a third package that can be used by both web and teleterm.

What do you think about creating lib/ui/labels.go and moving MakeUILabelsWithoutInternalPrefixes there? Then we could even drop UI from its name.

ravicious · 2024-09-20T13:47:22Z

lib/teleterm/daemon/daemon.go

+	var proxyGRPCClient kubeproto.KubeServiceClient
+
+	err = clusters.AddMetadataToRetryableError(ctx, func() error {
+		proxyGRPCClient, err = tc.NewKubernetesServiceClient(ctx, cluster.Name)


Nit: We can put both calls under the same AddMetadataToRetryableError call, it results in the same behavior and it's slightly shorter.

ravicious · 2024-09-20T13:47:56Z

proto/teleport/lib/teleterm/v1/kube.proto

+  // note:
+  //  this field will be blank if this resource "kind" is "namespace",
+  //  refer to field "name" for the name of namespace


Suggested change

// note:

// this field will be blank if this resource "kind" is "namespace",

// refer to field "name" for the name of namespace

// note: this field will be blank if this resource "kind" is "namespace",

// refer to field "name" for the name of namespace

kimlisa added 3 commits September 19, 2024 00:41

Rename makeLabels into MakeUILabelsWithoutInternalPrefixes

31dbf1a

Teleterm: add listing kube resource endpoint

3b2818b

Teleterm pb updates

ca6a136

kimlisa requested review from gzdunek and ravicious September 19, 2024 07:45

github-actions bot requested review from kiosion and ryanclark September 19, 2024 07:46

github-actions bot added size/md ui labels Sep 19, 2024

This comment was marked as resolved.

Sign in to view

kimlisa commented Sep 19, 2024

View reviewed changes

kimlisa added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Sep 19, 2024

kimlisa removed request for ryanclark and kiosion September 19, 2024 07:47

gzdunek reviewed Sep 19, 2024

View reviewed changes

Address crs

cde17d4

kimlisa requested a review from gzdunek September 20, 2024 04:37

ravicious reviewed Sep 20, 2024

View reviewed changes

gzdunek mentioned this pull request Sep 20, 2024

Add missing @types/react-router #46802

Merged

Address CRs

5ee774f

kimlisa force-pushed the lisa/teleterm-kube-resource-endpoint branch from 86815ca to 5ee774f Compare September 23, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teleterm: Add listing kube resources endpoint #46753

Teleterm: Add listing kube resources endpoint #46753

kimlisa commented Sep 19, 2024 •

edited

Loading

This comment was marked as resolved.

kimlisa Sep 19, 2024

gzdunek Sep 19, 2024

kimlisa Sep 20, 2024

ravicious Sep 20, 2024

gzdunek Sep 19, 2024

gzdunek Sep 19, 2024

gzdunek Sep 19, 2024

ravicious Sep 20, 2024

ravicious Sep 20, 2024

ravicious Sep 20, 2024

ravicious Sep 20, 2024

ravicious Sep 20, 2024

ravicious Sep 20, 2024

		"github.com/gravitational/teleport/lib/teleterm/clusters"
		"github.com/gravitational/teleport/lib/web/ui"

Teleterm: Add listing kube resources endpoint #46753

Are you sure you want to change the base?

Teleterm: Add listing kube resources endpoint #46753

Conversation

kimlisa commented Sep 19, 2024 • edited Loading

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa commented Sep 19, 2024 •

edited

Loading