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: Add permissions for S3 write on FluentBit addons; add dependency on EKS addon for generic helm_releases #203

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

bryantbiggs
Copy link
Contributor

What does this PR do?

  • Add permissions for S3 write on FluentBit addons;
  • Add dependency on EKS addon for generic helm_releases

Motivation

  • FluentBit supports writing to multiple locations including S3; this adds the permissions to send logs to S3 in addition to Cloudwatch (or not send to CloudWatch and only send to S3 - any combination)
  • Since we deploy the EBS CSI driver in the EKS addons, and a number of helm charts require EBS CSI, I've added a dependency between these. I didn't go through and check our other addons, but potentially if they rely on EKS addons such as EBS CSI, then we can add the same dependency on them. This came up when deploying Weaviate using the generic helm_releases which requires EBS CSI and it failed since the CSI driver wasn't ready in time

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I ran pre-commit run -a with this PR

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@bryantbiggs bryantbiggs requested a review from a team as a code owner July 11, 2023 22:21
resources = [
"arn:${local.partition}:logs:${local.region}:${local.account_id}:log-group:${try(var.aws_for_fluentbit_cw_log_group.name, "*")}:log-stream:*",
]
dynamic "statement" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing these blocks to be dynamic is not a breaking change, it will not show up in a diff for users. That only occurs at the resource level

enable_fargate_fluentbit = true
enable_aws_for_fluentbit = true
aws_for_fluentbit = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

somewhat contrived since in order for this to actually send logs to S3, the values output would need to be updated for S3. But, it does test and validate the permission change works as intended

Ref: #118

@@ -68,4 +68,9 @@ resource "helm_release" "this" {
type = try(set_sensitive.value.type, null)
}
}

depends_on = [
# Wait for EBS CSI, etc. to be installed first
Copy link
Contributor

Choose a reason for hiding this comment

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

What if EBS CSI is not enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing in particular. If no addons are passed in via eks_addons then it will just treat it as a no-op. If any addons are passed in (kube-proxy, VPC CNI, etc.) then it will ensure those are provisioned before the helm_releases which should be what we want for 99% of the time.

@bryantbiggs bryantbiggs merged commit c3675b1 into main Jul 12, 2023
5 checks passed
@bryantbiggs bryantbiggs deleted the feat/wait-and-s3-bucket branch July 12, 2023 00:00
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.

2 participants