Skip to content

feat: adds a DescribeSecurityGroups queryType - #12

Merged
jakubramut merged 3 commits into
mainfrom
feat/GLO-1699
Sep 25, 2026
Merged

jakubramut merged 3 commits into
mainfrom
feat/GLO-1699

Conversation

@jakubramut

Copy link
Copy Markdown
Contributor

Description of your changes

Adds a DescribeSecurityGroups queryType.

Why

DescribeSecurityGroupRules cannot filter by VPC:

$ aws ec2 describe-security-group-rules --filters Name=vpc-id,Values=vpc-0537adbd799764eec
An error occurred (InvalidParameterValue) ...: The filter 'vpc-id' is invalid

A rules query therefore needs a group-id that nothing produced from a VPC.
Tags do not help for the canonical case, a VPC's default group: AWS creates it untagged and unreferenced.

What

  • describeSecurityGroups - paginated describe behind the shared ec2Client guard (region and filters required). Projects groupId, groupName, securityGroupArn, description, vpcId, ownerId, tags{}. IpPermissions/IpPermissionsEgress are dropped on purpose.
  • example XRD - pre-existing bug: Crossplane drops x-kubernetes-preserve-unknown-fields from the XRD status object, so every target: status.<field> in example/ fails apply with field not declared in schema. All 11 status targets and 3 *Ref spec fields now declared.

Additive: a new enum value plus docs, no existing queryType changes behaviour.

Tests

on the CI-pinned go1.25.10:
go test => ok 0.836s
golangci-lint@v2.8.0 => 0 issues.

Verified live on a throwaway VPC with 56 groups, through the built package on kind running crossplane:v2.4.1-up.1: all 56 cross-checked field by field against aws ec2 describe-security-groups with zero mismatches.
Chaining groupId into DescribeSecurityGroupRules returned 6 rules with securityGroupRuleId populated.
The empty-filter, bad-filter-name and missing-ec2:DescribeSecurityGroups guards each surface on the XR.

c9f954e A/B'd on Crossplane v1.20.13 and v2.4.1-up.1: both drop the flag and fail identically before, both land all 11 status fields after

jakubramut and others added 3 commits September 23, 2026 14:45
…y types

Adds TestEc2APIError (403 UnauthorizedOperation surfaces wrapped with the
operation name and AWS error code) and table-drives TestEc2RegionGuard over
every EC2 describe, including DescribeSecurityGroups. respStub gains an
optional HTTP status.

@ytsarev ytsarev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - additive, no behaviour change to existing query types.

Verified on the CI-pinned go1.25.10:

  • go vet, go test ./... and golangci-lint v2.8.0 clean (0 issues)
  • go generate ./... produces no diff, so the input CRD is in sync with input.go
  • the describeSecurityGroups handler follows the shared ec2Client guard / paginator pattern; the projection test pins that inline ipPermissions stay dropped

Pushed 8b9353d to close test gaps:

  • TestEc2APIError: a 403 UnauthorizedOperation surfaces as <Op> failed ... UnauthorizedOperation for all four EC2 describes (the previously uncovered error branch)
  • TestEc2RegionGuard now table-driven over every EC2 describe and asserts the actual requires a region message, since SGsNoRegion in the guards table only asserted err != nil
  • a shared ec2QueryTypes list, so the next EC2 describe picks up the region, filter and error tests from one place

The four EC2 handlers other than subnets are now at 100% coverage.

Nits, non-blocking:

  • The docs say DescribeSecurityGroupRules errors on vpc-id only where the region holds rules and otherwise returns []. That is observed AWS behaviour, not documented behaviour, so it could change without notice.
  • The example XRD no longer uses preserve-unknown-fields on status, so anyone who copies it must declare each new target. The inline comment covers this.

@jakubramut
jakubramut merged commit 2494446 into main Sep 25, 2026
6 checks passed
@jakubramut
jakubramut deleted the feat/GLO-1699 branch September 25, 2026 13:45
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.

3 participants