Skip to content

Introduce the Access List object.#28385

Merged
mdwn merged 10 commits intomasterfrom
mike.wilson/access-list
Jul 3, 2023
Merged

Introduce the Access List object.#28385
mdwn merged 10 commits intomasterfrom
mike.wilson/access-list

Conversation

@mdwn
Copy link
Copy Markdown
Contributor

@mdwn mdwn commented Jun 27, 2023

The Access List object, which is the foundational object for access grants, has been introduced. Due to the size/complexity of this object, the implementation in api/types will come in a follow up.

RFD-6E for context

The Access List object, which is the foundational object for access grants,
has been introduced. Due to the size/complexity of this object, the
implementation in api/types will come in a follow up.
@github-actions github-actions Bot requested review from tigrato and zmb3 June 27, 2023 19:02
@tigrato
Copy link
Copy Markdown
Contributor

tigrato commented Jun 27, 2023

@mdwn do we have the reasoning behind this object anywhere?

It's quite difficult to review with no/little knowledge

@mdwn
Copy link
Copy Markdown
Contributor Author

mdwn commented Jun 27, 2023

@mdwn do we have the reasoning behind this object anywhere?

It's quite difficult to review with no/little knowledge

Ahh sorry, I meant to add in a link to the RFD in the PR description. It's been added.

Comment thread api/proto/teleport/legacy/types/types.proto Outdated
Comment thread api/proto/teleport/legacy/types/types.proto Outdated
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Jun 28, 2023

Thanks for breaking this change into a separate PR and for the great comments!

Copy link
Copy Markdown
Contributor

@tigrato tigrato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what I'm going to ask for next is a lot of work but since you're the one who launched the RFD Gogo must go, can we try not to add more uses of Gogo?
Will we support syncing these objects or will they only exist on the auth server?

Comment thread api/proto/teleport/legacy/types/types.proto Outdated
Comment thread api/proto/teleport/legacy/types/types.proto Outdated
@mdwn
Copy link
Copy Markdown
Contributor Author

mdwn commented Jun 28, 2023

I know what I'm going to ask for next is a lot of work but since you're the one who launched the RFD Gogo must go, can we try not to add more uses of Gogo? Will we support syncing these objects or will they only exist on the auth server?

These objects will only exist on the auth server.

So, in theory I agree with you w/r/t not adding more uses of gogo. However, I want to make sure that the gogo RFD has a definitive, agreed upon plan before actually implementing that -- I would hate to implement what I think is a good way around this and then find ourselves having to migrate this one particular object to the outcome of the RFD. Until then, I would rather it be very obvious that this needs migration as well.

@mdwn
Copy link
Copy Markdown
Contributor Author

mdwn commented Jun 28, 2023

Hrm:

# github.com/gravitational/teleport/api/types
api/types/types.pb.go:38448:29: m.Frequency.MarshalToSizedBuffer undefined (type *durationpb.Duration has no field or method MarshalToSizedBuffer)
api/types/types.pb.go:47515:19: m.Frequency.Size undefined (type *durationpb.Duration has no field or method Size)
api/types/types.pb.go:105087:26: m.Frequency.Unmarshal undefined (type *durationpb.Duration has no field or method Unmarshal)

Gogo maybe doesn't like Duration?

@tigrato
Copy link
Copy Markdown
Contributor

tigrato commented Jun 28, 2023

Hrm:

# github.com/gravitational/teleport/api/types
api/types/types.pb.go:38448:29: m.Frequency.MarshalToSizedBuffer undefined (type *durationpb.Duration has no field or method MarshalToSizedBuffer)
api/types/types.pb.go:47515:19: m.Frequency.Size undefined (type *durationpb.Duration has no field or method Size)
api/types/types.pb.go:105087:26: m.Frequency.Unmarshal undefined (type *durationpb.Duration has no field or method Unmarshal)

Gogo maybe doesn't like Duration?

You need to add a directive to the protoc compiler.
I will check that today or tomorrow morning

@mdwn mdwn force-pushed the mike.wilson/access-list branch from c614bf8 to d33bbcf Compare June 28, 2023 20:34
@mdwn
Copy link
Copy Markdown
Contributor Author

mdwn commented Jun 28, 2023

Okay, I've migrated this away from legacy into its own proto file. I had mistakenly thought that there would be an issue with the cache, but I was misremembering the reason that was a concern before.

@mdwn
Copy link
Copy Markdown
Contributor Author

mdwn commented Jun 29, 2023

I've just added in a "common" package to create a header for this object -- I'm attempting to create this object in the "correct" way, so let me know if you have any objections and I can revert this commit.

@mdwn mdwn force-pushed the mike.wilson/access-list branch from 3a50258 to 7325999 Compare June 30, 2023 18:37
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by per Mike's request.

Comment thread api/proto/teleport/common/v1/common.proto Outdated
Comment thread api/proto/teleport/common/v1/common.proto Outdated
@mdwn mdwn enabled auto-merge July 3, 2023 16:48
@mdwn mdwn added this pull request to the merge queue Jul 3, 2023
Merged via the queue into master with commit 22d72d3 Jul 3, 2023
@mdwn mdwn deleted the mike.wilson/access-list branch July 3, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants