feat(alicloud): aggregate cloudfw policies and assets via DescribeAss…#105
feat(alicloud): aggregate cloudfw policies and assets via DescribeAss…#105j3ttt wants to merge 2 commits intoantgroup:mainfrom
Conversation
Summary of ChangesHello @j3ttt, 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 significantly enhances the Alicloud Cloud Firewall collector by enabling the aggregation of both firewall policies and their associated assets. The changes streamline the data collection process by consolidating related information into a single, comprehensive data structure, providing a more complete view of Cloud Firewall instances. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![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 refactors the Cloud Firewall collector to aggregate policies and assets before sending them to the results channel. This is a positive change that improves data cohesion and reduces channel communication overhead. The introduction of the describeAssetList function for fetching assets with proper pagination is well-implemented. Additionally, this PR fixes a pre-existing bug that caused policy collection to terminate prematurely. I've identified one issue regarding an incorrect string comparison used for pagination that could lead to erroneous behavior.
| if bd.TotalCount == nil || strconv.Itoa(count) >= *bd.TotalCount { | ||
| cancel() | ||
| return nil | ||
| break | ||
| } |
There was a problem hiding this comment.
The string comparison strconv.Itoa(count) >= *bd.TotalCount is incorrect and can lead to bugs. For example, in string comparison, "9" > "10" is true. You should convert *bd.TotalCount to an integer before comparing it with count. This is already handled correctly in the new describeAssetList function where TotalCount is an integer type.
if bd.TotalCount == nil {
break
}
totalCount, err := strconv.Atoi(*bd.TotalCount)
if err != nil {
log.CtxLogger(ctx).Warn("failed to parse TotalCount", zap.String("totalCount", *bd.TotalCount), zap.Error(err))
break
}
if count >= totalCount {
break
}
|
/gemini review |
|
@gemini-code-assist /gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Alicloud CloudFW collector to aggregate policies and assets before sending them, which is a good improvement over streaming individual items. The implementation introduces a new function describeAssetList and modifies GetInstanceDetail to support this aggregation. The changes are logical and align with the goal of the pull request. I have identified a few areas for improvement regarding code clarity, robustness against context cancellation, and consistency in API rate limiting.
| for { | ||
| request := &cloudfw20171207.DescribeAssetListRequest{ | ||
| CurrentPage: tea.String(strconv.Itoa(page)), | ||
| PageSize: tea.String(strconv.Itoa(pageSize)), | ||
| } | ||
|
|
||
| resp, err := cli.DescribeAssetListWithOptions(request, collector.RuntimeObject) | ||
| if err != nil { | ||
| log.CtxLogger(ctx).Warn("describe asset list error", zap.Error(err)) | ||
| return assets | ||
| } | ||
| if resp == nil || resp.Body == nil { | ||
| return assets | ||
| } | ||
|
|
||
| assets = append(assets, resp.Body.Assets...) | ||
| collected += int32(len(resp.Body.Assets)) | ||
|
|
||
| if resp.Body.TotalCount == nil || collected >= *resp.Body.TotalCount || len(resp.Body.Assets) == 0 { | ||
| return assets | ||
| } | ||
|
|
||
| page += 1 | ||
| } |
There was a problem hiding this comment.
The pagination loop in describeAssetList doesn't check for context cancellation. If the context is cancelled (e.g., due to a timeout), this loop will continue to run until all pages are fetched. This can lead to goroutines not terminating promptly and holding up resources. It's important to check for ctx.Done() in each iteration, similar to the pattern used in GetInstanceDetail.
for {
select {
case <-ctx.Done():
log.CtxLogger(ctx).Warn("context cancelled during asset list description")
return assets
default:
request := &cloudfw20171207.DescribeAssetListRequest{
CurrentPage: tea.String(strconv.Itoa(page)),
PageSize: tea.String(strconv.Itoa(pageSize)),
}
resp, err := cli.DescribeAssetListWithOptions(request, collector.RuntimeObject)
if err != nil {
log.CtxLogger(ctx).Warn("describe asset list error", zap.Error(err))
return assets
}
if resp == nil || resp.Body == nil {
return assets
}
assets = append(assets, resp.Body.Assets...)
collected += int32(len(resp.Body.Assets))
if resp.Body.TotalCount == nil || collected >= *resp.Body.TotalCount || len(resp.Body.Assets) == 0 {
return assets
}
page += 1
}
}| return assets | ||
| } | ||
|
|
||
| page += 1 |
There was a problem hiding this comment.
The loop in GetInstanceDetail includes a 1-second sleep (time.Sleep(1 * time.Second)) on line 92, presumably for rate limiting. However, this pagination loop in describeAssetList lacks a similar delay. This inconsistency could lead to hitting API rate limits when fetching assets. For consistency and to prevent potential issues, I recommend adding a similar sleep in this loop.
page += 1
time.Sleep(1 * time.Second)
There was a problem hiding this comment.
Code Review
This pull request refactors the Alicloud CloudFW collector to aggregate policies and assets before sending them to the results channel. This is a good architectural improvement over sending each item individually. The change also correctly fixes a pagination logic bug that would have prevented collecting policies for all firewall directions. I have a few suggestions to improve the implementation further, mainly regarding context handling in the new pagination loop, removing some redundant code, and using constants for magic numbers.
| pageSize := 50 | ||
| var collected int32 = 0 | ||
|
|
||
| for { |
There was a problem hiding this comment.
The for loop in describeAssetList does not check for context cancellation. If this function takes a long time to run (e.g., due to a large number of assets requiring many pages of API calls), it will not honor the timeout or cancellation signal from the parent context. This can lead to goroutine leaks and unresponsive behavior. Please add a select block to check ctx.Done() at the beginning of each loop iteration, similar to the pattern used in GetInstanceDetail.
| for { | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| log.CtxLogger(ctx).Warn("describeAssetList context cancelled") | |
| return assets | |
| default: | |
| } |
| @@ -67,15 +73,18 @@ func GetInstanceDetail(ctx context.Context, cancel context.CancelFunc, service s | |||
| bd := resp.Body | |||
| count += len(bd.Policys) | |||
| req.PageSize = tea.String(strconv.Itoa(size)) | |||
There was a problem hiding this comment.
This line is redundant. The req object is re-initialized at the beginning of each loop iteration on line 62. Modifying it here has no effect on subsequent iterations and can be safely removed for clarity.
1 participant
…etList
Thank you for your contribution to CloudRec!
What About:
java)go)opa)Description:
Explain the purpose of the PR.