xtf-cz / xtf

MIT License
12 stars 55 forks source link

Fixed permissions of shell scripts in binary builds #512

Closed tommaso-borgato closed 2 years ago

tommaso-borgato commented 2 years ago

In binary builds from sources, when the source is an already provisioned server (e.g. some "target/server" produced by the wildfly-maven-plugin), then execution permissions on ".sh" files are lost;

This causes the following error when the POD starts:

/opt/jboss/container/wildfly/run/run: line 74: /opt/server/bin/openshift-launch.sh: Permission denied

This MR just fixes permissions on ".sh" files which is the bare minimum required to have a working server installation;

Please make sure your PR meets the following requirements:

mnovak1 commented 2 years ago

@tommaso-borgato I don't understand the details here, I just want to make sure that this is not workaround for something what is actually a bug. Shouldn't wildfly-maven-plugin preserve permission on .sh files?

mnovak1 commented 2 years ago

Or is the executable flag lost when creating tar?

tommaso-borgato commented 2 years ago

@mnovak1 the issue here is that classes in org.apache.commons.compress.archivers.tar e.g. TarArchiveEntry do not preserve permissions when building a tar .... this is actually a bug in xtf

it's no surprise to me that the issue is in the only step of the process which is using Java library for creating a tar because I experienced the very same issue on another project (the solution was running zip directly there - which we cannot apply here)

this MR is a workaround for the one scenario that we use: creating a tar stream containing a server directory to be sent to the builder image

a more comprehensive solution would be try and find and alternative to org.apache.commons.compress.archivers.tar which preserves permissions (if any) and re-write BinaryBuildFromSources

perhaps we might trace the more comprehensive solution in a separate issue (with lower priority :smile: )

fabiobrz commented 2 years ago

@mnovak1 the issue here is that classes in org.apache.commons.compress.archivers.tar e.g. TarArchiveEntry do not preserve permissions when building a tar .... this is actually a bug in xtf

it's no surprise to me that the issue is in the only step of the process which is using Java library for creating a tar because I experienced the very same issue on another project (the solution was running zip directly there - which we cannot apply here)

this MR is a workaround for the one scenario that we use: creating a tar stream containing a server directory to be sent to the builder image

a more comprehensive solution would be try and find and alternative to org.apache.commons.compress.archivers.tar which preserves permissions (if any) and re-write BinaryBuildFromSources

perhaps we might trace the more comprehensive solution in a separate issue (with lower priority smile )

From what I can read around, this might be intentional, since permissions are dealt with at the OS level. In such a case, it seems to me this is not a workaround for an external component bug, but rather a proper fix for the XTF (caller) behavior when dealing with org.apache.commons.compress.archivers.tar.TarArchiveEntry etc.

In such a case there's no issue that should be created externally, maybe an internal one fixed by this very PR. Let me know in case I am missing something.

mnovak1 commented 2 years ago

Ok, I think that correct fix should retrieve file permission from the file being added into tar archive and set on tar entry. Something like:

PosixFileAttributes attrs = Files.getFileAttributeView(tempFile, PosixFileAttributeView.class).readAttributes();
System.out.format("%s %s%n", attrs.owner().getName(), PosixFilePermissions.toString(attrs.permissions())); 
// prints OS: mnovak rw-------
// convert "rw-------" into "int mode" and then
tarArchiveEntry.setMode(intMode);

wdyt?

tommaso-borgato commented 2 years ago

@mnovak1 good idea, going to check

mnovak1 commented 2 years ago

@mnovak1 good idea, going to check

:+1: ...thanks for looking at it.

tommaso-borgato commented 2 years ago

@mnovak1 I applied your suggestion and the bug fix is now complete

mnovak1 commented 2 years ago

LGTM, merging. Thanks @tommaso-borgato !