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

chore: add docs for wiring in pre-reqs for S3/MetalLB #1235

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Racer159
Copy link
Contributor

@Racer159 Racer159 commented Jan 27, 2025

Description

Adds some docs for S3 pre-reqs and mentions the MetalLB package

Related Issue

Fixes #N/A

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)

Steps to Validate

  • If this PR introduces new functionality to UDS Core or addresses a bug, please document the steps to test the changes.

Checklist before merging

@Racer159 Racer159 requested a review from a team as a code owner January 27, 2025 23:47
Copy link
Contributor

@UnicornChance UnicornChance left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@mjnagel mjnagel left a comment

Choose a reason for hiding this comment

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

A few minor comments, appreciate you taking the time to test this out for us! One other note we may want to add, while the examples are values, for a prod bundle the end user would likely want variables for connecting to cloud storage at least.

Comment on lines +191 to +192
overrides:
loki:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add the overrides to lock down the egress traffic to strictly what is necessary here (uds-loki-config chart has some values for storage egress that when not specified default to egress anywhere).

Comment on lines +295 to +296
overrides:
velero:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here on the egress policy overrides, see the values here.

Comment on lines +317 to +319
:::caution
If you are using the in-cluster Minio Operator UDS Package for backups you must ensure that the volumes that back that storage are themselves backed up! Cluster or deployment issues may result in a loss of Minio as well as the application you intend to back up.
:::
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, but I wonder if there's a better place to put this caution note given that it applies to both Loki and Velero (and is currently "nested" under the Velero section). We could just copy/paste it to Loki but that doesn't feel right 😅

Comment on lines +149 to +150
loki:
values:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this confused me initially (especially when comparing against the minio example) - can we add the boilerplate of overrides, package, component, etc?

Comment on lines +250 to +251
velero:
values:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as loki on adding the boilerplate to the example.

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