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

NOISSUE - Vault integration as an addon #1266

Merged
merged 2 commits into from
Oct 31, 2020
Merged

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Oct 23, 2020

What does this do?

Integrates Vault PKI service as a service addon.

Also adds some helper scripts to help setup the CA in Vault, as well as
some docs to explain how to use them.

Originally based from https://github.com/mteodor/vault.

Signed-off-by: Joao Matos joao@tritao.eu

Which issue(s) does this PR fix/relate to?

Could be related to https://github.com/mainflux/mainflux/issues/935.

Did you document any new/modified functionality?

Yes.

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

@tritao Thanks for the contribution. @mainflux/maintainers will review and comment during this week. Did you test integration with the Provision service?

@nmarcetic
Copy link
Collaborator

@tritao Thanks for the contribution. @mainflux/maintainers will review and comment during this week. Did you test integration with the Provision service?

@tritao Provision service. Doc doest mention Vault, but you can see it in Certs service

@tritao
Copy link
Contributor Author

tritao commented Oct 26, 2020

I didn't use the Provision service yet, however I did test Vault with the Certs service and I can successfully issue new certificates.

There are some improvements to Certs service around Vault service that I want to send as another PR, however I was waiting until this one was merged, because it depends on the new Vault environment variables in .env.

Should I send Certs PR now or wait for this one?

@dborovcanin
Copy link
Collaborator

@tritao I think that this PR will be merged, but it's better to wait for the proper review so that you don't have to redo things.

@tritao
Copy link
Contributor Author

tritao commented Oct 26, 2020

Updated with all feedback.

Now using the official Vault image, unfortunately I could not get rid of the custom Docker image.

I spent some time trying but there is a bug with docker-compose handling of relative paths when being called with multiple compose files, like: docker-compose -f docker/docker-compose.yml f docker/addons/vault/docker-compose.yml

It will assume paths are relative to the root file so it breaks relative path behavior when using volumes to pass files (I tested and absolute paths worked OK).

@mteodor
Copy link
Contributor

mteodor commented Oct 28, 2020

Updated with all feedback.

Now using the official Vault image, unfortunately I could not get rid of the custom Docker image.

I spent some time trying but there is a bug with docker-compose handling of relative paths when being called with multiple compose files, like: docker-compose -f docker/docker-compose.yml f docker/addons/vault/docker-compose.yml

It will assume paths are relative to the root file so it breaks relative path behavior when using volumes to pass files (I tested and absolute paths worked OK).

than you for your contribution
I think that it is better to use official vault image without custom Dockerfile, check the other addons how they handle volumes and file paths, so same principle can be applied here
It is better to use two commands and have official vault image.
Tako care that if official vault is used I'm not sure which shell it uses if it has any ( regarding the execution of entrypoint.sh)

.env Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
@tritao
Copy link
Contributor Author

tritao commented Oct 28, 2020

I think that it is better to use official vault image without custom Dockerfile, check the other addons how they handle volumes and file paths, so same principle can be applied here

The other addons have the same bug.

Docker-compose will always compute relative paths relative to the the directory of the first compose file that is passed.

So if you launch like this:

docker-compose -f docker/docker-compose.yml up

docker/docker-compose.yml paths will be relative to docker and everything works.

But if you launch like this:

docker-compose -f docker/docker-compose.yml -f docker/addons/addon/docker-compose.yml up

Then addon/docker-compose.yml paths will still be relative to docker, so a line like - ./subjects.toml:/config/subjects.toml will fail to find subjects.toml. Docker will mount an empty directory instead with the name of the file.

You should be able to easily confirm this.

Given this docker-compose behavior (bug?), I think a solution should be found, because right now, depending on how user launches the addons in the system, it leads to broken platform (and hard to debug problem).

Potential solutions:

  1. Build docker images with files (like I did here)
  2. Move all addons folders to the docker folder (and do not use nested folders with compose files)
  3. Build some kind of tool or make commands that call docker-compose with working directory from docker-compose.yml

