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.

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

Truncate mode bits for z/OS file permissions #2023

Merged
merged 1 commit into from Dec 5, 2019

Conversation

aguibert
Copy link
Contributor

@aguibert aguibert commented Nov 1, 2019

When using Testcontainers on z/OS with the ImageFromDockerFile.withFileFromFile() API, file we get an illegal set of mode bits from the OS.

Here is an example of my container definition:

@ClassRule
public static CustomPostgreSQLContainer<?> postgre = new CustomPostgreSQLContainer<>(new ImageFromDockerfile()
            .withDockerfileFromBuilder(builder -> builder.from("postgres:11.2-alpine")
                            .copy("/var/lib/postgresql/server.crt", "/var/lib/postgresql/server.crt")
                            .copy("/var/lib/postgresql/server.key", "/var/lib/postgresql/server.key")
                             .run("chown postgres /var/lib/postgresql/server.key && chmod 600 /var/lib/postgresql/server.key && " +
                                  "chown postgres /var/lib/postgresql/server.crt && chmod 600 /var/lib/postgresql/server.crt")
                             .build())
            .withFileFromFile("/var/lib/postgresql/server.crt", new File("lib/LibertyFATTestFiles/ssl-certs/server.crt"))
            .withFileFromFile("/var/lib/postgresql/server.key", new File("lib/LibertyFATTestFiles/ssl-certs/server.key")))
                            .withDatabaseName(POSTGRES_DB)
                                    // ...

We get an error that looks like this:

org.testcontainers.containers.ContainerLaunchException: Container startup failed
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:290)
at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:272)
at org.testcontainers.containers.GenericContainer.starting(GenericContainer.java:807)
at org.testcontainers.containers.FailureDetectingExternalResource$1.evaluate(FailureDetectingExternalResource.java:29)
at componenttest.custom.junit.runner.FATRunner$2.evaluate(FATRunner.java:314)
at componenttest.custom.junit.runner.FATRunner.run(FATRunner.java:167)
Caused by: org.testcontainers.containers.ContainerFetchException: Can't get Docker image: org.testcontainers.images.builder.ImageFromDockerfile@90e28151
at org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1057)
at org.testcontainers.containers.GenericContainer.logger(GenericContainer.java:410)
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:279)
Caused by: java.lang.RuntimeException: mode '50332068' is too big ( > 2097151 ).
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumber(TarArchiveOutputStream.java:636)
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumber(TarArchiveOutputStream.java:624)
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.failForBigNumbers(TarArchiveOutputStream.java:616)
at org.apache.commons.compress.archivers.tar.TarArchiveOutputStream.putArchiveEntry(TarArchiveOutputStream.java:369)
at org.testcontainers.utility.MountableFile.recursiveTar(MountableFile.java:315)
at org.testcontainers.utility.MountableFile.transferTo(MountableFile.java:292)
at org.testcontainers.images.builder.ImageFromDockerfile.resolve(ImageFromDockerfile.java:139)
at org.testcontainers.images.builder.ImageFromDockerfile.resolve(ImageFromDockerfile.java:36)
at org.testcontainers.utility.LazyFuture.getResolvedValue(LazyFuture.java:20)
at org.testcontainers.utility.LazyFuture.get(LazyFuture.java:27)
at org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1055)

After adding some debug code to my test, I found out that there is an extra set of mode bits on z/OS files.

In my case, the mode bits are:

300 000 644

Which is outside of the maximum allowed range of:

007 777 777

My proposed solution on this is to simply truncate to the maximum allowed range when running on z/OS, and then setting the default file or dir mode bits (0100000 or 040000 respectively). In this case, it would result in mode bits of:

000 100 644

@aguibert aguibert changed the title Truncate mode bits for z/OS file permissions [Bug fix] Truncate mode bits for z/OS file permissions Nov 9, 2019
@rnorth
Copy link
Member

rnorth commented Nov 9, 2019

Thanks @aguibert! I'm afraid I don't have many z/OS systems knocking around to test this on, so I'll have to take your word for it 😂

Are there any other areas where you're seeing compatibility issues, or is this the only thing?

I think we just need to be a little careful about including compatibility fixes for OS environments that we don't currently officially support and which we don't have a plausible CI story.

I'm not against this PR, just want to make sure that we're not opening a Pandora's box 😄

// Truncate mode bits for z/OS
if ("OS/390".equals(SystemUtils.OS_NAME) ||
"z/OS".equals(SystemUtils.OS_NAME) ||
"zOS".equals(SystemUtils.OS_NAME) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought: is it necessary to decide whether to truncate based on system type, or could we always do it?

Linux is our only supported container OS, so perhaps the way to think about it could be just as ensuring that no matter what unixMode we get, it's always within the right bounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be safest to only truncate for z/OS to minimize potential side effects of this PR.

I can't think of any reason a Linux distribution would have out of range mode bits, but I think it would be good to capture such a scenario in the form of an error first so we can dissect what's going on, rather than coercing the mode bits into the allowed range and possibly dealing with harder-to-debug side effects as a result.

@aguibert
Copy link
Contributor Author

aguibert commented Nov 9, 2019

hi @rnorth, thanks for having a look at this PR. Let me give a bit more background on the environment where I hit this issue. The scenario where we are hitting this problem is where our test client is running on zOS where we set up an app server, and then we use Testcontainers to spin up databases on a remote Linux machine where Docker is installed. So the container environment here is still Linux, but it just happens to be a Java client running on zOS driving the workload.

Are there any other areas where you're seeing compatibility issues, or is this the only thing?

Nope, this is the only issue we are facing w/ Testcontainers on zOS. Other than that it works great!

I think we just need to be a little careful about including compatibility fixes for OS environments that we don't currently officially support and which we don't have a plausible CI story.

Yea, unfortunately there isn't a good way to publicly test libs with zOS. I'd prefer to contribute this fix back to open source so everyone can benefit from the fix. The alternative is for us to build/maintain an in-house fork of Testcontainers which I'd really like to avoid.

@aguibert
Copy link
Contributor Author

hi @rnorth, any further thoughts on this bug fix?

@rnorth
Copy link
Member

rnorth commented Nov 28, 2019

@aguibert sorry for taking ages to get back to you. I'm fine with this, and understand your reasoning, but I'm afraid there's now a merge conflict 😣.

@aguibert
Copy link
Contributor Author

resolved the merge conflict so now it should be ready for merge @rnorth

@rnorth rnorth added the type/bug label Dec 5, 2019
@rnorth rnorth changed the title [Bug fix] Truncate mode bits for z/OS file permissions Truncate mode bits for z/OS file permissions Dec 5, 2019
@rnorth rnorth merged commit 0adacde into testcontainers:master Dec 5, 2019
@rnorth
Copy link
Member

rnorth commented Dec 5, 2019

Thanks @aguibert - finally merged!

@rnorth rnorth added this to the next milestone Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants