:bug: [BUG]: Silently failing birdhouse-deploy operations

[fmigneault]

Summary

Some operations executed by birdhouse-deploy.sh can fail silently.

This issue is opened as a general discussion about how to handle this kind of situation as a whole (what is our philosophy about managing these cases?), but providing a specific use case that was encountered.

Do we consider these to be edge cases that we don't manage? Do we reinforce strict error handling? Or, do we have some middle ground best-practice to put forward in the docs?

Details

For example, if any of the following commands fail, for example, because "${THIS_DIR}/static" already exists, but with inadequate user/group permissions causing the files to not be created, the birdhouse-deploy.sh execution will happily resume it operations without any apparent failure.

birdhouse-deploy/birdhouse/components/proxy/pre-docker-compose-up

Lines 9 to 13 in 293645a

	mkdir -p "${THIS_DIR}/static"
	echo "${BIRDHOUSE_VERSION_JSON}" > "${THIS_DIR}/static/version.json"
	echo "${BIRDHOUSE_DEPLOY_SERVICES_JSON}" > "${THIS_DIR}/static/services.json"
	echo "${BIRDHOUSE_DEPLOY_COMPONENTS_JSON}" > "${THIS_DIR}/static/components.json"

At the very least, permissions errors such as in this case would be relevant to be displayed on stderr.

In this specific case, this leads to a seemingly "functional" platform that boots and where services respond without problem, but the informative /version, /services and /components endpoints cause 404 errors.

To Reproduce

Steps to reproduce the behavior:

1. Create a birdhouse/components/proxy/static directory with some limited access (eg: root or specific user read-only)
2. Run the stack with birdhouse-deploy to achieve a "running" state, and notice that no errors are reported.
3. Access any of the /version, /services or /components endpoint.

Environment

all

Concerned Organizations

[tlvu]

Simple fix to do chmod a+w -R ${THIS_DIR}/static after the files are generated?

[tlvu]

That's odd, all the instantiated templates do not suffer from this? Only these .json?

[mishaschwartz]

I agree that overall we should be reporting errors like this better.

In this specific example the error message will get written to stderr but it is not logged properly and we really should be logging it properly.

I think that we should either properly catch these errors and stop the stack from building, or we should be better at logging errors but don't stop the stack from building:

Stop the stack from building:

• add set -e in birdhouse-compose.sh
• add -e to the default SHELL_EXEC_FLAGS variable
• go through the birdhouse-compose.sh and the pre/post-docker-compose* files and add an explicit || true after commands that are allowed to fail
• add a trap to catch unexpected errors and log that an unexpected error occurred

Let the stack build but report errors better:

• in birdhouse-deploy.sh add || log ERROR "informative message" after commands that may fail
• convert pre-docker-compose files to pre-docker-compose.include (so that we can use the log commands) and do the same thing there
• Note: I think this one will be a lot more work and is prone to mistakes if we miss possible errors but it is more "backwards compatible" since the stack will build under the same conditions as before.

[mishaschwartz]

@tlvu I think @fmigneault is using the static json files as an example of something that could go wrong, not something that is currently actually causing problems.

[tlvu]

I think this silent bad config file generation problem is due to user permission (current user vs root).

If birdhouse-compose.sh spawn a container and runs in there, we will control the user of the container so the problem should be fixed.

Advantages of birdhouse-compose.sh running in a container:

• we control the docker-compose binary version, which will be the same everywhere. Any updates will be consistent across all deployments and not force manual intervention for each deployment (like the current v1 to v2 update)
• fix this config file generation permission issue
• catch error earlier with autodeploy, because autodeploy runs in a container, sometime a missing volume-mount, a missing binary in the container is discovered later because current birdhouse-compose.sh runs directly on the host so those errors can not be caught by birdhouse-compose.sh currently.
  • The docker image of birdhouse-compose.sh and autodeploy should be the same, ensuring compatibility and early error discovery.

So a 3rd option to fix this with extra advantages and fixing #246 (comment). And probably can work alongside with extra trap or -e to improve error handling.

[mishaschwartz]

@tlvu I think that is a great solution to a different problem than the one being discussed here.

Let's explore that as an option but the discussion here is about how to handle any commands that fail during deployment, whether it's running in a docker container or not.

[fmigneault]

That's odd, all the instantiated templates do not suffer from this? Only these .json?

In my case, it occurred only for the static files because their generation are always executed (by pre-docker-compose), while the templates were only on up (and restart also now). So a previous birdhouse-compose ps ran by root "blocked" the directory.

But indeed like pointed out by @mishaschwartz, the important here should be the general concept of error handling regardless of the use case.

I think that we should either properly catch these errors and stop the stack from building, or we should be better at logging errors but don't stop the stack from building [...]

Agreed.

All suggestions seem reasonable, except the pre-docker-compose.include, since this can be used by external components. The set -x looks easier to implement globally (and we don't miss one case), but might not be descriptive enough depending on the case (or maybe it is?). Logs could be more explicit in certain cases, but might also not consider all cases, and misinform more than anything in others...

I'm fine with either way, as long as "something" allows identifying errors (better than current situation).

I wouldn't worry about "backwards incompatible" non-raising errors, since I assume the intent was always to break out given that exit 1/return 1 were used, and they do act as such is certain specific situations when the required variables are used. I think we can consider the current behavior a bug.

If birdhouse-compose.sh spawn a container and runs in there, we will control the user of the container so the problem should be fixed.

I'm not sure it's that easy. Maybe it works in a root env, but any rootless configuration would not map users properly and could generate even more mismatching UID:GID on the host (I had that happen before with "mongo" user incorrectly mapped between the host and various dockers). We must aim for rootless if possible for improved security. Root should be used to block access to certain files, not brute force the problem cases. However, root is needed for certain aspects (eg. limiting SSL certificate access), so it's a bit of a catch-22 to define the user of that birdhouse-deploy docker image.