Update AccessList Proto, AccessList & AccessListMember types, add Hierarchy utils #47828

kiosion · 2024-10-22T20:16:54Z

Add Hierarchy utils for nested AccessList validation and handling
Update AccessList Protos with new fields
Update AccessList and AccessListMember types and converters with new fields

Split from #38738.

aws-amplify-us-west-2 · 2024-10-22T20:19:49Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-47828.d3pp5qlev8mo18.amplifyapp.com

github-actions · 2024-10-22T21:09:19Z

🤖 Vercel preview here: https://docs-b8sev9aux-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-10-24T00:42:25Z

🤖 Vercel preview here: https://docs-8mejpmy5t-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-10-24T19:10:12Z

🤖 Vercel preview here: https://docs-ia3ox80t5-goteleport.vercel.app/docs/ver/preview

public-teleport-github-review-bot · 2024-10-24T19:26:15Z

@kiosion - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

github-actions · 2024-10-24T19:51:35Z

🤖 Vercel preview here: https://docs-kpk92pprv-goteleport.vercel.app/docs/ver/preview

tigrato · 2024-10-25T13:38:10Z

api/proto/teleport/accesslist/v1/accesslist.proto

@@ -71,6 +71,12 @@ message AccessListSpec {

  // owner_grants describes the access granted by owners to this Access List.
  AccessListGrants owner_grants = 11;
+
+  // owner_of describes Access Lists that this Access List is an explicit owner of.
+  repeated string owner_of = 12;


Do we need to keep track of this information for every access list?

My concern here is that AL being imported by sync utils will have to preserve these fields when they grant access to Teleport access lists.
If we don't need to specify these here and can compute them when needed, it would be much better.

yes, these need to be tracked for every list. I don't love either option... computing when needed is what was initially implemented, but it was much more expensive and required passing around all existing access lists for checks

Can we store it under different keys if the goal is to keep it internal?

Or move it to the status field

Status aren't meant to be edited by users and you can always keep the state there. We can probably keep some logic to keep it when reconcilers run

Is the status field persisted?

Yes, we persist status fields and the operator/terraform skips them IIRC

No objections from me to moving these fields to status as users aren't supposed to set them.

Moved them into status, it wasn't persisted previously so I named the json/yaml field as status and set the member/nested list count fields within it to -. LMK if it looks good

docs/pages/reference/operator-resources/resources.teleport.dev_accesslists.mdx

github-actions · 2024-10-25T23:06:54Z

🤖 Vercel preview here: https://docs-971v2tura-goteleport.vercel.app/docs/ver/preview

smallinsky · 2024-10-25T12:55:37Z

api/types/accesslist/accesslist.go

+	if a.Spec.OwnerOf == nil {
+		a.Spec.OwnerOf = []string{}
+	}
+
+	if a.Spec.MemberOf == nil {
+		a.Spec.MemberOf = []string{}
+	}


Why the code flow distinguish between nil and []string{} array ?

smallinsky · 2024-10-28T09:47:29Z

api/proto/teleport/accesslist/v1/accesslist.proto

+  // membership_kind describes the type of membership, either
+  // `MEMBERSHIP_KIND_USER` or `MEMBERSHIP_KIND_LIST`.
+  MembershipKind membership_kind = 9;


Seems like according to proto field order it should field nr. 8

Suggested change

// membership_kind describes the type of membership, either

// `MEMBERSHIP_KIND_USER` or `MEMBERSHIP_KIND_LIST`.

MembershipKind membership_kind = 9;

// membership_kind describes the type of membership, either

// `MEMBERSHIP_KIND_USER` or `MEMBERSHIP_KIND_LIST`.

MembershipKind membership_kind = 8;

iirc there was a reason it was set to field 9, let me see if I can find it..

smallinsky · 2024-10-28T09:52:41Z

api/types/accesslist/accesslist.go

-	if len(a.Spec.Grants.Roles) == 0 && len(a.Spec.Grants.Traits) == 0 {
-		return trace.BadParameter("grants must specify at least one role or trait")
-	}


Does this validation change is intended ?

This change will allow to create a access list without any grants role and traits role.

Yes, creating a list without any roles or traits was part of the initial RFD

lib/accesslists/hierarchy.go

- Add Hierarchy utils for nested AccessList validation and handling - Update AccessList Protos with new fields - Update AccessList and AccessListMember types and converters with new fields

github-actions · 2024-10-28T22:23:16Z

🤖 Vercel preview here: https://docs-om8ctwyn8-goteleport.vercel.app/docs/ver/preview

tigrato · 2024-10-29T10:45:05Z

api/proto/teleport/accesslist/v1/accesslist.proto

@@ -268,6 +287,12 @@ message ReviewChanges {

 // AccessListStatus contains dynamic fields calculated during retrieval.
 message AccessListStatus {
-  // member_count is the number of members in the in the Access List.
+  // member_count is the number of members in the Access List.
  optional uint32 member_count = 1;


is this the total number of users including users that have access to it through nested access lists or just direct access list memberships?

just direct memberships.

Can we make it clear?

kiosion added the no-changelog Indicates that a PR does not require a changelog entry label Oct 22, 2024

kiosion force-pushed the maxim/nested-acl-protos branch from 48a0366 to aac30cf Compare October 22, 2024 20:30

kiosion temporarily deployed to vercel October 22, 2024 20:30 — with GitHub Actions Inactive

This comment was marked as outdated.

Sign in to view

kiosion force-pushed the maxim/nested-acl-protos branch from aac30cf to 9f961ba Compare October 22, 2024 20:45

kiosion had a problem deploying to vercel October 22, 2024 20:45 — with GitHub Actions Failure

kiosion force-pushed the maxim/nested-acl-protos branch from 9f961ba to e2eafd8 Compare October 22, 2024 20:58

kiosion temporarily deployed to vercel October 22, 2024 20:58 — with GitHub Actions Inactive

kiosion force-pushed the maxim/nested-acl-protos branch from e2eafd8 to d31db51 Compare October 24, 2024 00:31

kiosion temporarily deployed to vercel October 24, 2024 00:31 — with GitHub Actions Inactive

kiosion force-pushed the maxim/nested-acl-protos branch from d31db51 to 3acb03d Compare October 24, 2024 18:59

kiosion temporarily deployed to vercel October 24, 2024 18:59 — with GitHub Actions Inactive

kiosion marked this pull request as ready for review October 24, 2024 19:25

github-actions bot requested review from bernardjkim, hugoShaka, marcoandredinis, mmcallister, ptgott, r0mant, tigrato, xinding33 and zmb3 October 24, 2024 19:26

github-actions bot added documentation helm size/xl labels Oct 24, 2024

r0mant requested review from smallinsky and fspmarshall and removed request for marcoandredinis, mmcallister, bernardjkim, hugoShaka, xinding33, ptgott and tigrato October 25, 2024 01:29

tigrato reviewed Oct 25, 2024

View reviewed changes

fspmarshall approved these changes Oct 25, 2024

View reviewed changes

docs/pages/reference/operator-resources/resources.teleport.dev_accesslists.mdx Outdated Show resolved Hide resolved

kiosion force-pushed the maxim/nested-acl-protos branch from 4d5bd8a to 111c516 Compare October 25, 2024 22:56

kiosion temporarily deployed to vercel October 25, 2024 22:56 — with GitHub Actions Inactive

smallinsky reviewed Oct 28, 2024

View reviewed changes

feat: Update AccessList Proto with new fields, add utils

a07fb04

- Add Hierarchy utils for nested AccessList validation and handling - Update AccessList Protos with new fields - Update AccessList and AccessListMember types and converters with new fields

kiosion force-pushed the maxim/nested-acl-protos branch from 111c516 to a07fb04 Compare October 28, 2024 22:12

kiosion temporarily deployed to vercel October 28, 2024 22:12 — with GitHub Actions Inactive

r0mant mentioned this pull request Oct 29, 2024

Update nested Access List RFD #47651

Merged

smallinsky approved these changes Oct 29, 2024

View reviewed changes

tigrato approved these changes Oct 29, 2024

View reviewed changes

r0mant approved these changes Oct 29, 2024

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from zmb3 October 29, 2024 15:54

kiosion added this pull request to the merge queue Oct 30, 2024

Merged via the queue into master with commit a0540c5 Oct 30, 2024
46 checks passed

kiosion deleted the maxim/nested-acl-protos branch October 30, 2024 20:19

kiosion mentioned this pull request Oct 30, 2024

[v17] Update AccessList Proto, AccessList & AccessListMember types, add Hierarchy utils #48184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update AccessList Proto, AccessList & AccessListMember types, add Hierarchy utils #47828

Update AccessList Proto, AccessList & AccessListMember types, add Hierarchy utils #47828

kiosion commented Oct 22, 2024

aws-amplify-us-west-2 bot commented Oct 22, 2024

This comment was marked as outdated.

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

public-teleport-github-review-bot bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

tigrato Oct 25, 2024

tigrato Oct 25, 2024

kiosion Oct 25, 2024

tigrato Oct 25, 2024

tigrato Oct 25, 2024

kiosion Oct 26, 2024

tigrato Oct 28, 2024 •

edited

Loading

r0mant Oct 28, 2024

kiosion Oct 29, 2024

github-actions bot commented Oct 25, 2024

smallinsky Oct 25, 2024

smallinsky Oct 28, 2024

kiosion Oct 28, 2024

smallinsky Oct 28, 2024

kiosion Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024

tigrato Oct 29, 2024

kiosion Oct 29, 2024

tigrato Oct 30, 2024

Update AccessList Proto, AccessList & AccessListMember types, add Hierarchy utils #47828

Update AccessList Proto, AccessList & AccessListMember types, add Hierarchy utils #47828

Conversation

kiosion commented Oct 22, 2024

aws-amplify-us-west-2 bot commented Oct 22, 2024

This comment was marked as outdated.

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

public-teleport-github-review-bot bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

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

tigrato Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2024

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

kiosion Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrato Oct 28, 2024 •

edited

Loading

kiosion Oct 28, 2024 •

edited

Loading