Skip to content

feat(alicloud): aggregate cloudfw policies and assets via DescribeAss… #105

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.

Sign up for GitHub

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

Open
j3ttt wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(alicloud): aggregate cloudfw policies and assets via DescribeAss… #105

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd26381
feat(alicloud): aggregate cloudfw policies and assets via DescribeAss…
j3ttt Feb 26, 2026
a3783dc
fix(alicloud): compare cloudfw policy TotalCount as integer
j3ttt Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions collector/alicloud/collector/cloudfw/cloudfw_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ func GetCloudFWResource() schema.Resource {
func GetInstanceDetail(ctx context.Context, cancel context.CancelFunc, service schema.ServiceInterface, res chan<- any) error {

cli := service.(*collector.Services).Cloudfw

// Collect Internet boundary firewall assets by DescribeAssetList.
detail := &Detail{
Assets: describeAssetList(ctx, cli),
}

direction := []string{"in", "out"}
for _, d := range direction {
page := 1
Expand All @@ -67,15 +73,10 @@ func GetInstanceDetail(ctx context.Context, cancel context.CancelFunc, service s
bd := resp.Body
count += len(bd.Policys)
req.PageSize = tea.String(strconv.Itoa(size))
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

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

medium

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.

for i := 0; i < len(bd.Policys); i++ {
res <- Detail{
Policy: bd.Policys[i],
}
}
detail.Policies = append(detail.Policies, bd.Policys...)

if bd.TotalCount == nil || strconv.Itoa(count) >= *bd.TotalCount {
cancel()
return nil
break
}
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

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

high

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
                }


page += 1
Expand All @@ -85,9 +86,43 @@ func GetInstanceDetail(ctx context.Context, cancel context.CancelFunc, service s
}
}

res <- detail

return nil
}

type Detail struct {
Policy *cloudfw20171207.DescribeControlPolicyResponseBodyPolicys
Policies []*cloudfw20171207.DescribeControlPolicyResponseBodyPolicys
Assets []*cloudfw20171207.DescribeAssetListResponseBodyAssets
}

func describeAssetList(ctx context.Context, cli *cloudfw20171207.Client) (assets []*cloudfw20171207.DescribeAssetListResponseBodyAssets) {
page := 1
pageSize := 50
var collected int32 = 0

for {
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

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

high

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.

Suggested change
for {
for {
select {
case <-ctx.Done():
log.CtxLogger(ctx).Warn("describeAssetList context cancelled")
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
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

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

medium

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)

}
Comment on lines +112 to +135
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

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

high

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
		}
	}

}
Loading