Skip to content

feat: Make S3 endpoint domain configurable #282

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

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

Conversation

tworcester
Copy link

Description

Make S3 Endpoint configurable. Also add defensive pattern check to make sure no extra period is added at the start of the string.
...

Related Issue

Fixes #280

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@tworcester tworcester requested a review from a team as a code owner March 16, 2025 03:18
@zachariahmiller
Copy link
Contributor

Hi @tworcester thanks for your contribution. After taking a look at this i think we'll probably want to take a slightly different approach that is more configurable and still maintains backwards compatibility. I'm thinking it will look something like:

  1. use the existing endpoint field instead of endpointDomain
  2. default the existing endpoint field to empty string
  3. update the template to set default values for $endpoint based on whether the provider field is minio or aws and have those defaults match the current implementation
  4. if the endpoint field is specified use that value instead.

The endpointDomain option is more flexible than what exists today, but the reality is we cannot necessarily assume the s3. part of the endpoint either as there are also potentially other endpoints such as s3-fips..ors3.dual-stack..`.

You're welcome to take a stab at this if you like, otherwise we'll try to prioritize this for the next release after the update to 17.9.2 that is currently in progress.

@tworcester tworcester force-pushed the make-s3-endpoint-configurable branch from 04f0f04 to fd01ac0 Compare March 20, 2025 21:57
@tworcester
Copy link
Author

tworcester commented Mar 20, 2025

Took a crack at it! Let me know what you think. @zachariahmiller

@zachariahmiller
Copy link
Contributor

Took a crack at it! Let me know what you think. @zachariahmiller

@tworcester At first glance i think this looks good, but I'm also realizing now it might not be enough for your use case without additional changes.

As it is today, for AWS we do not have to specify the endpoint in any of the sections besides backups. However, for your scenario where the s3 endpoint is mapped to a different DNS entry i don't think it will "just work" as it does right now for the connection and registry sections. I'm thinking that regionendpoint in registry and host or endpoint might need to be set in the connection section.

If that is in fact the case it should be pretty easy to adapt this PR to support that as well. Do you have an environment provisioned or easily provisioned with the alternate DNS scenario where you could test this easily? I do not, so it might be a little bit more involved for us to validate exactly what config will work.

Would the setup happen to be or be analogous to the endpoint specific DNS setup when using privatelink and the vpc interface endpoints for private s3 access? If so i can inquire with our infra team on the feasibility on getting an account provisioned with a similar setup, so i can do some validation.

@tworcester
Copy link
Author

Ah, good point! I will verify the changes in my environment and update the PR tomorrow.

Yes, the Private link and custom VPC Endpoints is a viable test case for what I am trying to achieve!

@zachariahmiller
Copy link
Contributor

Ah, good point! I will verify the changes in my environment and update the PR tomorrow.

Yes, the Private link and custom VPC Endpoints is a viable test case for what I am trying to achieve!

@tworcester Just following up to see if you had the opportunity to test this in your environment. No rush on our end, just wanted to make sure to reach out and see how it was going.

@tworcester
Copy link
Author

Apologies for the slow follow up, here is explicitly what I have deployed and working:

backups: |
  [default]
  region = ###ZARF_VAR_REGION###
  provider = AWS
  use_iam_profile = true
  host_base = s3.###ZARF_VAR_REGION###.###ZARF_VAR_S3_DOMAIN###
  host_bucket = %(bucket)s.s3.###ZARF_VAR_REGION###.###ZARF_VAR_S3_DOMAIN###
connection: |
  endpoint: https://s3.###ZARF_VAR_REGION###.###ZARF_VAR_S3_DOMAIN###
  provider: AWS
  region: ###ZARF_VAR_REGION###
  use_iam_profile: true
registry: |
  s3:
    bucket: ###ZARF_VAR_CLUSTER_NAME###-gitlab-registry
    region: ###ZARF_VAR_REGION###

@zachariahmiller
Copy link
Contributor

Apologies for the slow follow up, here is explicitly what I have deployed and working:

backups: |
  [default]
  region = ###ZARF_VAR_REGION###
  provider = AWS
  use_iam_profile = true
  host_base = s3.###ZARF_VAR_REGION###.###ZARF_VAR_S3_DOMAIN###
  host_bucket = %(bucket)s.s3.###ZARF_VAR_REGION###.###ZARF_VAR_S3_DOMAIN###
connection: |
  endpoint: https://s3.###ZARF_VAR_REGION###.###ZARF_VAR_S3_DOMAIN###
  provider: AWS
  region: ###ZARF_VAR_REGION###
  use_iam_profile: true
registry: |
  s3:
    bucket: ###ZARF_VAR_CLUSTER_NAME###-gitlab-registry
    region: ###ZARF_VAR_REGION###

No worries whatsoever!

I'm assuming the ###ZARF_VAR_CLUSTER_NAME### is specific to your env and the existing way the buckets are named would still work.

Have you actually tested the registry functions? I would think an endpoint would be required there too and I thought the upstream chart had been updated so registry deployment failed when it couldnt make the s3 connection, but if you could verify you can push something to the container registry i would feel much more confident in this working config.

If so, we can look at updating the secret template to allow for this config setup.

@tworcester
Copy link
Author

Confirmed, the above works in three separate deployments right now and I have verified that docker/ folder in the registry bucket has layers stored in it!

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.

S3 endpoint is hardcoded
2 participants