Skip to content

Conversation

@emreyigit
Copy link

@emreyigit emreyigit commented Jul 7, 2025

The docker image fails to write diagnostics logs on default config. The default directory for logging output is HZ_HOME but it's missing the write permission. Apparently, the issue was there but it's catch during DEX-320 verifications.

Unfortunately, I couldn't find a way to put proper testing on Docker containers.

@emreyigit emreyigit added this to the 6.0 milestone Jul 7, 2025
@emreyigit emreyigit requested a review from cheels July 7, 2025 10:18
@emreyigit emreyigit requested a review from a team as a code owner July 7, 2025 10:18
@JackPGreen
Copy link
Collaborator

Unfortunately, I couldn't find a put proper testing on Docker containers.

I would really like a test to cover this change - maybe on the hazelcast side?

@emreyigit
Copy link
Author

Sure, @cheels is already started to test.

@nishaatr
Copy link
Contributor

nishaatr commented Jul 7, 2025

Not sure worth adding a test or enable logging or something so this gets tested during simple-smoke-test.sh

Surprised this wasn't fixed much earlier. I mean how did it work before!

&& echo "Granting read permission to ${HZ_HOME}" \
&& chmod -R +r ${HZ_HOME} \
&& echo "Granting read&write permission to ${HZ_HOME}" \
&& chmod -R +rw ${HZ_HOME} \
Copy link
Contributor

Choose a reason for hiding this comment

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

'w' is not enough to allow files to be written into /opt/hazelcast directory
You have to add chmod -R a+rwX

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

@emreyigit emreyigit requested a review from cheels July 8, 2025 07:50
@JackPGreen
Copy link
Collaborator

Sure, @cheels is already started to test.

Thanks - I'll hold off approving until that if that's ok?

I think the change is fine but I wouldn't have spotted #1009 (comment) so good to be confident.

@emreyigit
Copy link
Author

emreyigit commented Jul 8, 2025

Another option we might override the user.dir property over ENV variables for container usage only and limit the permission for that folder.
We can't since it will change for whole system.

Copy link
Contributor

@ldziedziul ldziedziul left a comment

Choose a reason for hiding this comment

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

This increases the risk of accidental or malicious changes to binaries, configs, or scripts, potentially compromising security and stability. Limiting permissions to read helps protect the integrity of the Hazelcast instance.

Why not use a custom hazelcast.diagnostics.directory?

I've tested it with:

docker run --rm -it -e HZ_LICENSEKEY -e HZ_INSTANCETRACKING_FILENAME=/dev/null -e JAVA_OPTS="-Dhazelcast.diagnostics.enabled=true -Dhazelcast.diagnostics.directory=/tmp" hazelcast/hazelcast-enterprise:latest-snapshot

@emreyigit
Copy link
Author

@ldziedziul diagnostics has already a default path config but it points to user.dir which is the opt/hazelcast in docker. We can't change the default path since it's a breaking change. That's why we have updated docker file.

@ldziedziul
Copy link
Contributor

@ldziedziul diagnostics has already a default path config but it points to user.dir which is the opt/hazelcast in docker. We can't change the default path since it's a breaking change. That's why we have updated docker file.

However, we can change the path specifically within the Docker images. Currently, diagnostics doesn't work in Docker at all, so it’s hardly a breaking change.

Side note: IMHO, compromising security and stability just to avoid a "breaking change" is a very questionable decision.

@devOpsHazelcast
Copy link
Collaborator

PR closed by Hazelcast automation as no activity (>3 months). Please reopen with comments, if necessary. Thank you for using Hazelcast and your valuable contributions

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants