fix:The number of AWS load balancers does not match the actual count #86
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactored ELB collector to reuse a single EC2 client for VPC and security group lookups and increased DescribeLoadBalancers page size to prevent AWS rate limiting and missing load balancers. Sequence diagram for ELB detail retrieval with shared EC2 clientsequenceDiagram
participant Collector
participant Services
participant ELBClient
participant EC2Client
Collector->>Services: InitServices()
Services->>ELBClient: create ELB client
Services->>EC2Client: create EC2 client
Collector->>ELBClient: describeELBs()
ELBClient-->>Collector: list of ELBs
loop for each ELB
Collector->>EC2Client: DescribeVPCDetailsByFilters(vpc-id)
EC2Client-->>Collector: VPC details
Collector->>EC2Client: DescribeSecurityGroupDetailsByFilters(group-id)
EC2Client-->>Collector: Security group details
end
Collector-->>Services: ELBDetails
Entity relationship diagram for ELBDetail, VPC, and SecurityGrouperDiagram
ELBDetail {
string ELB
string VPC
string[] SecurityGroups
}
VPCDetail {
string VpcId
}
SecurityGroupDetail {
string GroupId
}
ELBDetail ||--|{ VPCDetail : contains
ELBDetail ||--|{ SecurityGroupDetail : contains
Class diagram for updated AWS Services and ELBDetail retrievalclassDiagram
class Services {
+string Region
+EC2 *ec2.Client
+ELB *elasticloadbalancingv2.Client
+InitServices(cloudAccountParam)
}
class ELBDetail {
+ELB types.LoadBalancer
+VPC VPCDetail
+SecurityGroups []SecurityGroupDetail
}
Services --> EC2 : uses
Services --> ELB : uses
ELBDetail --> VPCDetail : contains
ELBDetail --> SecurityGroupDetail : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @Lyc-heng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where the count of AWS load balancers was inaccurate due to frequent AWS API rate limiting. The fix involves a significant refactoring of how AWS EC2 clients are initialized and utilized within the collector. By ensuring that EC2 clients are created only once and then reused for subsequent calls, the changes prevent excessive API requests, thereby mitigating rate limiting issues and ensuring a complete and accurate retrieval of load balancer data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `collector/aws/collector/elasticloadbalancing/elasticloadbalancingv2.go:105-106` </location>
<code_context>
func describeELBs(ctx context.Context, c *elasticloadbalancingv2.Client) (elbs []types.LoadBalancer, err error) {
- input := &elasticloadbalancingv2.DescribeLoadBalancersInput{}
+ input := &elasticloadbalancingv2.DescribeLoadBalancersInput{
+ PageSize: aws.Int32(400),
+ }
output, err := c.DescribeLoadBalancers(ctx, input)
</code_context>
<issue_to_address>
**suggestion:** Hardcoded PageSize may not be optimal for all AWS accounts.
Making PageSize configurable or implementing pagination will help ensure all load balancers are retrieved, regardless of account size or future API changes.
Suggested implementation:
```golang
func describeELBs(ctx context.Context, c *elasticloadbalancingv2.Client, pageSize int32) (elbs []types.LoadBalancer, err error) {
var marker *string
elbs = []types.LoadBalancer{}
for {
input := &elasticloadbalancingv2.DescribeLoadBalancersInput{
PageSize: aws.Int32(pageSize),
Marker: marker,
}
output, err := c.DescribeLoadBalancers(ctx, input)
if err != nil {
log.CtxLogger(ctx).Warn("DescribeLoadBalancers error", zap.Error(err))
return nil, err
}
elbs = append(elbs, output.LoadBalancers...)
if output.NextMarker == nil {
break
}
marker = output.NextMarker
}
```
- You will need to update all calls to `describeELBs` to pass the desired `pageSize` (e.g., `describeELBs(ctx, client, 400)`).
- Consider making `pageSize` a configurable value (e.g., from a config file or environment variable) if you want to avoid hardcoding it in function calls.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| input := &elasticloadbalancingv2.DescribeLoadBalancersInput{ | ||
| PageSize: aws.Int32(400), |
There was a problem hiding this comment.
suggestion: Hardcoded PageSize may not be optimal for all AWS accounts.
Making PageSize configurable or implementing pagination will help ensure all load balancers are retrieved, regardless of account size or future API changes.
Suggested implementation:
func describeELBs(ctx context.Context, c *elasticloadbalancingv2.Client, pageSize int32) (elbs []types.LoadBalancer, err error) {
var marker *string
elbs = []types.LoadBalancer{}
for {
input := &elasticloadbalancingv2.DescribeLoadBalancersInput{
PageSize: aws.Int32(pageSize),
Marker: marker,
}
output, err := c.DescribeLoadBalancers(ctx, input)
if err != nil {
log.CtxLogger(ctx).Warn("DescribeLoadBalancers error", zap.Error(err))
return nil, err
}
elbs = append(elbs, output.LoadBalancers...)
if output.NextMarker == nil {
break
}
marker = output.NextMarker
}- You will need to update all calls to
describeELBsto pass the desiredpageSize(e.g.,describeELBs(ctx, client, 400)). - Consider making
pageSizea configurable value (e.g., from a config file or environment variable) if you want to avoid hardcoding it in function calls.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the AWS rate limiting issue by reusing the EC2 client, which prevents the creation of multiple clients during load balancer discovery. The addition of PageSize to the DescribeLoadBalancers call is also a good optimization to reduce API calls. The implementation is sound. I have one minor suggestion to improve code maintainability by removing an unused field.
|
|
||
| // Services contains regional client of AWS services | ||
| type Services struct { | ||
| Region string |
There was a problem hiding this comment.
The Region field has been added to the Services struct, but it is not used anywhere in the codebase. To keep the code clean and avoid potential confusion, it's best to remove this unused field.
1 participant
Thank you for your contribution to CloudRec!
What About:
java)go)opa)Description:
Fixed the issue of missing counts when retrieving AWS load balancers. The reason was that the original AWS LB retrieval repeatedly created EC2 clients (each LB query would create 2 EC2 clients), causing rate limiting by AWS after a large number of requests, making it impossible to retrieve data for a period of time.
Summary by Sourcery
Reuse a single EC2 client when retrieving ELB details and increase the DescribeLoadBalancers page size to prevent AWS rate limiting and ensure accurate load balancer counts
Bug Fixes:
Enhancements: