Connect: return logged in user in `ListRootClusters` by gzdunek · Pull Request #20809 · gravitational/teleport

gzdunek · 2023-01-27T12:10:06Z

Closes #20670

This PR adds two integration tests for Connect RPC service to make sure we will not break it accidentally in the future.

To fix the actual issue, I add new cluster type ClusterWithDetails. It is quite similar to the struct that Michael introduced here, but then it was replaced with enriching the Cluster struct during the review.
IMO having one type that is enriched with some methods is problematic. You can't easily tell which fields are available and which are not.
I'm not 100% happy with having RequestableRoles and SuggestedReviewers directly in ClusterWithDetails, but I didn't want to complicate it by adding another UserDetails (?) struct. We can do it later when we have more fields.

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

… with optional properties

gzdunek · 2023-01-27T12:10:35Z

+			config.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Multiplex)
+		}),
+	)
+	pack.WaitForLeaf(t)


TODO: remove leaf cluster

Let's skip that. I was thinking about it but sooner or later we'll need a test which needs a leaf cluster anyway and so far no other test suite would benefit from being able to skip setting up a leaf cluster (AFAIR).

gzdunek · 2023-01-27T12:11:12Z

+	apiCluster := newAPIRootCluster(cluster.Cluster)
+
+	// Only include if they exist on the supplied cluster
 	if cluster.Features != nil {


I think we can get rid of this if now?

yeah seems appropriate now

ravicious · 2023-01-27T14:20:43Z

To fix the actual issue, I add new cluster type ClusterWithDetails. It is quite similar to the struct that Michael introduced here, but then it was replaced with enriching the Cluster struct during the review.
IMO having one type that is enriched with some methods is problematic. You can't easily tell which fields are available and which are not.

We had a discussion about it here: #17497 (comment)

gzdunek · 2023-01-27T16:24:56Z

Thanks for the link, I completely forgot about it.
The discussion there was also about the RPC API which remained unchanged here (I added only a new struct for our "internal" API). Also at that time we had only one optional field and now we have 4 😏. Because of this, it seemed to me that adding a new type made more sense.
But I'm fine with removing the new struct; I'm only wondering where to put suggestedReviewers and requestableRoles (as loggedInUser is quite confusing).

avatus

This is fine and was another option to do it this way originally. I think your logic makes sense that we were adding a few more fields. Honestly, this kind of just comes down to dev preference at this point. I'm ok with it either way and must admit, it's a bit neater/cleaner this way.

avatus · 2023-01-27T21:13:20Z

+	apiCluster := newAPIRootCluster(cluster.Cluster)
+
+	// Only include if they exist on the supplied cluster
 	if cluster.Features != nil {


yeah seems appropriate now

ravicious

I like the new approach, I left some comments mostly related to code comments and small improvements in tests.

ravicious

We should probably backport it to v11 too, don't we? In theory it fixes a "bug" that was introduced in v11.

gzdunek · 2023-02-08T16:43:31Z

@avatus or @lxea ping :)

avatus · 2023-02-08T16:44:29Z

I'm sorry, I thought I approved this last week. Looks awesome

public-teleport-github-review-bot · 2023-02-08T17:02:09Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v11	Failed
branch/v12	Create PR

* Add integration/teleterm_test with tests for get* and list* clusters Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> * Move `testAddingRootCluster to integration/teleterm_test * Add `ClusterWithDetails` type instead of enriching existing `Cluster` with optional properties * Remove unnecessary condition * Add license * Bunch of renames * Do more in mustLogin * Remove watcher --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 7ed92c1)

…21531)

* Add integration/teleterm_test with tests for get* and list* clusters Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> * Move `testAddingRootCluster to integration/teleterm_test * Add `ClusterWithDetails` type instead of enriching existing `Cluster` with optional properties * Remove unnecessary condition * Add license * Bunch of renames * Do more in mustLogin * Remove watcher --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

gzdunek and others added 3 commits January 27, 2023 12:55

Add integration/teleterm_test with tests for get* and list* clusters

b1c114c

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

Move `testAddingRootCluster to integration/teleterm_test

de22ef8

Add ClusterWithDetails type instead of enriching existing Cluster…

e66c4c8

… with optional properties

gzdunek requested review from avatus and ravicious January 27, 2023 12:10

github-actions Bot requested a review from lxea January 27, 2023 12:10

github-actions Bot added the size/md label Jan 27, 2023

gzdunek commented Jan 27, 2023

View reviewed changes

gzdunek added the backport/branch/v12 label Jan 27, 2023

gzdunek changed the title ~~Return logged in user in ListRootClusters~~ Connect: return logged in user in ListRootClusters Jan 27, 2023

avatus reviewed Jan 27, 2023

View reviewed changes

avatus had a problem deploying to reviewers January 27, 2023 21:16 — with GitHub Actions Failure

gzdunek added 4 commits January 31, 2023 14:54

Merge branch 'master' into gzdunek/fix-logged-in-user-in-list-clusters

8866765

Merge branch 'master' into gzdunek/fix-logged-in-user-in-list-clusters

53702a3

Remove unnecessary condition

2cb2721

Add license

5d3792d

ravicious reviewed Feb 6, 2023

View reviewed changes

gzdunek added 4 commits February 7, 2023 10:34

Merge branch 'master' into gzdunek/fix-logged-in-user-in-list-clusters

6682a44

Bunch of renames

54520df

Do more in mustLogin

1e19743

Remove watcher

96f40e5

ravicious approved these changes Feb 8, 2023

View reviewed changes

gzdunek added the backport/branch/v11 label Feb 8, 2023

avatus approved these changes Feb 8, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from lxea February 8, 2023 16:45

gzdunek added this pull request to the merge queue Feb 8, 2023

Merged via the queue into master with commit 7ed92c1 Feb 8, 2023

gzdunek mentioned this pull request Feb 8, 2023

[v12] Connect: return logged in user in ListRootClusters #21467

Merged

gzdunek mentioned this pull request Feb 9, 2023

[v11] Connect: return logged in user in ListRootClusters (#20809) #21531

Merged

gzdunek added a commit that referenced this pull request Feb 15, 2023

[v11] Connect: Return logged in user in ListRootClusters (#20809) (#…

e5274a3

…21531)

gzdunek deleted the gzdunek/fix-logged-in-user-in-list-clusters branch March 20, 2023 16:31

Conversation

gzdunek commented Jan 27, 2023

Uh oh!

gzdunek Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

ravicious Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

gzdunek Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

avatus Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

gzdunek Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

ravicious commented Jan 27, 2023

Uh oh!

gzdunek commented Jan 27, 2023

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

avatus Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

gzdunek commented Feb 8, 2023

Uh oh!

avatus commented Feb 8, 2023

Uh oh!

public-teleport-github-review-bot Bot commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants