Fix issue with docker volume-mounted config file

[robinboening]

Description

Using sed -i was causing an issue when a custom opensearch.yml file was mounted as a volume.

sed: cannot rename /usr/share/opensearch/config/sedqdMb0d: Device or resource busy

The reason for the issue was found by @unhipzippo opensearch-project/OpenSearch#768 (comment) ❤️

The "sed -i" is an attempt to modify the opensearch.yml file "in place" -- But according to the GNU sed documentation (https://www.gnu.org/software/sed/manual/sed.html#Command_002dLine-Options), "in-place" actually "does this by creating a temporary file and sending output to this file rather than to the standard output. ... the temporary file is renamed to the output file’s original name".

I believe this rename would require changing the inode of the original file -- something that Docker volume mounts don't permit.

Issues Resolved

Potentially opensearch-project/OpenSearch#768 and opensearch-project/OpenSearch#1579

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov-commenter]

Codecov Report

Merging #1130 (ccf0b8c) into main (c3d0129) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1130   +/-   ##
=========================================
  Coverage     93.30%   93.30%           
  Complexity       10       10           
=========================================
  Files            99       99           
  Lines          2496     2496           
  Branches          7        7           
=========================================
  Hits           2329     2329           
  Misses          159      159           
  Partials          8        8           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3d0129...ccf0b8c. Read the comment docs.

[scratchings]

Whilst this fixes the initial issue with regard to the behavior of sed -i, it still prevents Opensearch from starting if Docker Compose 'config's are used to bind mount the opensearch.yml file (the most sensible way of distributing in a swarm mode cluster) as these are read-only.

[dblock]

Whilst this fixes the initial issue with regard to the behavior of sed -i, it still prevents Opensearch from starting if Docker Compose 'config's are used to bind mount the opensearch.yml file (the most sensible way of distributing in a swarm mode cluster) as these are read-only.

So what's the suggested fix? If this change doesn't fix the entire problem maybe we should hold it off?

@robinboening you need to --amend -s your commits for DCO, please.

[scratchings]

Wrapping it with an test -w on the opensearch.yml file would be a simple work-around - if the file is being bind mounted, then it seems likely to me that the end user does not expect their config file to be modified on startup.

   if [ -w "$OPENSEARCH_HOME/config/opensearch.yml" ]; then
        if [ "$DISABLE_SECURITY_PLUGIN" = "true" ]; then
            echo "Disabling OpenSearch Security Plugin"
            sed 's/plugins.security.disabled.*$/plugins.security.disabled: true' $OPENSEARCH_HOME/config/opensearch.yml | tee $OPENSEARCH_HOME/config/opensearch.yml
        else
           echo "Enabling OpenSearch Security Plugin"
           sed '/plugins.security.disabled/d' $OPENSEARCH_HOME/config/opensearch.yml | tee $OPENSEARCH_HOME/config/opensearch.yml 
            echo "plugins.security.disabled: true" >> $OPENSEARCH_HOME/config/opensearch.yml
        fi
    fi

[peternied]

@peterzhuamazon Could you take a look?

[peterzhuamazon]

Hi @robinboening can you check @scratchings comment #1130 (comment) and see if that is useful and update the PR?

Or I can approve the PR at the current state.

Thanks.

[robinboening]

@peterzhuamazon I simply don't have enough knowledge about docker/swarm to judge if the proposed change in #1130 (comment) is good or not.

@scratchings I thought docker creates writeable bind mounts by default. Referring to the docs https://docs.docker.com/storage/bind-mounts/#use-a-read-only-bind-mount only if specifically declared as readonly they're treated as such. I have never used swarm, so maybe its different in that mode... 🤷 it seems you know more on this topic, would you mind to enlighten me? :)

[peterzhuamazon]

@scratchings comment seems simply checking whether the file is writable before the changes are made.
Which seems simple enough for me.
But I also want his comment on @robinboening replies.

Thanks.

[scratchings]

@scratchings I thought docker creates writeable bind mounts by default. Referring to the docs https://docs.docker.com/storage/bind-mounts/#use-a-read-only-bind-mount only if specifically declared as readonly they're treated as such. I have never used swarm, so maybe its different in that mode... 🤷 it seems you know more on this topic, would you mind to enlighten me? :)

Docker swarm mode has 'configs' https://docs.docker.com/engine/swarm/configs/ and 'secrets' https://docs.docker.com/engine/swarm/secrets objects (docker compose can implement these for non-swarm systems to a limited extent). Both of these objects types are stored in the Swarm database that is distributed amongst the nodes so that your container can be started anywhere and have access to these items. Secrets are mounted from a RAM file system into /run/secrets/ and configs into a location of your choice. In a swarm you have full control over ownership and access permissions (in plain compose you don't as they are 'faked' with bind mounts) they are ALWAYS read-only. If you've gone to the effort of importing the opensearch.yml file into a swarm configs item you are unlikely to want automatic modification of the content, the block here seems to be largely to support 'demo' deployments.

[peterzhuamazon]

@scratchings I thought docker creates writeable bind mounts by default. Referring to the docs https://docs.docker.com/storage/bind-mounts/#use-a-read-only-bind-mount only if specifically declared as readonly they're treated as such. I have never used swarm, so maybe its different in that mode... 🤷 it seems you know more on this topic, would you mind to enlighten me? :)

Docker swarm mode has 'configs' https://docs.docker.com/engine/swarm/configs/ and 'secrets' https://docs.docker.com/engine/swarm/secrets objects (docker compose can implement these for non-swarm systems to a limited extent). Both of these objects types are stored in the Swarm database that is distributed amongst the nodes so that your container can be started anywhere and have access to these items. Secrets are mounted from a RAM file system into /run/secrets/ and configs into a location of your choice. In a swarm you have full control over ownership and access permissions (in plain compose you don't as they are 'faked' with bind mounts) they are ALWAYS read-only. If you've gone to the effort of importing the opensearch.yml file into a swarm configs item you are unlikely to want automatic modification of the content, the block here seems to be largely to support 'demo' deployments.

Thanks for the detailed explanation here @scratchings.
On that note, I do agree that a simple -w fix would be enough for us here.
@robinboening could update the PR accordingly.

Thanks.

[peterzhuamazon]

Block this change as we will not introduce this to 1.1.1 patch release. We will unblock this for the next major/minor release.

Thanks.

[peterzhuamazon]

Since 1.2.1 OpenSearch is out we can resume this, @robinboening any updates to my previous comment?
Thanks.

[gijs007]

This issue was reported in may, over half a year ago: opensearch-project/OpenSearch#768
I see a fix is ready, but not merged into the main build. When can we expect this?

Note: At this point we still cannot rollout OpenSearch for production, since this issue prevents us from using custom configs with the recommended docker installation...

[dblock]

I think I'd prefer the fix as written in this PR. Using -w avoids touching the configuration, but it also makes the assumption that the configuration doesn't need to change, i.e. it would silently swallow an error.

@peterzhuamazon Can we merge as is?

[peterzhuamazon]

I think I'd prefer the fix as written in this PR. Using -w avoids touching the configuration, but it also makes the assumption that the configuration doesn't need to change, i.e. it would silently swallow an error.

@peterzhuamazon Can we merge as is?

I agree with DB on this one.

However, I have since identified some missed changes for dashboards.

@robinboening would you mind adding similar changes for dashboards entrypoint.sh as well?
https://github.com/opensearch-project/opensearch-build/blob/main/docker/release/config/opensearch-dashboards/opensearch-dashboards-docker-entrypoint.sh#L180-L181

        sed -i /^opensearch_security/d $OPENSEARCH_DASHBOARDS_HOME/config/opensearch_dashboards.yml
        sed -i 's/https/http/' $OPENSEARCH_DASHBOARDS_HOME/config/opensearch_dashboards.yml

Thanks.

[dblock]

However, I have since identified some missed changes for dashboards.

Good catch.

@gijs007 @scratchings feel free to PR the complete fix on top of this, will merge, apologies for missing this earlier

[robinboening]

I think I'd prefer the fix as written in this PR. Using -w avoids touching the configuration, but it also makes the assumption that the configuration doesn't need to change, i.e. it would silently swallow an error.
@peterzhuamazon Can we merge as is?

I agree with DB on this one.

However, I have since identified some missed changes for dashboards.

@robinboening would you mind adding similar changes for dashboards entrypoint.sh as well? https://github.com/opensearch-project/opensearch-build/blob/main/docker/release/config/opensearch-dashboards/opensearch-dashboards-docker-entrypoint.sh#L180-L181

        sed -i /^opensearch_security/d $OPENSEARCH_DASHBOARDS_HOME/config/opensearch_dashboards.yml
        sed -i 's/https/http/' $OPENSEARCH_DASHBOARDS_HOME/config/opensearch_dashboards.yml

Thanks.

I can look into this as well. Hopefully today / tomorrow.

[robinboening]

However, I have since identified some missed changes for dashboards.

@dblock I've covered the other part and amended the changes to the commit.

I think I'd prefer the fix as written in this PR. Using -w avoids touching the configuration, but it also makes the assumption that the configuration doesn't need to change, i.e. it would silently swallow an error.

I understand your arguments on this and I don't want to make this PR bigger than it is right now, but I'd like to kick off a separate discussion around this topic (maybe to be moved in to a new thread):

I am questioning if the current approach of silently altering a custom config file is any better than swallowing a potential error. My argument is that the user deploying OpenSearch is making a conscious decision on how the config file is customised and how it finally looks. OpenSearch then silently does its own changes on top of that, possibly without being noticed.

Shouldn't OpenSearch detect the error at runtime and throw a meaningful error instead of silently altering the config to make it right?

[dblock]

However, I have since identified some missed changes for dashboards.

@dblock I've covered the other part and amended the changes to the commit.

Looks good, thanks.

I am questioning if the current approach of silently altering a custom config file is any better than swallowing a potential error. My argument is that the user deploying OpenSearch is making a conscious decision on how the config file is customised and how it finally looks. OpenSearch then silently does its own changes on top of that, possibly without being noticed.

Shouldn't OpenSearch detect the error at runtime and throw a meaningful error instead of silently altering the config to make it right?

I agree. Feel free to open a new issue for this in OpenSearch proper. I'd want to be able to set DISABLE_SECURITY as an ENV variable and see it override the setting without having to alter a .yml config file.

[gijs007]

Glad to see this issue has been fixed. Would it be possible to get the docker images updated directly, since this is a blocking issue for production deployments?

[peterzhuamazon]

Thanks.

[kinseii]

Excuse me, can you please tell me when a new release with this fix will be available?