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

Investigate on aws_partition parameter #23153

Closed
kaiyan-sheng opened this issue Dec 15, 2020 · 11 comments · Fixed by #23539
Closed

Investigate on aws_partition parameter #23153

kaiyan-sheng opened this issue Dec 15, 2020 · 11 comments · Fixed by #23539
Assignees
Labels
bug Team:Platforms Label for the Integrations - Platforms team

Comments

@kaiyan-sheng
Copy link
Contributor

#19423 added the aws_partition config into aws module. But this is still limited the region to cn-north-1 or us-gov-east-1 or default to us-east-1.

#16263 added endpoint config. Both region name and endpoint are used in EnrichAWSConfigWithEndpoint function, which enables endpoint resolver for AWS service clients.

We need to spend some time revisiting these two config options for private regions use case and better document these two config options.

@kaiyan-sheng kaiyan-sheng self-assigned this Dec 15, 2020
@kaiyan-sheng kaiyan-sheng added the Team:Platforms Label for the Integrations - Platforms team label Dec 15, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@mbarretta
Copy link

Just wanted to add that it's important for users to be able to specify undocumented region/endpoint/partition values as well, since not all private AWS regions are documented.

Also, this same idea of configuration-driven cloud endpoints should extend to GCP and Azure, which similarly have private regions

@kaiyan-sheng
Copy link
Contributor Author

Hi @mbarretta, I just looked into the code around aws_partition and I think it should work with your use case. When the user specifies aws_partition equals to aws-us-gov for example, us-gov-east-1 is given to be the init region to start making AWS API calls(GetCallerIdentity, ListAccountAliases, DescribeRegions). If user wants to collect all AWS EC2 CloudWatch metrics from one specific region, regions config can be used with aws_partition together. For example:

- module: aws
  period: 5m
  credential_profile_name: elastic-beats
  aws_partition: aws-us-gov
  regions:
    - us-gov-west-1
  metricsets:
    - ec2

Will this solve your use case?

@kaiyan-sheng
Copy link
Contributor Author

PR to add aws-iso and aws-iso-b partition and endpoint: #23209

@mbarretta
Copy link

@kaiyan-sheng my concern is for partition values not harded coded in our implementation. It's a shortcut, yes, by why can't users forego the partition value and just provide the necessary region and endpoint values directly?

It seems like that's what the endpoint setting was added to do (at least that was what I was asking for), but it wasn't implemented everywhere it was needed, leading to the ticket where you added the aws_partition value.

Given that the AWS endpoints follow the form https://{service}.{region}.{endpoint}, then there should be no need for aws_partition at all except as a shortcut. And if that's its only purpose, it might just add confusion and maybe better dropped in favor of the more explicit and customizable region/endpoint combo.

@kaiyan-sheng
Copy link
Contributor Author

Thanks @mbarretta for the input! That's a good point! I got several concerns/questions:

  1. We already have a config parameter called regions, which is to define what are the regions user wants to collect metrics from. But we also need an InitRegion for making some initial AWS API calls and this is where the aws_partition plays a role. For example: User can give no regions(optional) in the config but give aws_partition = aws-us-gov. This means the user wants to collect metrics from all regions under this AWS partition, which includes both us-gov-west-1 and us-gov-east-1. We get this InitRegion from the hardcoded switch case but it doesn't mean we are only collecting metrics from the hardcoded regions for each AWS partition.
  2. Unfortunately I don't have the environment to test this with other AWS partitions. Do you know if DescribeRegions API work with other AWS partitions?
  3. At this point, I'm more leaning towards deprecating the endpoint config parameter for Metricbeat. Because both endpoint and region can be derived from aws_partition. For example: if user wants to collect EC2 metrics from all regions in aws-us-gov partition. The config can be:
- module: aws
  period: 5m
  credential_profile_name: elastic-beats
  aws_partition: aws-us-gov
  metricsets:
    - ec2

If user wants to only collect EC2 metrics from one specific region, they can specify that using the regions parameter:

- module: aws
  period: 5m
  credential_profile_name: elastic-beats
  aws_partition: aws-us-gov
  regions:
    - us-gov-west-1
  metricsets:
    - ec2

@mbarretta
Copy link

mbarretta commented Jan 4, 2021

@kaiyan-sheng

Overall, while I like the ease of configuration offered by aws_partition, I don't think it should be the only configuration option for "non-default" regions like aws-cn, aws-us-gov, etc. My primary objection is that we don't know if those region/endpoint values that your PR hardcodes are static or that the list of supported partitions is exhaustive. In either case, we'd need to continue to update our integrations every time AWS changes, adds, or removes regions, endpoints, or services.

I'd strongly prefer that users have the ability to override any/all values that we try to automagically provide.

You mention the DescribeRegions function, and I think that is indeed a likely answer. Since you only need the service and endpoint to make that call, the only real configuration value needed is endpoint

So I guess I'm on the opposite end of things. I'd prefer to deprecate aws_partition and go with endpoint

@mbarretta
Copy link

@mukeshelastic curious about your opinion here.

@masci
Copy link

masci commented Jan 5, 2021

+1 for having users to set endpoint and remove everything else. While I see the rationale behind aws_partition and region, this abstraction is something I would expect in a UI for example, not in a configuration file. The switch/case hardcoding the actual endpoints is a yellow flag in the code that in my opinion validates the UX problem - the only hardcoded value we should have in the code is a sane default for optional params.

@mukeshelastic
Copy link

@mbarretta - @sorantis is closer to these integration details than me he can share his perspective from product and UX perspective.

@kaiyan-sheng
Copy link
Contributor Author

@mbarretta Thanks for all the input on this! I just created a PR to deprecate aws_partition and fully rely on endpoint instead. It's still working in progress but please feel free to take a look and let me know what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Team:Platforms Label for the Integrations - Platforms team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants