Skip to content

refactor(pkg/archive): factor out createImpliedDirectories helper #44196

Merged
thaJeztah merged 2 commits intomoby:masterfrom
neersighted:createImpliedDirectories
Sep 27, 2022
Merged

refactor(pkg/archive): factor out createImpliedDirectories helper #44196
thaJeztah merged 2 commits intomoby:masterfrom
neersighted:createImpliedDirectories

Conversation

@neersighted
Copy link
Copy Markdown
Member

@neersighted neersighted commented Sep 26, 2022 •
edited
Loading

- What I did

  • Factor out common 'implied' directory creation code in pkg/archive into a helper.
  • Update the non-diff/overlay codepath to use the same permissions mask as the (more common) overlay codepath.
  • Document why the constant 0755 was chosen, and why image authors cannot currently rely on this value given it is not standardized/is fully implementation-defined.
  • Add tests to ensure the constant is used and directories are created with the expected permissions.

- How to verify it
CI

- Description for the changelog
Update the default permissions for directories implied but not explicitly defined by a layer to 0755 across all graphdrivers.

- A picture of a cute animal (not mandatory but encouraged)
image

@neersighted neersighted added status/2-code-review impact/changelog area/images Image Service kind/refactor PR's that refactor, or clean-up code labels Sep 26, 2022
@neersighted neersighted added this to the v-next milestone Sep 26, 2022
@neersighted
Copy link
Copy Markdown
Member Author

neersighted commented Sep 26, 2022

For additional context regarding why this value is presently unreliable/implementation defined, see #44106 (comment).

@neersighted neersighted force-pushed the createImpliedDirectories branch from 3b119a2 to 395f455 Compare September 26, 2022 14:56
corhere
corhere reviewed Sep 26, 2022
View reviewed changes
Comment thread pkg/archive/archive.go
Comment thread pkg/archive/archive.go Outdated
Comment thread pkg/archive/archive.go Outdated
Comment thread pkg/archive/archive_test.go Outdated
Comment thread pkg/archive/archive_test.go Outdated
Comment thread pkg/archive/archive_test.go Outdated
Comment thread pkg/archive/archive_test.go Outdated
@neersighted @vasiliy-ul
refactor(pkg/archive): factor out createImpliedDirectories helper
4831ff9 
This code was duplicated in two places -- factor it out, add
documentation, and move magic numbers into a constant.

Additionally, use the same permissions (0755) in both code paths, and
ensure that the ID map is used in both code paths.

Co-authored-by: Vasiliy Ulyanov <[email protected]>
Signed-off-by: Bjorn Neergaard <[email protected]>
Signed-off-by: Vasiliy Ulyanov <[email protected]>
@neersighted neersighted force-pushed the createImpliedDirectories branch from 395f455 to 64e5936 Compare September 26, 2022 17:19
@neersighted neersighted requested a review from corhere September 26, 2022 17:20
@neersighted neersighted force-pushed the createImpliedDirectories branch from 64e5936 to 2e64eb6 Compare September 26, 2022 17:43
@neersighted @corhere
test(pkg/archive): add TestImpliedDirectoryPermissions
5dff494 
Co-authored-by: Cory Snider <[email protected]>
Signed-off-by: Bjorn Neergaard <[email protected]>
@neersighted neersighted force-pushed the createImpliedDirectories branch from 2e64eb6 to 5dff494 Compare September 26, 2022 17:46
corhere
corhere approved these changes Sep 26, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

:shipit:

@thaJeztah thaJeztah mentioned this pull request Sep 27, 2022
Merged
thaJeztah
thaJeztah reviewed Sep 27, 2022
View reviewed changes
Comment thread pkg/archive/archive.go
Comment thread pkg/archive/archive.go
This was referenced Sep 27, 2022
Merged
Closed
thaJeztah
thaJeztah approved these changes Sep 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Sep 27, 2022

arf... I edited the top description, but that triggered another run of Jenkins, well 🤷‍♂️

@neersighted
Copy link
Copy Markdown
Member Author

neersighted commented Sep 27, 2022

arf... I edited the top description, but that triggered another run of Jenkins, well 🤷‍♂️

For what it's worth, I already included the issue linking in the body, but at the bottom. I'll adopt the bulleted-on-top style in the future 😅

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Sep 27, 2022

For what it's worth, I already included the issue linking in the body, but at the bottom.

Oh! I completely overlooked.

I nowadays also use the bullet-list formatting (where suitable), which renders the links with the linked description; not always useful, but sometimes saves me from having to "mouse-hover" (or "click") to see the linked issue/

@thaJeztah thaJeztah merged commit a4f3c08 into moby:master Sep 27, 2022
@neersighted neersighted deleted the createImpliedDirectories branch September 27, 2022 17:46
@neersighted
Copy link
Copy Markdown
Member Author

neersighted commented Sep 27, 2022

@thaJeztah I've gone ahead and applied the cherry-pick label since backporting this was mentioned. Feel free to remove it if you conclude that it's not worth backporting (likewise, I am happy to backport on request, but don't have any special interest personally).

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Sep 27, 2022

@neersighted ah, thanks; thought it was on there already; yes, if you have time to prepare a cherry pick, that'd be good 👍

@neersighted neersighted mentioned this pull request Sep 27, 2022
Merged
@neersighted
Copy link
Copy Markdown
Member Author

neersighted commented Sep 27, 2022

@neersighted ah, thanks; thought it was on there already; yes, if you have time to prepare a cherry pick, that'd be good 👍

#44207 is up

@thaJeztah thaJeztah mentioned this pull request Feb 9, 2023
Open
@thaJeztah thaJeztah mentioned this pull request Jun 7, 2023
Closed
myback added a commit to myback/docker_pull that referenced this pull request Nov 6, 2023
@myback
tar header changed after moby [refactoring](moby/moby#44196)
27bea1f
myback added a commit to myback/docker_pull that referenced this pull request Nov 6, 2023
@myback
tar header changed after moby refactoring moby/moby#44196
0157032
myback added a commit to myback/docker_pull that referenced this pull request Nov 6, 2023
@myback
tar header changed after moby refactoring moby/moby#44196
582bc84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corhere corhere corhere approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/images Image Service impact/changelog kind/refactor PR's that refactor, or clean-up code process/cherry-picked status/4-merge

Projects

None yet

Milestone

24.0.0

Development

Successfully merging this pull request may close these issues.

Wrong directory permissions with btrfs or devicemapper storage drivers

3 participants

@neersighted @thaJeztah @corhere