Skip to content

Fix the test auth server's upload handler#57601

Merged
bl-nero merged 1 commit intomasterfrom
bl-nero/test-auth-server-uploader
Aug 10, 2025
Merged

Fix the test auth server's upload handler#57601
bl-nero merged 1 commit intomasterfrom
bl-nero/test-auth-server-uploader

Conversation

@bl-nero
Copy link
Copy Markdown
Contributor

@bl-nero bl-nero commented Aug 6, 2025

The server didn't have its upload handler properly configured, which prevented me from uploading session summaries in unit tests.

Blocks https://github.com/gravitational/teleport.e/pull/7015

@github-actions github-actions Bot requested review from Tener and kopiczko August 6, 2025 13:52
AccessLists: accessLists,
FIPS: cfg.FIPS,
KeyStoreConfig: cfg.KeystoreConfig,
MultipartHandler: uploadHandler,
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.

Does it have to be the same uploader or can it be a fresh instance of eventstest.NewMemoryUploader()?

If the latter then I'd suggest doing so to keep the complexity lower.

Should we set MultipartHandler: eventstest.NewMemoryUploader(), in other tests too?

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.

@Tener I would argue that using the same instance will better reflect the behavior of a real component. The real one is based on a filesystem, and multiple instances — if configured the same way — will share the same space of uploaded files. Since a memory uploader manages its own internal set of "uploads" in memory, using the same one everywhere brings us closer to the behavior of a production system. A more accurate solution would be to implement the upload set separately and provide thin interfaces over it, but this would actually increase complexity instead.

As for other tests — I honestly don't know, and I didn't consider it. If you think that they need to be changed, we can do it separately, as my goal here is to unblock an enterprise PR whose tests depend on it.

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.

Ok, I see your point regarding reusing the instance.

Regarding other tests: I see that MultipartHandler field is very new. @eriktate can you preemptively check if this should be added in other tests that are using auth.InitConfig?

@bl-nero bl-nero added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v18 labels Aug 8, 2025
@bl-nero bl-nero requested a review from Tener August 8, 2025 03:24
@bl-nero bl-nero added this pull request to the merge queue Aug 10, 2025
Merged via the queue into master with commit ae3f2a9 Aug 10, 2025
44 of 45 checks passed
@bl-nero bl-nero deleted the bl-nero/test-auth-server-uploader branch August 10, 2025 13:18
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@bl-nero See the table below for backport results.

Branch Result
branch/v18 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v18 no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants