SAMZA-2653: Write pathing jar of run-class.sh into its own directory #1498
Conversation
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
Sanil15
approved these changes
May 6, 2021
Contributor
Sanil15
left a comment
Sanil15
left a comment
There was a problem hiding this comment.
Thanks for the clean up!
Contributor
|
Note that this PR has a bug. See #1716 for followup. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: Currently, the pathing.jar file used to specify the classpath for run-class.sh is written directly into the top-level working directory of the app. When using container images, there may be some predefined permissions set on the working directory. This may cause permission issues with being able to write the pathing.jar file.
Changes: Create a separate directory for writing classpath-related files (
manifest.txt,pathing.jar) and write thepathing.jarto that separate directory. Update the classpath arguments to use the new location of thepathing.jar.Tests:
./bin/integration-tests.sh /tmp/samza-tests yarn-integration-tests --nopasswordProcessJobFactoryand checked that the job started up.API changes, usage/upgrade instructions: N/A
Some more context about a specific use case: When I tried to run a container image of the current code in the Kubernetes environment, I ran into a file permissions issue for writing
manifest.txtandpathing.jar. This was because therun-class.shwas being run by a non-root user, but the container image did not give write access for the main working directory for non-root users. It makes sense that the main working directory from the container image shouldn't be writable by non-root users, because it could potentially contain binaries/scripts installed in the container image which are needed to execute a Samza job. I couldn't mount a volume in order to give writable permissions to the directory, because that would wipe out the content that was installed to that directory in the container image. It could have worked to runrun-class.shas root, but that isn't ideal from a security perspective. By using a separate new directory, it is easier to apply broader permissions to just this new directory by mounting a volume, and it allows the directories from the container image to continue to have more restricted access.