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

Extend Layer Packaging for Windows Containers #343

Merged
merged 2 commits into from
Mar 4, 2023

Conversation

Danielku15
Copy link
Contributor

Related Issue: #117

Key change in this PR are:

  1. Handle Windows RIDs in EntryPoint and WorkingDir of images
  2. Change the Layer Tar structure to the one needed for Windows layers with Files/ and Hives/ and not having a drive letter folder.
  3. Ensure we have the a Security Identifiers set as PAX header for the tar entries allowing BUILTIN\Users access to the app folder.
  4. Adopted this remark from my previous rework to pull the EnsureDirectoryEntries into a local function.
  5. I added a test which checks for the expected folder structure of Windows layer. Further tests might be added once there is a Windows Container Runtime available in the CI pipelines. Unfortunately due to Pax Headers incorrectly treated is invalid if value contains an '=' sign runtime#81699 I had to also disable the verification of the Security Identifiers.

Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

The change looks good to me, though tests should be changed as we moved to xunit.
Please let me know if you need help with it.

It would be good to have an e2e test on Windows as you mentioned but it's not possible at the moment. Have you verified the change manually?

Microsoft.NET.Build.Containers/Layer.cs Outdated Show resolved Hide resolved
@Danielku15
Copy link
Contributor Author

The change looks good to me, though tests should be changed as we moved to xunit. Please let me know if you need help with it.

I will take care of the test updates, I saw the separate PR with XUnit but at the time of opening this one it wasn't merged yet 😁

It would be good to have an e2e test on Windows as you mentioned but it's not possible at the moment. Have you verified the change manually?

Good point. I had checked my changes on a different branch and reported my findings here: #117 (comment) but to be sure nothing changed since my initial findings, I will verify the changes again using this branch.

{
RuntimeIdentifier = runtimeIdentifier;
Copy link
Member

Choose a reason for hiding this comment

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

We should not need this - the config JsonNode has the os and architecture properties, which we should be able to use to answer this question appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to use the os of the base image config.

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Thanks @Danielku15 - this LGTM now. This is a great piece of work, I really appreciate you taking the time to dig in and solve this.

@baronfel
Copy link
Member

baronfel commented Mar 3, 2023

@Danielku15 can you resolve conflicts with main?

# Conflicts:
#	Microsoft.NET.Build.Containers/ImageConfig.cs
#	Microsoft.NET.Build.Containers/Layer.cs
@baronfel
Copy link
Member

baronfel commented Mar 4, 2023

Tested this this morning and it works great - we'll need to do some work writing tests that will work on a windows-configured daemon (our default local registry doesn't work, for example), but the core functionality is great.

@baronfel baronfel merged commit ec62929 into dotnet:main Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants