Remove assigning by default first alphabetical kube cluster to a user. by AntonAM · Pull Request #37242 · gravitational/teleport

AntonAM · 2024-01-25T05:30:48Z

As a leftover of initial kube access implementation, we were setting first alphabetical kube cluster to a user's certificate, even if they didn't have access to it. It lead to unnecessary leaking of a name of existing kube cluster. This PR removes assignment of a default cluster and we only check if specified cluster is available. Also removes redundant check from kube forwarder, since kube cluster name should never be empty there (and kube cluster presence is checked further in the code).

Changelog: Do not add alphabetically first Kube cluster's name to a user certificate on login.

Fixes https://github.com/gravitational/teleport-private/issues/1319

tigrato

This looks good to me.
Did you tested with legacy kubernetes proxy?

tigrato · 2024-01-25T11:59:18Z

@@ -740,21 +739,6 @@ func (f *Forwarder) setupContext(
 	}

 	kubeCluster := identity.KubernetesCluster


this block can move the the var now that's not used anymore

It's still useful as a shorthand, since it's used several times down the code.

rosstimothy · 2024-01-25T20:57:50Z

Suggested change

return trace.BadParameter("Kubernetes cluster %q is not registered in this teleport cluster; you can list registered kubernetes clusters using 'tsh kube ls'", kubeClusterName)

return trace.BadParameter("kubernetes cluster %q is not registered in this teleport cluster; you can list registered kubernetes clusters using 'tsh kube ls'", kubeClusterName)

In this case it's capitalization of a proper name.

How come we aren't capitalizing Kubernetes the second time it appears in the message then?

Oh, we should. 😅 I moved the error and didn't notice it had second usage down the text. Fixed.

AntonAM · 2024-01-26T09:53:34Z

@tigrato yep, I tried with legacy kube proxy config and it works.

AntonAM · 2024-01-26T09:54:02Z

Flaky test detector is failing because of 10 minutes timeout.

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

AntonAM · 2024-01-29T20:13:20Z

@r0mant @zmb3 can I get a flaky tests skip? It always gets timeout.

zmb3 · 2024-01-29T20:20:23Z

/excludeflake *

public-teleport-github-review-bot · 2024-01-29T23:09:45Z

@AntonAM See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed
branch/v15	Create PR

#37242) * Remove assigning by default first alphabetical kube cluster to a user. * Fix tests. * Address review comments * Fix tests. * Fix error message. * Use default value. Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * Fix error message. Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

#37242) (#37503) * Remove assigning by default first alphabetical kube cluster to a user. * Fix tests. * Address review comments * Fix tests. * Fix error message. * Use default value. * Fix error message. --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

#37242) (#37501) * Remove assigning by default first alphabetical kube cluster to a user. * Fix tests. * Address review comments * Fix tests. * Fix error message. * Use default value. * Fix error message. --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

AntonAM added kubernetes-access backport/branch/v13 labels Jan 25, 2024

AntonAM force-pushed the anton/remove-default-kube-cluster branch 3 times, most recently from 2503915 to f43e781 Compare January 25, 2024 08:09

AntonAM requested a review from tigrato January 25, 2024 08:10

AntonAM marked this pull request as ready for review January 25, 2024 08:10

github-actions Bot requested review from kimlisa and lxea January 25, 2024 08:10

github-actions Bot added size/sm tctl tctl - Teleport admin tool labels Jan 25, 2024

tigrato reviewed Jan 25, 2024

View reviewed changes

AntonAM force-pushed the anton/remove-default-kube-cluster branch from 0e9258f to 6484994 Compare January 25, 2024 20:10

rosstimothy reviewed Jan 25, 2024

View reviewed changes

AntonAM force-pushed the anton/remove-default-kube-cluster branch from 6484994 to 33e61ff Compare January 25, 2024 21:03

kimlisa removed their request for review January 26, 2024 03:45

AntonAM force-pushed the anton/remove-default-kube-cluster branch 3 times, most recently from 33ef58e to cad4924 Compare January 26, 2024 05:19

AntonAM requested review from rosstimothy and tigrato January 26, 2024 17:27

AntonAM force-pushed the anton/remove-default-kube-cluster branch from bdf1f39 to 0b9d5b8 Compare January 26, 2024 18:52

rosstimothy approved these changes Jan 26, 2024

View reviewed changes

Comment thread tool/tctl/common/auth_command_test.go Outdated

Comment thread lib/kube/proxy/forwarder.go Outdated

tigrato approved these changes Jan 29, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from lxea January 29, 2024 09:29

Remove assigning by default first alphabetical kube cluster to a user.

804bd5c

AntonAM and others added 6 commits January 29, 2024 14:50

Fix tests.

68a7da2

Address review comments

daa9749

Fix tests.

b12b732

Fix error message.

2fe8682

Use default value.

ba0e10c

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

Fix error message.

f9f4bfc

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

AntonAM force-pushed the anton/remove-default-kube-cluster branch from 97ef1f1 to f9f4bfc Compare January 29, 2024 19:50

AntonAM enabled auto-merge January 29, 2024 20:47

AntonAM added this pull request to the merge queue Jan 29, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 29, 2024

AntonAM added this pull request to the merge queue Jan 29, 2024

Merged via the queue into master with commit a576cb2 Jan 29, 2024

AntonAM deleted the anton/remove-default-kube-cluster branch January 29, 2024 23:08

AntonAM mentioned this pull request Jan 30, 2024

[v15] Remove assigning by default first alphabetical kube cluster to a user. #37495

Merged

AntonAM mentioned this pull request Jan 30, 2024

[v14] Remove assigning by default first alphabetical kube cluster to a user. #37501

Merged

AntonAM mentioned this pull request Jan 30, 2024

[v13] Remove assigning by default first alphabetical kube cluster to a user. #37503

Merged

		@@ -740,21 +739,6 @@ func (f *Forwarder) setupContext(
		}

		kubeCluster := identity.KubernetesCluster

	return trace.BadParameter("Kubernetes cluster %q is not registered in this teleport cluster; you can list registered kubernetes clusters using 'tsh kube ls'", kubeClusterName)
	return trace.BadParameter("kubernetes cluster %q is not registered in this teleport cluster; you can list registered kubernetes clusters using 'tsh kube ls'", kubeClusterName)

Conversation

AntonAM commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tigrato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tigrato Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

AntonAM Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

AntonAM Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

rosstimothy Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

AntonAM Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AntonAM commented Jan 26, 2024

Uh oh!

AntonAM commented Jan 26, 2024

Uh oh!

Uh oh!

Uh oh!

AntonAM commented Jan 29, 2024

Uh oh!

zmb3 commented Jan 29, 2024

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Jan 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AntonAM commented Jan 25, 2024 •

edited

Loading