FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix filesystem permission parity by joe4dev · Pull Request #22 · localstack/lambda-runtime-init · GitHub

/ lambda-runtime-init Public
forked from aws/aws-lambda-runtime-interface-emulator

Fix filesystem permission parity - #22

Merged
joe4dev merged 3 commits into
localstackfrom
fix-filesystem-permission-parity
Oct 16, 2023
ConversationCommits (3)ChecksFiles changed
Merged

Fix filesystem permission parity#22
joe4dev merged 3 commits into
localstackfrom
fix-filesystem-permission-parity

Conversation

joe4dev commented Oct 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Motivation

Addresses localstack/localstack#8897

Changes

  • Recursively change permissions for /opt (layers directory) to 0755
  • Recursively (?) change permissions for /tmp to 0700
  • Add a file utils function ChmodRecursively

Unrelated changes:

  • Add error handling for dropping privileges
  • Rename variable that shows import

Testing

Run the test tests.aws.services.lambda_.test_lambda.TestLambdaLayerBehavior.test_layer_permissions in https://github.com/localstack/localstack-ext/pull/2165 against this new Go binary.

joe4dev added the bug Something isn't working label Oct 4, 2023
joe4dev self-assigned this Oct 4, 2023
joe4dev mentioned this pull request Oct 4, 2023
Merged
1 task
joe4dev removed the bug Something isn't working label Oct 4, 2023
joe4dev commented Oct 4, 2023
View reviewed changes
Comment thread cmd/localstack/main.go
log.Warnln("Could not change file mode recursively of directory /opt:", err)
}
// fix permissions of the tmp directory for better AWS parity
if err := ChmodRecursively("/tmp", 0700); err != nil {

joe4dev Oct 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we do that for the directory only assuming that in ephemeral environments /tmp should be empty 🤔 ?
I guess that's mostly relevant for custom worker scenarios.

dfangl Oct 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think in custom worker scenarios we might want to clear the /tmp directory anyway?

joe4dev Oct 10, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I fully agree 👍 . Assuming an empty /tmp directory seems fair.

Hence, it doesn't matter too much.

dominikschubert Oct 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

FYI custom worker currently cleans /tmp, /var/task and /opt

joe4dev mentioned this pull request Oct 16, 2023
Merged
3 tasks
joe4dev merged commit 605e17d into localstack Oct 16, 2023
joe4dev deleted the fix-filesystem-permission-parity branch October 16, 2023 13:43
joe4dev mentioned this pull request Oct 18, 2023
Closed
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dfangl dfangl approved these changes
dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

Assignees

joe4dev

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL