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

[Cloudflare Logpush] Add non_aws_bucket_name option to AWS S3 input #8278

Merged

Conversation

chemamartinez
Copy link
Contributor

@chemamartinez chemamartinez commented Oct 24, 2023

Proposed commit message

It exposes the non_aws_bucket_name of the AWS S3 input for the Cloudflare Logpush integration so users are able to collect logs from a 3rd party S3 bucket. Apart from the global setting, a local one for each data stream has been added so a different bucket can be set for each data stream.

See more about how to use the non_aws_bucket_name here.

The new parameter is called Cloudflare R2 bucket name, as Cloudflare R2 is the S3-compatible storage service that Cloudflare provides by default, so it is assumed to be the one used by its users. Nevertheless, it is also noted that the parameter can be used to specify any other S3-compliant service.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Screenshots

image

@chemamartinez chemamartinez marked this pull request as ready for review October 24, 2023 16:37
@chemamartinez chemamartinez requested a review from a team as a code owner October 24, 2023 16:37
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@elasticmachine
Copy link

elasticmachine commented Oct 24, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-10-25T11:47:32.058+0000

  • Duration: 28 min 48 sec

Test stats 🧪

Test Results
Failed 0
Passed 115
Skipped 0
Total 115

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Oct 24, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (18/18) 💚
Files 100.0% (18/18) 💚 2.941
Classes 100.0% (18/18) 💚 2.941
Methods 100.0% (215/215) 💚 8.877
Lines 91.959% (4883/5310) 👎 -0.761
Conditionals 100.0% (0/0) 💚

multi: false
required: false
show_user: true
description: "This can replace Bucket ARN with a Bucket Name for collecting logs from a 3rd party S3 compatible service. It will override the global Non AWS Bucket Name if provided."
Copy link
Contributor

@efd6 efd6 Oct 24, 2023

Choose a reason for hiding this comment

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

I think it would be helpful to have a brief section in the global docs about this. Probably something similar to the text in the docs that you link to in the PR description. Though maybe what you have in the package manifest is enough.

{{/each}}
{{#contains "forwarded" tags}}
publisher_pipeline.disable_host: true
{{/contains}}
{{#if processors}}
processors:
{{processors}}
{{/if}}
{{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Final new line (and below).

multi: false
required: false
show_user: true
description: "This can replace Bucket ARN with a Bucket Name for collecting logs from a 3rd party S3 compatible service. This is a global setting which can be overriden by specific local bucket names for each data stream if required. \nUsing non-AWS S3 compatible buckets requires the use of Access Key ID and Secret Access Key for authentication. To specify the S3 bucket name, use the non_aws_bucket_name config and the endpoint must be set to replace the default API endpoint."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: "This can replace Bucket ARN with a Bucket Name for collecting logs from a 3rd party S3 compatible service. This is a global setting which can be overriden by specific local bucket names for each data stream if required. \nUsing non-AWS S3 compatible buckets requires the use of Access Key ID and Secret Access Key for authentication. To specify the S3 bucket name, use the non_aws_bucket_name config and the endpoint must be set to replace the default API endpoint."
description: |-
This can replace Bucket ARN with a Bucket Name for collecting logs from a 3rd party S3 compatible service. This is a global setting which can be overriden by specific local bucket names for each data stream if required.
Using non-AWS S3 compatible buckets requires the use of Access Key ID and Secret Access Key for authentication. To specify the S3 bucket name, use the non_aws_bucket_name config and the endpoint must be set to replace the default API endpoint.

multi: false
required: false
show_user: true
description: "This can replace Bucket ARN with a Bucket Name for collecting logs from a 3rd party S3 compatible service. This is a global setting which can be overriden by specific local bucket names for each data stream if required. \nUsing non-AWS S3 compatible buckets requires the use of Access Key ID and Secret Access Key for authentication. To specify the S3 bucket name, use the non_aws_bucket_name config and the endpoint must be set to replace the default API endpoint."
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the wording should be To specify the non-AWS S3 bucket name, use the Non AWS Bucket Name config

Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

Copy link
Contributor

@bhapas bhapas left a comment

Choose a reason for hiding this comment

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

Can you add system test to run on CI. Probably, Localstack as a Non-AWS service?

@jamiehynds
Copy link

@chemamartinez should we considering renaming thenon_aws_bucket_name setting to Cloudflare R2. Non AWS Bucket Name probably isn't going to resonate with Cloudflare users, if they are looking to ingest via R2, so maybe worth making the name and description more relevant to the Cloudflare integration?

@bhapas
Copy link
Contributor

bhapas commented Oct 25, 2023

Can you add system test to run on CI. Probably, Localstack as a Non-AWS service?

I think the system tests can be added in #7034

Copy link
Contributor

@bhapas bhapas left a comment

Choose a reason for hiding this comment

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

LGTM.. You can proceed to merge after @jamiehynds approval

@chemamartinez chemamartinez force-pushed the cloudflare_logpush_non_aws_bucket branch from 655cf3f to da179ed Compare October 25, 2023 09:40
@chemamartinez
Copy link
Contributor Author

@chemamartinez should we considering renaming thenon_aws_bucket_name setting to Cloudflare R2. Non AWS Bucket Name probably isn't going to resonate with Cloudflare users, if they are looking to ingest via R2, so maybe worth making the name and description more relevant to the Cloudflare integration?

@jamiehynds sure! Looks like more user-friendly this way. Here you have the requested changes: da179ed.

@chemamartinez chemamartinez merged commit 19ba187 into elastic:main Oct 25, 2023
4 checks passed
@elasticmachine
Copy link

Package cloudflare_logpush - 1.16.0 containing this change is available at https://epr.elastic.co/search?package=cloudflare_logpush

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:Cloudflare Logpush Cloudflare Logpush
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants