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

feat: minio service #297

Open
wants to merge 17 commits into
base: releases/v18
Choose a base branch
from
Open

feat: minio service #297

wants to merge 17 commits into from

Conversation

mjdupont12
Copy link

No description provided.

@mjdupont12 mjdupont12 self-assigned this Dec 5, 2024
@davidlougheed davidlougheed changed the base branch from main to releases/v18 December 9, 2024 16:27
Copy link
Contributor

@v-rocheleau v-rocheleau left a comment

Choose a reason for hiding this comment

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

Nice start!
I added some comments for small stuff, did not try to deploy it yet.
Once the PR is close to merge, it would be nice if you can add the start of a documentation file at docs/object-store.md.
For now it would only concern Minio, but it could include S3 docs for Bento at large in the future.

etc/bento.env Outdated Show resolved Hide resolved
etc/bento.env Outdated Show resolved Hide resolved
etc/bento_dev.env Outdated Show resolved Hide resolved
etc/default_config.env Outdated Show resolved Hide resolved
lib/gateway/public_services/minio.conf.tpl Outdated Show resolved Hide resolved
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

another minor note: some files are missing terminal newlines to make them valid POSIX text files; these show up as the red circles with dashes through them on GitHub - it's not essential, but these would be good to add.

etc/bento_services.json Outdated Show resolved Hide resolved
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

left some notes on the changes themselves.

when I try to log into the minio console using root/my root password, it gives me an "invalid login" message. are there any additional steps I need to do here?

EDIT - nevermind, was using wrong username :)

also, we should fill out the v18 migration guide for how to set this up when you have a chance - maybe you can start by writing the steps needed to set up minio in the pull request description?

etc/bento_services.json Outdated Show resolved Hide resolved
etc/bento.env Outdated Show resolved Hide resolved
etc/bento.env Show resolved Hide resolved
lib/gateway/public_services/minio.conf.tpl Outdated Show resolved Hide resolved
lib/minio/docker-compose.minio.yaml Outdated Show resolved Hide resolved
test: ["CMD", "mc", "ready", "local"]
interval: 5s
timeout: 5s
retries: 5
Copy link
Member

Choose a reason for hiding this comment

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

use standard healthcheck settings for dependency-type services (see databases elsewhere in this repo for examples)

Copy link
Author

Choose a reason for hiding this comment

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

@davidlougheed I updated the healthcheck with standard settings, just tell me if everything is ok?

Copy link
Contributor

@v-rocheleau v-rocheleau left a comment

Choose a reason for hiding this comment

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

API routing testing

Tried out the branch locally, managed to go in the console to create credentials and create a bucket with s3cmd. Did some diagnosis on HTTPS and routing issues.

s3cmd CLI tool

S3cmd is an helpful CLI tool to test API connections with an S3 endpoint.
For s3cmd you usually create a config file per endpoint, I made mine at ~/.s3cfg-minio

# Setup endpoint (get API IP with 'bentoctl logs minio')
host_base = 172.16.28.2:9000
host_bucket = 172.16.28.2:9000
use_https = False

# Setup access keys
access_key =  <GET FROM YOUR LOCAL MINIO SERVER>
secret_key = <SAME>

Then, using this configuration file I was able to create and list buckets:

# create bucket
s3cmd -c ~/.s3cfg-minio mb s3://test
# list buckets at /
s3cmd -c ~/.s3cfg-minio ls s3://

In the current state of the PR I only managed to connect to the API with HTTP and the plain IP (more on why bellow).

Like keycloak, minio seems to be pretty capricious with TLS and hostnames, so here are some considerations:

  • When I used bentov2.local/api/minio for host_base and host_bucket, with HTTPS enabled, there is a certificate validation error.
    • For TLS connections, Minio requires a certificate (docs).
    • We can use the minio.bentov2.local subdomain to issue certificates, it will help avoid routing issues with the S3 API as well.
    • Self-signed certs can be generated and mounted to the container like we do with Keycloak using ./bentoctl.bash init-certs

More quirks from my s3cmd experiments:

  • bentov2.local/api/minio for host_base and host_bucket with HTTP breaks the routing, since the Bento gateway does a 301 permanent redirect from HTTP to HTTPS on all requests.
  • So using the IP on the Docker network with plain HTTP requests is the only way I found to connect to minio's API right now.

@mjdupont12
Copy link
Author

Nice start! I added some comments for small stuff, did not try to deploy it yet. Once the PR is close to merge, it would be nice if you can add the start of a documentation file at docs/object-store.md. For now it would only concern Minio, but it could include S3 docs for Bento at large in the future.

@v-rocheleau I started writing object-store.md and I only put the doc linked to minio but I will improve it once the drop-box + drs service has been modified.

@mjdupont12
Copy link
Author

another minor note: some files are missing terminal newlines to make them valid POSIX text files; these show up as the red circles with dashes through them on GitHub - it's not essential, but these would be good to add.

@davidlougheed all errors related to "missing terminal newlines" should be resolved. I had a problem with my Visual Studio Code which removed this terminal newline each time I saved a file (.yaml & .json), this should no longer happen!

@mjdupont12
Copy link
Author

also, we should fill out the v18 migration guide for how to set this up when you have a chance - maybe you can start by writing the steps needed to set up minio in the pull request description?

@davidlougheed I updated the v18 migrating guide with the steps to activate minio.

@mjdupont12 mjdupont12 marked this pull request as ready for review December 18, 2024 15:02
@mjdupont12 mjdupont12 changed the title (WIP) =add minio service to bento =add minio service to bento Dec 18, 2024
@v-rocheleau v-rocheleau changed the title =add minio service to bento feat: minio service Dec 20, 2024
etc/bento.env Outdated Show resolved Hide resolved
docs/img/minio_object_store.png Outdated Show resolved Hide resolved
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

consistent capitalization requests. otherwise, looks good to me!

docs/object-store.md Outdated Show resolved Hide resolved
docs/migrating_to_18.md Outdated Show resolved Hide resolved
docs/migrating_to_18.md Outdated Show resolved Hide resolved
docs/migrating_to_18.md Outdated Show resolved Hide resolved
docs/object-store.md Outdated Show resolved Hide resolved
docs/object-store.md Outdated Show resolved Hide resolved
docs/object-store.md Outdated Show resolved Hide resolved
docs/migrating_to_18.md Outdated Show resolved Hide resolved
py_bentoctl/other_helpers.py Outdated Show resolved Hide resolved
py_bentoctl/other_helpers.py Outdated Show resolved Hide resolved
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

lgtm. @v-rocheleau look good to you?

Copy link
Contributor

@v-rocheleau v-rocheleau left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@v-rocheleau v-rocheleau left a comment

Choose a reason for hiding this comment

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

Missed something

docs/object-store.md Outdated Show resolved Hide resolved
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.

3 participants