Skip to content

Conversation

@GabrielYamin
Copy link

@GabrielYamin GabrielYamin commented Jan 28, 2026

Summary by CodeRabbit

  • Documentation
    • Added docs for S3 output encryption options: sse and sse_kms_key_id, describing SSE-S3, SSE-KMS, and DSSE-KMS modes.
    • Added configuration examples demonstrating SSE-KMS usage for S3 outputs across common deployment and PutObject-related scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@GabrielYamin GabrielYamin requested review from a team and eschabell as code owners January 28, 2026 14:47
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 28, 2026

📝 Walkthrough

Walkthrough

The S3 output plugin docs add two public configuration parameters, sse and sse_kms_key_id, and include example Fluent Bit configuration blocks demonstrating SSE-KMS usage for general uploads and PutObject scenarios.

Changes

Cohort / File(s) Summary
S3 Output Plugin Documentation
pipeline/outputs/s3.md
Added sse and sse_kms_key_id configuration docs describing SSE-S3, SSE-KMS, and DSSE-KMS. Inserted example Fluent Bit snippets (fluent-bit.yaml, fluent-bit.conf) showing SSE-KMS for uploads and PutObject usage.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

4.2.2

Poem

🐰 I hopped through docs, with keys held tight,
SSE whispers, KMS beams bright.
Examples nestled in YAML and conf,
Buckets sleep sound, encryption turned on. 🔐

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'docs: s3: SSE configuration docs' directly and clearly summarizes the main change: adding Server-Side Encryption (SSE) configuration documentation to the S3 output plugin documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@patrick-stephens
Copy link
Contributor

@GabrielYamin I think this is functionality waiting for the PR to be merged right?

@GabrielYamin
Copy link
Author

GabrielYamin commented Jan 28, 2026

@patrick-stephens, The BL PR is still in draft, I'll be opening it for review very soon.
fluent/fluent-bit#11410
Thank you for the fast response!!!

@GabrielYamin
Copy link
Author

PR is ready for review @patrick-stephens
Thanks :)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pipeline/outputs/s3.md (1)

117-132: Document required KMS permissions for SSE-KMS encryption.

The permissions section only lists s3:PutObject, but when using sse: aws:kms or sse: aws:kms:dsse, additional KMS permissions are required. Without these, uploads will fail with permission errors.

Required KMS permissions:

  • kms:GenerateDataKey - Required for encrypting objects
  • kms:Decrypt - Required if using customer-managed keys
📋 Suggested documentation update

Add a new subsection after line 132:

### Additional permissions for SSE-KMS

When using server-side encryption with AWS KMS (`sse: aws:kms` or `sse: aws:kms:dsse`), the following KMS permissions are also required:

```json
{
  "Version": "2012-10-17",
  "Statement": [{
    "Effect": "Allow",
    "Action": [
      "kms:GenerateDataKey",
      "kms:Decrypt"
    ],
    "Resource": "arn:aws:kms:region:account-id:key/key-id"
  }]
}

Replace region, account-id, and key-id with your specific KMS key details. If using the AWS-managed S3 key (when sse_kms_key_id is not specified), permissions are managed automatically.

</details>

</blockquote></details>

</blockquote></details>
🧹 Nitpick comments (1)
pipeline/outputs/s3.md (1)

579-617: Consider adding examples for SSE-S3 and DSSE-KMS.

The example demonstrates SSE-KMS effectively, but users may also need guidance on:

  • SSE-S3 (simpler, no KMS key required): Just set sse: AES256
  • DSSE-KMS (dual-layer encryption): Use sse: aws:kms:dsse with a KMS key ARN

Adding brief examples of these variants would provide more complete documentation coverage.

📝 Suggested additional examples

Add after line 617:

An example using SSE-S3 encryption (S3-managed keys):

{% tabs %}
{% tab title="fluent-bit.yaml" %}

```yaml
pipeline:

  outputs:
    - name: s3
      match: '*'
      bucket: your-bucket
      region: us-east-1
      sse: AES256

{% endtab %}
{% tab title="fluent-bit.conf" %}

[OUTPUT]
  Name     s3
  Match    *
  bucket   your-bucket
  region   us-east-1
  sse      AES256

{% endtab %}
{% endtabs %}

</details>

</blockquote></details>

</blockquote></details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@patrick-stephens
Copy link
Contributor

@GabrielYamin I do agree with Vale in some of the comments that it is a bit acronym/initialism heavy so useful to introduce the acronym/initialism. Can you check and resolve any comments raised by CI?

@GabrielYamin
Copy link
Author

GabrielYamin commented Jan 29, 2026

@patrick-stephens Do you think that if I introduce the acronyms in the SSE config definition it would suffice for the key_id config? or should I introduce them in both?
My assumption is that whoever is using SSE is probably familiar with the terms and the definition is mentioned in the description (other than KMS which I can add)
I don't want to make the description too long.

@patrick-stephens
Copy link
Contributor

patrick-stephens commented Jan 29, 2026

Yeah I'd just introduce it somewhere at the top or use the full name the first time it is present in the table/text - whilst I agree most folks should be aware I would still err on the side of caution. You don't need to define the term but at least clarify what the acronym/initialism stands for.

It's also pretty easy to get different definitions sometimes for the same 3 letters so being explicit never hurts. I'm not sure if there are conflicting definitions in this case but you can never know all possible usage now and in the future - it doesn't take much effort to be clear and resolves any potential issue (even if there are none!).

If anything is already covered as well but CI is flagging it then just make a comment to that effect and resolve it.

@GabrielYamin
Copy link
Author

GabrielYamin commented Jan 29, 2026

I have updated the descriptions, let me know if it's good now or you want me to adjust it a bit more.

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