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

[v11] Don't prematurely close context in app service. #20437

Merged
merged 3 commits into from
Jan 20, 2023

Conversation

mdwn
Copy link
Contributor

@mdwn mdwn commented Jan 19, 2023

The app service was prematurely closing the context for the AWS signer handler, causing tsh aws requests to fail with a context cancel. This only affects v11.

The app service was prematurely closing the context for the AWS signer
handler, causing `tsh aws` requests to fail with a context cancel.
@mdwn mdwn force-pushed the mike.wilson/dont-prematurely-close-context branch from e2f540b to 80eb678 Compare January 19, 2023 20:59
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Has this regressed recently? Do we know where it broke and why it wasn't caught by unit tests (and can we add test coverage here)?

@mdwn
Copy link
Contributor Author

mdwn commented Jan 19, 2023

Has this regressed recently? Do we know where it broke and why it wasn't caught by unit tests (and can we add test coverage here)?

Yes, this is a recent regression. The code here has changed recently due to the Azure CLI changes and some of this code was backported to v11 to make future backports easier, but it got a little clobbered in the process.

This is a little tough to introduce tests for because the issue doesn't even crop up until attempting to use the AWS signer handler, which we don't have any integration tests for. IMO in order to catch something like this we'd need a test that actually deploys a Teleport cluster to AWS and attempts to run tsh aws commands against it.

One of my biggest worries with app access is the need for more black box testing that uses Teleport as a user would. It's going to be increasingly hard to mimic these sorts of environments as app access grows more complex.

We could introduce tests in here that query the context for each sub-handler for app access, but it feels like the wrong way to go about detecting this problem IMO.

@r0mant
Copy link
Collaborator

r0mant commented Jan 20, 2023

@capnspacehook @rosstimothy Could one of you take a look as well when you get a chance? We should release this quickly in a patch release, combined with the other AWS access fix Mike has.

@github-actions github-actions bot removed the request for review from capnspacehook January 20, 2023 14:07
@mdwn mdwn enabled auto-merge (squash) January 20, 2023 14:11
@mdwn mdwn merged commit b2bc2ed into branch/v11 Jan 20, 2023
@zmb3 zmb3 deleted the mike.wilson/dont-prematurely-close-context branch May 7, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants