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

[bitnami/elasticsearch] do not fail if asked to chown read-only files #77526

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[bitnami/elasticsearch] do not fail if asked to chown read-only files #77526

wants to merge 1 commit into from

Conversation

ianroberts
Copy link

@ianroberts ianroberts commented Feb 17, 2025 •
edited
Loading

Description of the change

Add || true to chown commands that operate recursively on folders that are expected to be mounted into the container from volumes or bind mounts, so that the chown does not cause a fatal error if any of the target directories or their children are mounted read-only.

Benefits

Currently when running as root the elasticsearch container will fail to start if any folder under /opt/bitnami/elasticsearch/config is mounted from a read-only filesystem. In particular this happens in the corresponding bitnami Helm chart, where the TLS certificates for Elasticsearch are mounted in from a Kubernetes secret. This change means that such cases are no longer fatal errors - the files in the read-only filesystems will not of course have their ownership changed, but there's not really anything better we can do if the filesystem is read-only.

Possible drawbacks

If a mounted filesystem should have been read-write but was mounted read-only by mistake, it would previously have caused a fatal error, prompting the user to change their configuration; this change will make that a no-op so they might not notice.

Applicable issues

Additional information

This fix should probably be applied before bitnami/charts#31960 is merged.

@github-actions github-actions bot added elasticsearch triage Triage is needed labels Feb 17, 2025
@github-actions github-actions bot requested a review from javsalgar February 17, 2025 13:37
@ianroberts ianroberts mentioned this pull request Feb 17, 2025
Open
4 tasks
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Feb 18, 2025
@github-actions github-actions bot removed the triage Triage is needed label Feb 18, 2025
@github-actions github-actions bot removed the request for review from javsalgar February 18, 2025 08:56
@github-actions github-actions bot requested a review from migruiz4 February 18, 2025 08:56
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 6, 2025

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Mar 6, 2025
@ianroberts
Copy link
Author

Still relevant.

@github-actions github-actions bot removed the stale 15 days without activity label Mar 7, 2025
migruiz4
migruiz4 reviewed Mar 11, 2025
View reviewed changes
Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution @ianroberts. I'm sorry for the late response.

I would like to suggest two changes to your contribution:

  • You used || :, but we prefer using || true.
  • You added both || : and -f option for chown, which is redundant. The option -f suppresses both error exit-code and messages, while the || : would supress the exit code. I would suggest not using -f, since hiding error message could make troubleshooting really complicated in some cases.

@ianroberts
[bitnami/elasticsearch] do not fail if asked to chown read-only files
2a9bd60 
Added "|| true" to chown commands so they do not cause the container startup to fail if any of the target folders or their subdirectories are mounted from a read-only filesystem

Signed-off-by: Ian Roberts <[email protected]>
@ianroberts
Copy link
Author

Very sensible suggestions, I've implemented them and rebased on the latest main branch.

@ianroberts ianroberts requested a review from migruiz4 March 11, 2025 12:26
@ianroberts
Copy link
Author

  • You added both || : and -f option for chown, which is redundant. The option -f suppresses both error exit-code and messages, while the || : would supress the exit code.

My first attempt just used the -f but it turns out that -f suppresses the messages but does not on its own affect the exit code. So I added the || : (now || true) as well to actually fix the issue.

@ianroberts
Copy link
Author

Still waiting for final review.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 7, 2025

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Apr 7, 2025
@ianroberts
Copy link
Author

Not stale, still waiting for final approval by @migruiz4

@github-actions github-actions bot removed the stale 15 days without activity label Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@migruiz4 migruiz4 Awaiting requested review from migruiz4

At least 1 approving review is required to merge this pull request.

Assignees

@migruiz4 migruiz4

Labels
elasticsearch in-progress verify Execute verification workflow for these changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bitnami/elasticsearch] crash when running as root when TLS certificates are read-only
4 participants
@ianroberts @migruiz4 @javsalgar @carrodher