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.

Sign up for GitHub

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

Jump to bottom

S3 terraform fixes #15429

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

S3 terraform fixes #15429

wants to merge 3 commits into from

Conversation

hypesystem
Copy link

What this PR does / why we need it:

I followed the guide on using Terraform to set up S3 buckets for Loki storage in an EKS cluster.

I cleaned up the terraform files to eliminate warnings, and updated the guide to accurately reflect how the terraform module is used when installing the helm chart.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

The helm chart further assumes 3 buckets for s3 storage, but the terraform project only creates 1.

I have a solution for this locally, but I think it would make sense to update the docs and terraform project to reflect this. I am not sure what the impact would be/if the terraform project is also used elsewhere.

Let me know if you want me to work on an approach for this/point me in the right direction if possible.

Also note: I am rebasing to fix the commit naming, but creating the PR to get these notes out.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated (Not applicable)
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@hypesystem
Remove deprecated terraform aws_iam_policy inline_policy
dc7280e 
In terraform project for setting up s3 buckets for Loki, the inline_policy has been deprecated.
@hypesystem
S3 terraform: remove trailing spaces on line
96ea4b1
@hypesystem
Update docs on using terraform to create s3 buckets for accuracy
155a34b 
The oidc_provider variable is invalid for the Terraform module, which extracts the provide info from the given EKS cluster.
The terraform requires a namespace for the service account, which was undocumented here.
The s3 bucket name will not work as loki-data as s3 bucket names are global, so we should encourage setting it explicitly.
The syntax for applying a terraform module was incorrect: missing `apply` command and variables were given with incorrect syntax.
@hypesystem hypesystem requested a review from a team as a code owner December 16, 2024 13:24
@CLAassistant
Copy link

CLAassistant commented Dec 16, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
size/S type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hypesystem @CLAassistant