I think solution 1. is easiest, and good idea to have Mainflux containers for all services with all necessary files to make them self contained when deploying.

If you want to me change this as you asked to still use relative paths, I will for the sake of merging. But it's a broken design given this docker-compose behavior.

@dborovcanin
Copy link
Collaborator

dborovcanin commented Oct 28, 2020

Updated with all feedback.

Now using the official Vault image, unfortunately I could not get rid of the custom Docker image.

I spent some time trying but there is a bug with docker-compose handling of relative paths when being called with multiple compose files, like: docker-compose -f docker/docker-compose.yml f docker/addons/vault/docker-compose.yml

It will assume paths are relative to the root file so it breaks relative path behavior when using volumes to pass files (I tested and absolute paths worked OK).

If the only reason for the custom Docker image is multiple compose files support - I'd favor the official image over the multiple compose file support. Just mention it somewhere in the docs, and that's it.

@tritao
Copy link
Contributor Author

tritao commented Oct 28, 2020

If the only reason for the custom Docker image is multiple compose files support - I'd favor the official image over the multiple compose file support. Just mention it somewhere in the docs, and that's it.

I think an even more important reason is simplification of production deployments.

Docker containers should be, if possible self-contained, so that an user can push them to an organization Docker registry, and from a production machine, either using an orchestrator like Nomad or Kubernetes, or just using docker-compose, pull them from the registry and up them.

If the containers are not self-contained, then a separate system for uploading (and rollback, if necessary) the necessary support files will be necessary. In this specific case, the shell file entrypoint.sh (and config file) are crucial to the startup of the Vault instance. Without them, the system will not work. They belong inside the container, where they can be distributed and versioned in a self-contained way.

They can still be overridden from a docker-compose.yml file if necessary by the user.

If you think that is a good idea, I can add support for building the containers and pushing to registry via the Makefile system, and change other containers to work like that.

If not, then I will revert this back to just using a relative file like requested.

Let me know what I should do.

@drasko
Copy link
Contributor

drasko commented Oct 28, 2020

@tritao avoid custom images. We will not be ones maintaining Vault image. Also - it is extremely beneficial to have configurable deployments, so all config files must be outside the image and mapped via volume.

Sometimes mapping config files is enough, and yes - if special scripts need to be executed, than entrypoint.sh is needed. This is not a problem at all, as all our deployments are orchestrated - either via docker-compose or via K8s.

@tritao
Copy link
Contributor Author

tritao commented Oct 28, 2020

@tritao avoid custom images. We will not be ones maintaining Vault image. Also - it is extremely beneficial to have configurable deployments, so all config files must be outside the image and mapped via volume.

Sometimes mapping config files is enough, and yes - if special scripts need to be executed, than entrypoint.sh is needed. This is not a problem at all, as all our deployments are orchestrated - either via docker-compose or via K8s.

@drasko Just to make it clear, there is not any custom Vault image, the PR was updated to be based from the official image based upon the initial feedback, it uses FROM vault:latest. So I think the maintainability concerns do not apply.

It just creates another Docker image based off the official one with the necessary files for it to work. The shell script (entrypoint.sh) is always necessary to unseal the Vault, this is why I think it makes sense that it is inside the container image.

Anyway, I've updated the PR to work like requested.

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Please update the branch, remove the unused section in the README and it's ready.

docker/addons/vault/README.md Outdated Show resolved Hide resolved
@dborovcanin dborovcanin changed the title NOISSUE - Vault integration as an addon. NOISSUE - Vault integration as an addon Oct 28, 2020
Integrates Vault PKI service as a service addon.

Also adds some helper scripts to help setup the CA in Vault, as well as
some docs to explain how to use them.

Originally based from https://github.com/mteodor/vault.

Signed-off-by: Joao Matos <joao@tritao.eu>
@tritao
Copy link
Contributor Author

tritao commented Oct 28, 2020

Updated.

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 46c675c into absmach:master Oct 31, 2020
@drasko
Copy link
Contributor

drasko commented Oct 31, 2020

Merged!

Thanks @tritao for this great contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants