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

[AWS] Expose default region config #4158

Merged
merged 8 commits into from
Sep 20, 2022

Conversation

legoguy1000
Copy link
Contributor

@legoguy1000 legoguy1000 commented Sep 7, 2022

What does this PR do?

Expose default region config to the UI. This allows users that utilize non-regular AWS environments to set the initial region which sets the correct endpoints to use with such as IAM which will be called prior to service specific endpoints. For the SaaS solutions such as Cloudflare, Crowdstrike, Cisco Umbrella, this change allows for integration to AWS Gov-CLoud or AWS China. The changes to AWS and AWS Logs integrations allow for AWS Gov-Cloud, AWS China, US Gov AWS isolated regions.

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.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@legoguy1000 legoguy1000 requested a review from a team as a code owner September 7, 2022 20:30
@elasticmachine
Copy link

elasticmachine commented Sep 7, 2022

💚 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: 2022-09-19T19:12:06.195+0000

  • Duration: 43 min 0 sec

Test stats 🧪

Test Results
Failed 0
Passed 229
Skipped 2
Total 231

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@aspacca
Copy link
Contributor

aspacca commented Sep 8, 2022

that's amazing @legoguy1000 , thank you very much!

I was just discussing about making this change. :)

If you don't mind I will go further and remove endpoint but for where non_aws_bucket_name is defined (it should be only in packages/aws_logs/data_stream/generic/agent/stream/aws-s3.yml.hbs)

@aspacca
Copy link
Contributor

aspacca commented Sep 8, 2022

@aspacca
Copy link
Contributor

aspacca commented Sep 8, 2022

/test

Copy link
Contributor

@aspacca aspacca left a comment

Choose a reason for hiding this comment

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

missing packages/aws/data_stream/ecs_metrics/agent/stream/stream.yml.hbs

packages/aws/manifest.yml Outdated Show resolved Hide resolved
packages/aws_logs/manifest.yml Outdated Show resolved Hide resolved
@aspacca
Copy link
Contributor

aspacca commented Sep 8, 2022

kibana.version must be bumped to at least ^8.0.0

default_region is available since then https://github.com/elastic/beats/blob/v8.0.0/x-pack/libbeat/common/aws/credentials.go#L43

@elasticmachine
Copy link

elasticmachine commented Sep 8, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (23/23) 💚
Files 96.774% (30/31) 👎 -0.71
Classes 96.774% (30/31) 👎 -0.71
Methods 90.099% (364/404) 👎 -0.143
Lines 95.164% (10568/11105) 👍 3.851
Conditionals 100.0% (0/0) 💚

@legoguy1000
Copy link
Contributor Author

@aspacca Do u think I should just remove endpoint entirely from the manifest for the AWS integration? As i went through removing endpoint from the agent configs, realized that we shouldn't need it at all in the entire AWS package.

@aspacca
Copy link
Contributor

aspacca commented Sep 8, 2022

Do u think I should just remove endpoint entirely from the manifest for the AWS integration? As i went through removing endpoint from the agent configs, realized that we shouldn't need it at all in the entire AWS package.

yes, let's go for it. also in the other integrations where is not needed anymore

@legoguy1000
Copy link
Contributor Author

ok, I left it in the aws_logs since it could be used to pull logs in via non-AWS S3 compatible services. I also left it in crowdstrike and Cisco Umbrella as I suspect AWS is the only S3 compatible service they support but figured that might not always be the case so having it be in the config doesn't hurt, especially since it defaults to blank.

@aspacca
Copy link
Contributor

aspacca commented Sep 8, 2022

I also left it in crowdstrike and Cisco Umbrella as I suspect AWS is the only S3 compatible service they support but figured that might not always be the case so having it be in the config doesn't hurt, especially since it defaults to blank.

for me it's fine.

we can ping someone from @elastic/security-external-integrations to validate on this anyway :)

cc @P1llus , any hint ? thanks

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

/test

@legoguy1000
Copy link
Contributor Author

Seems good to merge??

@aspacca
Copy link
Contributor

aspacca commented Sep 19, 2022

@legoguy1000 we need a final approval from @elastic/security-external-integrations

packages/aws_logs/manifest.yml Show resolved Hide resolved
packages/crowdstrike/manifest.yml Show resolved Hide resolved
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Tentatively (and modulo the change in version number); it would be nice to have a more clear PR description with 'what' and 'why' that could be the basis of the commit message.

@legoguy1000
Copy link
Contributor Author

@aspacca @efd6 updated.

@elasticmachine
Copy link

elasticmachine commented Sep 19, 2022

🚀 Benchmarks report

Package aws 👍(7) 💚(4) 💔(2)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
cloudfront_logs 2164.5 1845.02 -319.48 (-17.32%) 💔
cloudwatch_logs 500000 333333.33 -166666.67 (-50%) 💔

Package cisco_umbrella 👍(0) 💚(0) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
log 9009.01 7246.38 -1762.63 (-24.32%) 💔

Package cloudflare_logpush 👍(6) 💚(1) 💔(0)

Package crowdstrike 👍(1) 💚(1) 💔(0)

To see the full report comment with /test benchmark fullreport

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.

5 participants