Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conversions to/from decisionpb.TLSIdentity #50308

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

codingllama
Copy link
Contributor

Add conversion methods between tlsca.Identity and decisionpb.TLSIdentity.

#49837

@codingllama codingllama added the no-changelog Indicates that a PR does not require a changelog entry label Dec 16, 2024

// TLSIdentityToTLSCA transforms a [decisionpb.TLSIdentity] into its
// equivalent [tlsca.Identity].
func TLSIdentityToTLSCA(id *decisionpb.TLSIdentity) *tlsca.Identity {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fspmarshall these are very similar to the helpers in our shared branch, with the major exception that I'm passing tlsca.Identity around as a pointer instead of by value - the struct is too big to keep copying around IMO (despite what a large part of the codebase seems to do).

I also don't distinguish between zero and nil for structs within tlsca.Identity, as they aren't distinguished there either. Semantics should be alright in the existing system (for example we'll handle an "empty" DeviceExtensions just fine).

return apitraitconvert.ToProto(apiTraits)
}

func routeToAppFromProto(routeToApp *decisionpb.RouteToApp) tlsca.RouteToApp {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This returns tlsca.RouterToApp by value as that is how tlsca.Identity wants it. The inverse conversion (routeToAppToProto) will both take and return a pointer.

Same for other structs.

@codingllama
Copy link
Contributor Author

Friendly ping reviewers?

Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

Would it be useful to put those conversion functions in api/ ?

Also, are we okay with the fact some fields are not deep copied and that editing the result of the conversion might change the original resource? (I think a few slices are passed as-is). Do we want to mention it in the godoc?

@codingllama
Copy link
Contributor Author

Would it be useful to put those conversion functions in api/ ?

We can move if needed, for now I think this is fine (I don't see api/ code touching the protos here).

Also, are we okay with the fact some fields are not deep copied and that editing the result of the conversion might change the original resource? (I think a few slices are passed as-is). Do we want to mention it in the godoc?

Seems OK too. I'll mention in godocs, that is a good suggestion.

@codingllama
Copy link
Contributor Author

PTAL @hugoShaka.

Friendly ping @fspmarshall @eriktate.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from eriktate December 19, 2024 18:35
@codingllama codingllama added this pull request to the merge queue Dec 19, 2024
Merged via the queue into master with commit 860a9d4 Dec 19, 2024
41 checks passed
@codingllama codingllama deleted the codingllama/pdp-tlsidentity branch December 19, 2024 18:57
@fspmarshall fspmarshall mentioned this pull request Jan 14, 2025
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants