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

template: disable sandboxed rendering on Windows #23432

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

pkazmierczak
Copy link
Contributor

@pkazmierczak pkazmierczak commented Jun 25, 2024

Following #23443, we no longer need to sandbox template rendering on Windows.

Relates to: #20585
Relates to: #20034
Internal ref: https://hashicorp.atlassian.net/browse/NET-9311

This is part of a solution outlined in RFC NMD-195.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

I think I understand where you're going with this, but the TaskTemplateManagerConfig we're passing in is still having a ReaderFn and RenderFn set. By having those functions just empty return nil, I think we end up not rendering at all because we never read anything. That should be detected by tests on Windows but doesn't seem to be?

I'd probably leave the TaskTemplateManagerConfig constructor in place for both Windows and default, and have the ReaderFn and RenderFn on Windows just call os.Open/Read and CT's own render function directly.

@pkazmierczak pkazmierczak added backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.6.x+ent Changes are backported to 1.6.x+ent labels Jun 27, 2024
@pkazmierczak
Copy link
Contributor Author

pkazmierczak commented Jun 28, 2024

but the TaskTemplateManagerConfig we're passing in is still having a ReaderFn and RenderFn set.

yeah these ReaderFn and RenderFn functions are somewhat misleading. They are only ever used when the sandbox is enabled. So since we no longer support sandboxing on Windows, they should return nil, because ct will default them. (this also solves the mystery why the tests passed)

@pkazmierczak pkazmierczak requested a review from tgross June 28, 2024 12:38
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

yeah these ReaderFn and RenderFn functions are somewhat misleading. They are only ever used when the sandbox is enabled.

Oh right, what I was missing was that the ReaderFn and RenderFn functions are "factory" functions! (Which is hilarious because I wrote all this junk! 🤦) So they're getting called to return a nil function for that configuration, which CT will accept and replace with its own default internal functions.

LGTM!

@pkazmierczak pkazmierczak merged commit 356ea87 into main Jun 28, 2024
19 checks passed
@pkazmierczak pkazmierczak deleted the b-win-app-container-removal branch June 28, 2024 15:16
pkazmierczak added a commit that referenced this pull request Jun 28, 2024
This removes helper winappcontainer and winexec helper code, since it is no longer needed after #23432
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.6.x+ent Changes are backported to 1.6.x+ent backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent backport/1.8.x backport to 1.8.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants