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.

Sign up for GitHub

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

Jump to bottom

[Fix-16918][Task] Fix wrong permissions configuration while executing shell #16923

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

[Fix-16918][Task] Fix wrong permissions configuration while executing shell #16923

wants to merge 10 commits into from

Conversation

BruceWong96
Copy link
Contributor

@BruceWong96 BruceWong96 commented Dec 25, 2024 •
edited by SbloodyS
Loading

Purpose of the pull request

close #16918

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@BruceWong96 BruceWong96 changed the title [Fix-16918][Task] Wrong permissions configuration while executing shell [Fix-16918][Task] Fix wrong permissions configuration while executing shell Dec 25, 2024
ruanwenjun
ruanwenjun reviewed Dec 25, 2024
View reviewed changes
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java Outdated Show resolved Hide resolved
...src/main/java/org/apache/dolphinscheduler/server/worker/utils/TaskExecutionContextUtils.java Outdated
@@ -51,6 +52,9 @@ public static void createTaskInstanceWorkingDirectory(TaskExecutionContext taskE
taskInstanceWorkingDirectory);
}
FileUtils.createDirectoryWith755(Paths.get(taskInstanceWorkingDirectory));
final Path taskInstanceWorkingDirectoryPath = Paths.get(taskInstanceWorkingDirectory);
Copy link
Member

@ruanwenjun ruanwenjun Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we change to 775 and make bootstrap user has the same group with tenant, then tenant will have permission to create file at working directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we change to 775 and make bootstrap user has the same group with tenant, then tenant will have permission to create file at working directory.

So, can i create a function createDirectoryWith775() and call it here ?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can. cc @caishunfeng, @SbloodyS

Copy link
Member

@SbloodyS SbloodyS Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok with this. If we change the task instance working directory to 775, we should add it to the installation docs to tell users add tenant to ds user's group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SbloodyS
Which part of the document should I update?
Also, please review the code i just committed.

Thanks.

Copy link
Member

@SbloodyS SbloodyS Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/zh/guide/security/security.md
https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/guide/security/security.md

The create tenant part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/zh/guide/security/security.md https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/guide/security/security.md

The create tenant part.

Finished.

@SbloodyS SbloodyS added the bug Something isn't working label Dec 26, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Dec 26, 2024
@SbloodyS
Copy link
Member

You should run mvn spotless:apply to format the code. @BruceWong96

@SbloodyS SbloodyS changed the title [Fix-16918][Task] Fix wrong permissions configuration while executing shell [Fix-16918][Task] Fix wrong permissions configuration while executing shell Dec 26, 2024
@BruceWong96
Copy link
Contributor Author

You should run mvn spotless:apply to format the code. @BruceWong96

Finished. Thanks.

@sonarqubecloud SonarQubeCloud
Copy link

Please retry analysis of this Pull-Request directly on SonarQube Cloud

@BruceWong96
Copy link
Contributor Author

@caishunfeng @EricGao888 PTAL.

ruanwenjun
ruanwenjun requested changes Dec 27, 2024
View reviewed changes
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java
} else {
Path parent = path.getParent();
if (parent != null && !parent.toFile().exists()) {
createDirectoryWith755(parent);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
createDirectoryWith755(parent);
createDirectoryWith775(parent);

Can we unify the method createDirectoryWith755 ? these two method has the same logic.
We can create a new method createDirectory(@nonnull Path path, Set permissions);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanwenjun ruanwenjun ruanwenjun requested changes

@SbloodyS SbloodyS SbloodyS left review comments

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@BruceWong96 BruceWong96

Labels
backend bug Something isn't working document
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

[Bug] [Task] Wrong permissions configuration while executing shell
3 participants
@BruceWong96 @SbloodyS @ruanwenjun