Skip to content

Convert user tasks and user login state to new cache collections#54491

Merged
rosstimothy merged 1 commit intomasterfrom
tross/cache_user
May 6, 2025
Merged

Convert user tasks and user login state to new cache collections#54491
rosstimothy merged 1 commit intomasterfrom
tross/cache_user

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

Moves the resources to the new cache collection scheme that was introduced in #52210. No additional functionality changes have been made here. This should be a purely mechanical translation to the new internal caching machinery.

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label May 2, 2025
@rosstimothy rosstimothy marked this pull request as ready for review May 2, 2025 20:20
// Clone returns a copy of the member.
func (u *UserLoginState) Clone() *UserLoginState {
var copy *UserLoginState
utils.StrictObjectToStruct(u, &copy)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does StrictObjectToStruct export a pointer to a pointer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I shamelessly copied this from access lists. It looks like all existing variants of this resource style use the same pattern though. So if it's not correct we need to fix it everywhere.

func (a *AccessList) CloneResource() types.ResourceWithLabels {
var copy *AccessList
utils.StrictObjectToStruct(a, &copy)
return copy
}

func (a *DiscoveryConfig) CloneResource() types.ResourceWithLabels {
var copy *DiscoveryConfig
utils.StrictObjectToStruct(a, &copy)
return copy
}

func (a *ExternalAuditStorage) Clone() *ExternalAuditStorage {
var copy *ExternalAuditStorage
utils.StrictObjectToStruct(a, &copy)
return copy
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like we just pass it to (json.Decoder).Decode so we really don't need the double pointer, though it will work just fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we changed this to

	var copy UserLoginState
	utils.StrictObjectToStruct(u, &copy)
	return &copy

then ((*UserLoginState)(nil)).Clone() will be equivalent to &UserLoginState{}, right now it returns (*UserLoginState)(nil).

Obviously neither option is in any way sane and we should write or derive some clone function - and awalterschulze/goderive keeps distinguishing itself in making the entirely wrong choices once again, since it will unconditionally use unsafe and reflect to clone private fields of types which is absolutely wild - but in the meantime the way StrictObjectToStruct is being used is the least worst way.

@rosstimothy rosstimothy requested review from fspmarshall and zmb3 May 5, 2025 22:20
Moves the resources to the new cache collection scheme that was
introduced in #52210. No additional functionality changes have been
made here. This should be a purely mechanical translation to the
new internal caching machinery.
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from flyinghermit May 6, 2025 15:32
@rosstimothy rosstimothy enabled auto-merge May 6, 2025 15:32
@rosstimothy rosstimothy added this pull request to the merge queue May 6, 2025
Merged via the queue into master with commit 4452b07 May 6, 2025
41 checks passed
@rosstimothy rosstimothy deleted the tross/cache_user branch May 6, 2025 16:25
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.

4 participants