From a8fc6555713fc008284b13e25cfecccb5c29434d Mon Sep 17 00:00:00 2001 From: Jakub Ramut Date: Fri, 4 Sep 2026 12:44:42 +0200 Subject: [PATCH 1/2] feat: add a DescribeEc2 query type for direct EC2 describes --- README.md | 17 +- aws.go | 173 ++++++++++++ aws_test.go | 248 +++++++++++++++++- example/README.md | 1 + example/composition-ec2-route-tables.yaml | 33 +++ input/v1beta1/input.go | 10 +- package/crossplane.yaml | 3 +- .../input/aws.fn.crossplane.io_inputs.yaml | 9 +- 8 files changed, 486 insertions(+), 8 deletions(-) create mode 100644 example/composition-ec2-route-tables.yaml diff --git a/README.md b/README.md index 8cd9428..47ab2ff 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,7 @@ result is written to `target` (`status.` or `context.`). | `DescribeRegions` | `[{name, endpoint, optInStatus}]` | | `DescribeAvailabilityZones` | `[{name, zoneId, state, regionName, zoneType, groupName}]` | | `DescribeImages` | `[{imageId, name, ownerId, creationDate, architecture, state, rootDeviceType, description}]` | +| `DescribeEc2` | Read-only EC2 `Describe*` selected by `parameters.operation` (case-sensitive), for identifiers Cloud Control does not model or where the Tagging API is not authoritative. `RouteTables` -> `[{routeTableId, vpcId, ownerId, associations[{routeTableAssociationId, routeTableId, subnetId, gatewayId, main, state}], routes[{destinationCidrBlock, destinationIpv6CidrBlock, destinationPrefixListId, gatewayId, natGatewayId, transitGatewayId, vpcPeeringConnectionId, egressOnlyInternetGatewayId, carrierGatewayId, localGatewayId, coreNetworkArn, instanceId, networkInterfaceId, origin, state}], tags{}}]`. `Subnets` -> `[{subnetId, subnetArn, vpcId, ownerId, availabilityZone, availabilityZoneId, cidrBlock, state, defaultForAz, mapPublicIpOnLaunch, availableIpAddressCount, tags{}}]`. `SecurityGroupRules` -> `[{securityGroupRuleId, securityGroupRuleArn, groupId, groupOwnerId, isEgress, ipProtocol, fromPort, toPort, cidrIpv4, cidrIpv6, prefixListId, referencedGroupId, description, tags{}}]`. Server-side `filters` (native EC2 names). | | `ListServiceQuotas` | `[{quotaCode, quotaName, value, unit, adjustable, globalQuota}]` (all quotas for a `serviceCode`) | | `GetServiceQuota` | `{quotaCode, quotaName, value, unit, adjustable, globalQuota}` (a single quota; needs `serviceCode`+`quotaCode`) | @@ -43,9 +44,22 @@ result is written to `target` (`status.` or `context.`). type including untagged ones. Caveats: needs that type's read IAM permissions, filters are applied client-side, and hydration costs one `GetResource` call per resource (use `hydrate=false` to skip it when you only want identifiers). +- **`DescribeEc2`** - when the identifier you need is not in the resource's + CloudFormation schema (a route table's **main association ID** is the + canonical case), or when you need an **authoritative** answer. It reads only + what the `filters` select, server-side, so a foreign resource cannot fail the + query. Always filter (`vpc-id`, `group-id`): unfiltered, these are region-wide + reads. Filter *values* are not validated - an id that does not exist yields + an empty result, not an error - but an unrecognised filter *name* is fatal. Both alternatives can mislead here: + Cloud Control's `ListResources` walks every resource of the type account-wide + and aborts the whole composition if any one of them fails to hydrate, and the + Tagging API can keep reporting deleted resources for a while - which, if they + carry the same identifying tag as their live replacements, silently doubles + the result set. Rule of thumb: *IDs by tag →* `GetResources`; *attributes / full inventory of a -type →* `ListResources`. +type →* `ListResources`; *EC2 identifiers CloudFormation does not model, or an +authoritative VPC-scoped read →* `DescribeEc2`. ## Input reference @@ -59,6 +73,7 @@ filters: # or filtersRef: (status./context values: ["prod"] parameters: # or parametersRef: ; scalar args per queryType: # allRegions, allAvailabilityZones (bool); owners, imageIds (csv) [EC2] + # operation: RouteTables | SecurityGroupRules | Subnets (DescribeEc2) [EC2] # serviceCode, quotaCode [Service Quotas] # typeName, resourceModel, roleArn, hydrate (default true) [Cloud Control] # resourceTypeFilters (csv) [Tagging API] diff --git a/aws.go b/aws.go index f5c88c9..640f1d3 100644 --- a/aws.go +++ b/aws.go @@ -57,6 +57,7 @@ func (q *AWSQuery) registry() map[string]handler { "DescribeRegions": q.describeRegions, "DescribeAvailabilityZones": q.describeAvailabilityZones, "DescribeImages": q.describeImages, + "DescribeEc2": q.describeEc2, "ListServiceQuotas": q.listServiceQuotas, "GetServiceQuota": q.getServiceQuota, "ListResources": q.listResources, @@ -308,6 +309,162 @@ func (q *AWSQuery) describeImages(ctx context.Context, cfg aws.Config, in *v1bet return res, nil } +// describeEc2 runs one read-only EC2 Describe*, selected by +// parameters.operation. Filters are server-side, so unlike ListResources it +// never enumerates the account. +func (q *AWSQuery) describeEc2(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, error) { + if cfg.Region == "" { + return nil, errRegionRequired("DescribeEc2") + } + client := ec2.NewFromConfig(cfg) + switch op := in.Parameters["operation"]; op { + case "RouteTables": + return describeRouteTables(ctx, client, in) + case "SecurityGroupRules": + return describeSecurityGroupRules(ctx, client, in) + case "Subnets": + return describeSubnets(ctx, client, in) + default: + return nil, errors.Errorf("DescribeEc2 requires parameters.operation to be one of RouteTables, SecurityGroupRules, Subnets (got %q)", op) + } +} + +// describeRouteTables lists route tables with their associations (EC2, +// paginated). The main association ID is not in the CloudFormation schema. +func describeRouteTables(ctx context.Context, client *ec2.Client, in *v1beta1.Input) (any, error) { + p := ec2.NewDescribeRouteTablesPaginator(client, &ec2.DescribeRouteTablesInput{Filters: toEC2Filters(in.Filters)}) + res := []any{} + for p.HasMorePages() { + page, err := p.NextPage(ctx) + if err != nil { + return nil, errors.Wrap(err, "DescribeRouteTables failed") + } + for _, rt := range page.RouteTables { + res = append(res, map[string]any{ + "routeTableId": aws.ToString(rt.RouteTableId), + "vpcId": aws.ToString(rt.VpcId), + "ownerId": aws.ToString(rt.OwnerId), + "associations": routeTableAssociations(rt.Associations), + "routes": routeTableRoutes(rt.Routes), + "tags": ec2TagsToMap(rt.Tags), + }) + } + } + return res, nil +} + +func routeTableAssociations(associations []ec2types.RouteTableAssociation) []any { + out := make([]any, 0, len(associations)) + for _, a := range associations { + state := "" + if a.AssociationState != nil { + state = string(a.AssociationState.State) + } + out = append(out, map[string]any{ + "routeTableAssociationId": aws.ToString(a.RouteTableAssociationId), + "routeTableId": aws.ToString(a.RouteTableId), + "subnetId": aws.ToString(a.SubnetId), + "gatewayId": aws.ToString(a.GatewayId), + "main": aws.ToBool(a.Main), + "state": state, + }) + } + return out +} + +func routeTableRoutes(routes []ec2types.Route) []any { + out := make([]any, 0, len(routes)) + for _, r := range routes { + out = append(out, map[string]any{ + // All three destination forms: aws_route's external name is + // {route_table_id}_{destination}, so omitting any of them makes that + // route unidentifiable. + "destinationCidrBlock": aws.ToString(r.DestinationCidrBlock), + "destinationIpv6CidrBlock": aws.ToString(r.DestinationIpv6CidrBlock), + "destinationPrefixListId": aws.ToString(r.DestinationPrefixListId), + "carrierGatewayId": aws.ToString(r.CarrierGatewayId), + "coreNetworkArn": aws.ToString(r.CoreNetworkArn), + "egressOnlyInternetGatewayId": aws.ToString(r.EgressOnlyInternetGatewayId), + "gatewayId": aws.ToString(r.GatewayId), + "instanceId": aws.ToString(r.InstanceId), + "localGatewayId": aws.ToString(r.LocalGatewayId), + "natGatewayId": aws.ToString(r.NatGatewayId), + "networkInterfaceId": aws.ToString(r.NetworkInterfaceId), + "transitGatewayId": aws.ToString(r.TransitGatewayId), + "vpcPeeringConnectionId": aws.ToString(r.VpcPeeringConnectionId), + "origin": string(r.Origin), + "state": string(r.State), + }) + } + return out +} + +// describeSecurityGroupRules lists security group rules (EC2, paginated). +// Scope with a group-id filter. +func describeSecurityGroupRules(ctx context.Context, client *ec2.Client, in *v1beta1.Input) (any, error) { + p := ec2.NewDescribeSecurityGroupRulesPaginator(client, &ec2.DescribeSecurityGroupRulesInput{Filters: toEC2Filters(in.Filters)}) + res := []any{} + for p.HasMorePages() { + page, err := p.NextPage(ctx) + if err != nil { + return nil, errors.Wrap(err, "DescribeSecurityGroupRules failed") + } + for _, r := range page.SecurityGroupRules { + m := map[string]any{ + "securityGroupRuleId": aws.ToString(r.SecurityGroupRuleId), + "securityGroupRuleArn": aws.ToString(r.SecurityGroupRuleArn), + "groupId": aws.ToString(r.GroupId), + "groupOwnerId": aws.ToString(r.GroupOwnerId), + "isEgress": aws.ToBool(r.IsEgress), + "ipProtocol": aws.ToString(r.IpProtocol), + "cidrIpv4": aws.ToString(r.CidrIpv4), + "cidrIpv6": aws.ToString(r.CidrIpv6), + "prefixListId": aws.ToString(r.PrefixListId), + "description": aws.ToString(r.Description), + "tags": ec2TagsToMap(r.Tags), + } + if r.ReferencedGroupInfo != nil { + m["referencedGroupId"] = aws.ToString(r.ReferencedGroupInfo.GroupId) + } + putInt32(m, "fromPort", r.FromPort) + putInt32(m, "toPort", r.ToPort) + res = append(res, m) + } + } + return res, nil +} + +// describeSubnets lists subnets (EC2, paginated). Returns only live subnets, +// unlike the Tagging API. +func describeSubnets(ctx context.Context, client *ec2.Client, in *v1beta1.Input) (any, error) { + p := ec2.NewDescribeSubnetsPaginator(client, &ec2.DescribeSubnetsInput{Filters: toEC2Filters(in.Filters)}) + res := []any{} + for p.HasMorePages() { + page, err := p.NextPage(ctx) + if err != nil { + return nil, errors.Wrap(err, "DescribeSubnets failed") + } + for _, s := range page.Subnets { + m := map[string]any{ + "subnetId": aws.ToString(s.SubnetId), + "subnetArn": aws.ToString(s.SubnetArn), + "vpcId": aws.ToString(s.VpcId), + "ownerId": aws.ToString(s.OwnerId), + "availabilityZone": aws.ToString(s.AvailabilityZone), + "availabilityZoneId": aws.ToString(s.AvailabilityZoneId), + "cidrBlock": aws.ToString(s.CidrBlock), + "state": string(s.State), + "defaultForAz": aws.ToBool(s.DefaultForAz), + "mapPublicIpOnLaunch": aws.ToBool(s.MapPublicIpOnLaunch), + "tags": ec2TagsToMap(s.Tags), + } + putInt32(m, "availableIpAddressCount", s.AvailableIpAddressCount) + res = append(res, m) + } + } + return res, nil +} + // listServiceQuotas lists quotas for a service (ServiceQuotas, paginated). func (q *AWSQuery) listServiceQuotas(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, error) { if cfg.Region == "" { @@ -542,6 +699,22 @@ func toEC2Filters(filters []v1beta1.Filter) []ec2types.Filter { return out } +// ec2TagsToMap flattens EC2 tags, matching the shape getResources returns. +func ec2TagsToMap(tags []ec2types.Tag) map[string]any { + out := map[string]any{} + for _, t := range tags { + out[aws.ToString(t.Key)] = aws.ToString(t.Value) + } + return out +} + +// putInt32 sets key only when v is set, so an absent value is not a real zero. +func putInt32(m map[string]any, key string, v *int32) { + if v != nil { + m[key] = int64(*v) + } +} + func toTagFilters(filters []v1beta1.Filter) []rgttypes.TagFilter { if len(filters) == 0 { return nil diff --git a/aws_test.go b/aws_test.go index 70baff8..78cb2e8 100644 --- a/aws_test.go +++ b/aws_test.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/credentials" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" sqtypes "github.com/aws/aws-sdk-go-v2/service/servicequotas/types" "github.com/google/go-cmp/cmp" "github.com/upbound/function-aws-query/input/v1beta1" @@ -28,9 +29,19 @@ type respStub struct { bodies []string contentType string calls int + // requests records each marshalled request body, so a test can assert what + // was actually sent (e.g. that a filter went server-side). + requests []string } -func (s *respStub) Do(_ *http.Request) (*http.Response, error) { +func (s *respStub) Do(req *http.Request) (*http.Response, error) { + if req.Body != nil { + sent, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + s.requests = append(s.requests, string(sent)) + } body := s.bodies[s.calls] if s.calls < len(s.bodies)-1 { s.calls++ @@ -325,6 +336,11 @@ func TestHandlerValidationGuards(t *testing.T) { "QuotasNoService": func() (any, error) { return q.listServiceQuotas(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, "GetQuotaNoCodes": func() (any, error) { return q.getServiceQuota(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, "ListResNoTypeName": func() (any, error) { return q.listResources(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, + "Ec2NoRegion": func() (any, error) { return q.describeEc2(ctx, noRegion, &v1beta1.Input{}) }, + "Ec2NoOperation": func() (any, error) { return q.describeEc2(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, + "Ec2BadOperation": func() (any, error) { + return q.describeEc2(ctx, stubCfg(&respStub{}), &v1beta1.Input{Parameters: map[string]string{"operation": "Vpcs"}}) + }, } for name, call := range cases { t.Run(name, func(t *testing.T) { @@ -473,6 +489,236 @@ func TestDescribeImages(t *testing.T) { } } +// --- DescribeEc2 ------------------------------------------------------------ + +const ( + routeTablesPage1 = `r` + + `rtb-1vpc-1123456789012` + + `rtbassoc-mainrtb-1` + + `
true
associated` + + `10.0.0.0/16local` + + `CreateRouteTableactive` + + `Namemain` + + `tok` + + routeTablesPage2 = `r` + + `rtb-2vpc-1123456789012` + + `rtbassoc-2rtb-2` + + `subnet-1
false
associated` + + `` + + subnetsBody = `r` + + `subnet-1` + + `arn:aws:ec2:eu-central-1:123456789012:subnet/subnet-1` + + `vpc-1123456789012eu-central-1a` + + `euc1-az210.0.1.0/24available` + + `falsetrue` + + `250` + + `Namepublic-a` + + securityGroupRulesBody = `r` + + `sgr-1sg-1` + + `arn:aws:ec2:eu-central-1:123456789012:security-group-rule/sgr-1` + + `123456789012falsetcp` + + `4434430.0.0.0/0https` + + `Nameingress` + + `sgr-2sg-1123456789012` + + `true-1-1-1` + + `sg-2` + + `sgr-3sg-1123456789012` + + `falseicmppl-1` + + `` +) + +func ec2Input(operation string) *v1beta1.Input { + return &v1beta1.Input{ + Parameters: map[string]string{"operation": operation}, + Filters: []v1beta1.Filter{{Name: "vpc-id", Values: []string{"vpc-1"}}}, + } +} + +// TestDescribeEc2Dispatches proves every allow-listed operation reaches its own +// describe call, and that an unsupported one names the supported values. +func TestDescribeEc2Dispatches(t *testing.T) { + if newQuery().registry()["DescribeEc2"] == nil { + t.Fatal("DescribeEc2 is not wired into the handler registry") + } + + cases := map[string]struct { + operation string + body string + }{ + "RouteTables": {operation: "RouteTables", body: routeTablesPage2}, + "SecurityGroupRules": {operation: "SecurityGroupRules", body: securityGroupRulesBody}, + "Subnets": {operation: "Subnets", body: subnetsBody}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, err := newQuery().describeEc2(context.Background(), + stubCfg(&respStub{bodies: []string{tc.body}, contentType: "text/xml"}), ec2Input(tc.operation)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + list, ok := got.([]any) + if !ok || len(list) == 0 { + t.Fatalf("expected a non-empty list, got %#v", got) + } + }) + } + + t.Run("Unsupported", func(t *testing.T) { + _, err := newQuery().describeEc2(context.Background(), stubCfg(&respStub{}), ec2Input("Vpcs")) + if err == nil { + t.Fatal("expected an error for an unsupported operation") + } + for _, want := range []string{"RouteTables", "SecurityGroupRules", "Subnets"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error should name %s: %v", want, err) + } + } + }) +} + +// TestDescribeEc2RouteTablesPaginates covers the projection (associations, incl. +// the main association ID) across two pages. +func TestDescribeEc2RouteTablesPaginates(t *testing.T) { + stub := &respStub{bodies: []string{routeTablesPage1, routeTablesPage2}, contentType: "text/xml"} + got, err := newQuery().describeEc2(context.Background(), stubCfg(stub), ec2Input("RouteTables")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []any{ + map[string]any{ + "routeTableId": "rtb-1", "vpcId": "vpc-1", "ownerId": "123456789012", + "associations": []any{map[string]any{ + "routeTableAssociationId": "rtbassoc-main", "routeTableId": "rtb-1", + "subnetId": "", "gatewayId": "", "main": true, "state": "associated", + }}, + "routes": []any{map[string]any{ + "destinationCidrBlock": "10.0.0.0/16", "destinationIpv6CidrBlock": "", + "destinationPrefixListId": "", "carrierGatewayId": "", "coreNetworkArn": "", + "egressOnlyInternetGatewayId": "", "gatewayId": "local", "instanceId": "", + "localGatewayId": "", "natGatewayId": "", "networkInterfaceId": "", + "transitGatewayId": "", "vpcPeeringConnectionId": "", + "origin": "CreateRouteTable", "state": "active", + }}, + "tags": map[string]any{"Name": "main"}, + }, + map[string]any{ + "routeTableId": "rtb-2", "vpcId": "vpc-1", "ownerId": "123456789012", + "associations": []any{map[string]any{ + "routeTableAssociationId": "rtbassoc-2", "routeTableId": "rtb-2", + "subnetId": "subnet-1", "gatewayId": "", "main": false, "state": "associated", + }}, + "routes": []any{}, + "tags": map[string]any{}, + }, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("-want +got:\n%s", diff) + } + if len(stub.requests) != 2 { + t.Fatalf("expected 2 requests (one per page), got %d", len(stub.requests)) + } + // vpc-id must go server-side - that is the whole point over ListResources. + if !strings.Contains(stub.requests[0], "Filter.1.Name=vpc-id") { + t.Errorf("vpc-id filter not sent server-side: %s", stub.requests[0]) + } +} + +// Isolates the region guard: the shared guards table only asserts err != nil, +// which the SDK's endpoint-resolution error satisfies on its own. +func TestDescribeEc2RegionGuard(t *testing.T) { + in := &v1beta1.Input{Parameters: map[string]string{"operation": "Subnets"}} + _, err := newQuery().describeEc2(context.Background(), aws.Config{}, in) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "requires a region") { + t.Errorf("expected the region guard, got: %v", err) + } +} + +func TestDescribeEc2Subnets(t *testing.T) { + stub := &respStub{bodies: []string{subnetsBody}, contentType: "text/xml"} + got, err := newQuery().describeEc2(context.Background(), stubCfg(stub), ec2Input("Subnets")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []any{map[string]any{ + "subnetId": "subnet-1", "subnetArn": "arn:aws:ec2:eu-central-1:123456789012:subnet/subnet-1", + "vpcId": "vpc-1", "ownerId": "123456789012", "availabilityZone": "eu-central-1a", + "availabilityZoneId": "euc1-az2", "cidrBlock": "10.0.1.0/24", "state": "available", + "defaultForAz": false, "mapPublicIpOnLaunch": true, "availableIpAddressCount": int64(250), + "tags": map[string]any{"Name": "public-a"}, + }} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("-want +got:\n%s", diff) + } + if !strings.Contains(stub.requests[0], "Filter.1.Name=vpc-id") { + t.Errorf("filter not sent server-side: %s", stub.requests[0]) + } +} + +// Optional keys: referencedGroupId only for group references, ports only when +// on the wire - a live all-protocol rule reports -1/-1, not nothing. +func TestDescribeEc2SecurityGroupRules(t *testing.T) { + stub := &respStub{bodies: []string{securityGroupRulesBody}, contentType: "text/xml"} + got, err := newQuery().describeEc2(context.Background(), stubCfg(stub), ec2Input("SecurityGroupRules")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []any{ + map[string]any{ + "securityGroupRuleId": "sgr-1", + "securityGroupRuleArn": "arn:aws:ec2:eu-central-1:123456789012:security-group-rule/sgr-1", + "groupId": "sg-1", "groupOwnerId": "123456789012", + "isEgress": false, "ipProtocol": "tcp", "fromPort": int64(443), "toPort": int64(443), + "cidrIpv4": "0.0.0.0/0", "cidrIpv6": "", "prefixListId": "", "description": "https", + "tags": map[string]any{"Name": "ingress"}, + }, + map[string]any{ + "securityGroupRuleId": "sgr-2", "securityGroupRuleArn": "", + "groupId": "sg-1", "groupOwnerId": "123456789012", + "isEgress": true, "ipProtocol": "-1", "fromPort": int64(-1), "toPort": int64(-1), + "cidrIpv4": "", "cidrIpv6": "", + "prefixListId": "", "description": "", "referencedGroupId": "sg-2", + "tags": map[string]any{}, + }, + map[string]any{ + "securityGroupRuleId": "sgr-3", "securityGroupRuleArn": "", + "groupId": "sg-1", "groupOwnerId": "123456789012", + "isEgress": false, "ipProtocol": "icmp", "cidrIpv4": "", "cidrIpv6": "", + "prefixListId": "pl-1", "description": "", "tags": map[string]any{}, + }, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("-want +got:\n%s", diff) + } + if !strings.Contains(stub.requests[0], "Filter.1.Name=vpc-id") { + t.Errorf("filter not sent server-side: %s", stub.requests[0]) + } +} + +func TestEc2TagsToMap(t *testing.T) { + got := ec2TagsToMap([]ec2types.Tag{{Key: aws.String("Name"), Value: aws.String("x")}}) + if diff := cmp.Diff(map[string]any{"Name": "x"}, got); diff != "" { + t.Errorf("-want +got:\n%s", diff) + } + if diff := cmp.Diff(map[string]any{}, ec2TagsToMap(nil)); diff != "" { + t.Errorf("ec2TagsToMap(nil) should be an empty map:\n%s", diff) + } +} + +func TestPutInt32(t *testing.T) { + m := map[string]any{} + putInt32(m, "set", aws.Int32(7)) + putInt32(m, "unset", nil) + if diff := cmp.Diff(map[string]any{"set": int64(7)}, m); diff != "" { + t.Errorf("-want +got:\n%s", diff) + } +} + // --- dispatch + remaining skip paths ---------------------------------------- func TestAWSQueryUnsupportedType(t *testing.T) { diff --git a/example/README.md b/example/README.md index 0492435..fdac288 100644 --- a/example/README.md +++ b/example/README.md @@ -47,6 +47,7 @@ status: | `composition-caller-identity.yaml` | GetCallerIdentity | STS | `status.callerIdentity` | | `composition-availability-zones.yaml` | DescribeAvailabilityZones | EC2 | `status.availabilityZones` | | `composition-ami-lookup.yaml` | DescribeImages | EC2 | `status.amis` | +| `composition-ec2-route-tables.yaml` | DescribeEc2 (`operation=RouteTables`) | EC2 | `status.routeTables` | | `composition-service-quotas.yaml` | ListServiceQuotas | Service Quotas | `status.ec2Quotas` | | `composition-cloudcontrol-vpcs.yaml` | ListResources | Cloud Control | `status.prodVpcs` | | `composition-cloudcontrol-ids.yaml` | ListResources (`hydrate=false`) | Cloud Control | `status.vpcIds` | diff --git a/example/composition-ec2-route-tables.yaml b/example/composition-ec2-route-tables.yaml new file mode 100644 index 0000000..45d9b71 --- /dev/null +++ b/example/composition-ec2-route-tables.yaml @@ -0,0 +1,33 @@ +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: function-aws-query-ec2-route-tables +spec: + compositeTypeRef: + apiVersion: example.io/v1alpha1 + kind: XAccount + mode: Pipeline + pipeline: + # Route tables for one VPC via EC2, with each table's associations. The main + # association ID is not in the AWS::EC2::RouteTable schema, so ListResources + # cannot substitute. The vpc-id filter is server-side. + - step: route-tables-by-vpc + functionRef: + name: function-aws-query + input: + apiVersion: aws.fn.crossplane.io/v1beta1 + kind: Input + queryType: DescribeEc2 + region: eu-central-1 + parameters: + operation: RouteTables + filters: + - name: vpc-id + values: ["vpc-0123456789abcdef0"] + target: status.routeTables + credentials: + - name: aws-creds + source: Secret + secretRef: + namespace: crossplane-system + name: aws-creds diff --git a/input/v1beta1/input.go b/input/v1beta1/input.go index 90a53d4..bc67da6 100644 --- a/input/v1beta1/input.go +++ b/input/v1beta1/input.go @@ -21,7 +21,7 @@ type Input struct { metav1.ObjectMeta `json:"metadata,omitempty"` // QueryType selects the AWS read operation to perform. - // +kubebuilder:validation:Enum=GetCallerIdentity;DescribeRegions;DescribeAvailabilityZones;DescribeImages;ListServiceQuotas;GetServiceQuota;ListResources;GetResources + // +kubebuilder:validation:Enum=GetCallerIdentity;DescribeRegions;DescribeAvailabilityZones;DescribeImages;DescribeEc2;ListServiceQuotas;GetServiceQuota;ListResources;GetResources QueryType string `json:"queryType"` // Region to target. Optional for global-ish calls (GetCallerIdentity, @@ -35,8 +35,9 @@ type Input struct { RegionRef *string `json:"regionRef,omitempty"` // Filters are name/values pairs. Their interpretation depends on QueryType: - // - EC2 ops (DescribeRegions, DescribeAvailabilityZones, DescribeImages): - // EC2 filter names such as "tag:Name", "state", "architecture". + // - EC2 ops (DescribeRegions, DescribeAvailabilityZones, DescribeImages, + // DescribeEc2): EC2 filter names such as "tag:Name", "state", + // "architecture", "vpc-id", "group-id" (server-side). // - GetResources (Tagging API): each entry is a tag filter where name is // the tag key and values are the tag values (server-side). // - ListResources (Cloud Control): client-side property match where name @@ -52,6 +53,9 @@ type Input struct { // Parameters carries scalar/string-list args specific to each QueryType: // allRegions, allAvailabilityZones (bool); owners, imageIds (csv) [EC2] + // operation: which EC2 describe to run - RouteTables (incl. their + // associations, i.e. the main association ID), SecurityGroupRules, + // Subnets. Required by DescribeEc2; bound by "filters" [EC2] // serviceCode, quotaCode [ServiceQuotas] // typeName (e.g. AWS::EC2::VPC), resourceModel (json), roleArn, // hydrate (bool, default true: GetResource each item for full props) [Cloud Control] diff --git a/package/crossplane.yaml b/package/crossplane.yaml index be52a5a..e9ab39a 100644 --- a/package/crossplane.yaml +++ b/package/crossplane.yaml @@ -14,7 +14,8 @@ metadata: (go-templating, patch-and-transform, etc.). It supports service-metadata queries (caller identity, regions, availability zones, AMI lookups, service quotas) and generic existing-resource discovery (AWS Cloud Control - ListResources and the Resource Groups Tagging API GetResources). + ListResources, the Resource Groups Tagging API GetResources, and direct + EC2 describes via DescribeEc2). Authentication mirrors the Official AWS Provider: the credentials secret is a shared-credentials INI compatible with provider-upjet-aws, and the diff --git a/package/input/aws.fn.crossplane.io_inputs.yaml b/package/input/aws.fn.crossplane.io_inputs.yaml index 5b03950..ccf5387 100644 --- a/package/input/aws.fn.crossplane.io_inputs.yaml +++ b/package/input/aws.fn.crossplane.io_inputs.yaml @@ -33,8 +33,9 @@ spec: filters: description: |- Filters are name/values pairs. Their interpretation depends on QueryType: - - EC2 ops (DescribeRegions, DescribeAvailabilityZones, DescribeImages): - EC2 filter names such as "tag:Name", "state", "architecture". + - EC2 ops (DescribeRegions, DescribeAvailabilityZones, DescribeImages, + DescribeEc2): EC2 filter names such as "tag:Name", "state", + "architecture", "vpc-id", "group-id" (server-side). - GetResources (Tagging API): each entry is a tag filter where name is the tag key and values are the tag values (server-side). - ListResources (Cloud Control): client-side property match where name @@ -163,6 +164,9 @@ spec: description: |- Parameters carries scalar/string-list args specific to each QueryType: allRegions, allAvailabilityZones (bool); owners, imageIds (csv) [EC2] + operation: which EC2 describe to run - RouteTables (incl. their + associations, i.e. the main association ID), SecurityGroupRules, + Subnets. Required by DescribeEc2; bound by "filters" [EC2] serviceCode, quotaCode [ServiceQuotas] typeName (e.g. AWS::EC2::VPC), resourceModel (json), roleArn, hydrate (bool, default true: GetResource each item for full props) [Cloud Control] @@ -186,6 +190,7 @@ spec: - DescribeRegions - DescribeAvailabilityZones - DescribeImages + - DescribeEc2 - ListServiceQuotas - GetServiceQuota - ListResources From c658ab1944c095965570cd283994568c1020d253 Mon Sep 17 00:00:00 2001 From: Jakub Ramut Date: Mon, 7 Sep 2026 14:40:18 +0200 Subject: [PATCH 2/2] fix: address review - enum-validated query types, must-filter guard, IPv6 projection --- README.md | 28 +++- aws.go | 96 +++++++++---- aws_test.go | 131 +++++++++++------- example/Makefile | 3 + example/README.md | 4 +- example/composition-ec2-route-tables.yaml | 4 +- .../composition-ec2-security-group-rules.yaml | 35 +++++ example/composition-ec2-subnets.yaml | 31 +++++ input/v1beta1/input.go | 22 +-- package/crossplane.yaml | 2 +- .../input/aws.fn.crossplane.io_inputs.yaml | 24 ++-- 11 files changed, 271 insertions(+), 109 deletions(-) create mode 100644 example/composition-ec2-security-group-rules.yaml create mode 100644 example/composition-ec2-subnets.yaml diff --git a/README.md b/README.md index 47ab2ff..b2b886b 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,9 @@ result is written to `target` (`status.` or `context.`). | `DescribeRegions` | `[{name, endpoint, optInStatus}]` | | `DescribeAvailabilityZones` | `[{name, zoneId, state, regionName, zoneType, groupName}]` | | `DescribeImages` | `[{imageId, name, ownerId, creationDate, architecture, state, rootDeviceType, description}]` | -| `DescribeEc2` | Read-only EC2 `Describe*` selected by `parameters.operation` (case-sensitive), for identifiers Cloud Control does not model or where the Tagging API is not authoritative. `RouteTables` -> `[{routeTableId, vpcId, ownerId, associations[{routeTableAssociationId, routeTableId, subnetId, gatewayId, main, state}], routes[{destinationCidrBlock, destinationIpv6CidrBlock, destinationPrefixListId, gatewayId, natGatewayId, transitGatewayId, vpcPeeringConnectionId, egressOnlyInternetGatewayId, carrierGatewayId, localGatewayId, coreNetworkArn, instanceId, networkInterfaceId, origin, state}], tags{}}]`. `Subnets` -> `[{subnetId, subnetArn, vpcId, ownerId, availabilityZone, availabilityZoneId, cidrBlock, state, defaultForAz, mapPublicIpOnLaunch, availableIpAddressCount, tags{}}]`. `SecurityGroupRules` -> `[{securityGroupRuleId, securityGroupRuleArn, groupId, groupOwnerId, isEgress, ipProtocol, fromPort, toPort, cidrIpv4, cidrIpv6, prefixListId, referencedGroupId, description, tags{}}]`. Server-side `filters` (native EC2 names). | +| `DescribeRouteTables` | `[{routeTableId, vpcId, ownerId, associations[{routeTableAssociationId, routeTableId, subnetId, gatewayId, main, state}], routes[{destinationCidrBlock, destinationIpv6CidrBlock, destinationPrefixListId, gatewayId, natGatewayId, transitGatewayId, vpcPeeringConnectionId, egressOnlyInternetGatewayId, carrierGatewayId, localGatewayId, coreNetworkArn, instanceId, networkInterfaceId, origin, state}], tags{}}]`. Carries the **main association ID**, which no CloudFormation schema models. `filters` **required**: `vpc-id`, `route-table-id`, `association.subnet-id`, `tag:`. | +| `DescribeSubnets` | `[{subnetId, subnetArn, vpcId, ownerId, availabilityZone, availabilityZoneId, cidrBlock, state, defaultForAz, mapPublicIpOnLaunch, availableIpAddressCount, ipv6Native, ipv6CidrBlockAssociationSet[{associationId, ipv6CidrBlock, state}], tags{}}]`. Returns only **live** subnets, unlike the Tagging API. `filters` **required**: `vpc-id`, `subnet-id`, `availability-zone`, `tag:`. | +| `DescribeSecurityGroupRules` | `[{securityGroupRuleId, securityGroupRuleArn, groupId, groupOwnerId, isEgress, ipProtocol, fromPort, toPort, cidrIpv4, cidrIpv6, prefixListId, referencedGroupId, referencedGroupUserId, referencedGroupVpcId, description, tags{}}]`. `filters` **required**: `group-id`, `security-group-rule-id`, `tag:` - this operation does **not** accept `vpc-id`. | | `ListServiceQuotas` | `[{quotaCode, quotaName, value, unit, adjustable, globalQuota}]` (all quotas for a `serviceCode`) | | `GetServiceQuota` | `{quotaCode, quotaName, value, unit, adjustable, globalQuota}` (a single quota; needs `serviceCode`+`quotaCode`) | @@ -44,10 +46,13 @@ result is written to `target` (`status.` or `context.`). type including untagged ones. Caveats: needs that type's read IAM permissions, filters are applied client-side, and hydration costs one `GetResource` call per resource (use `hydrate=false` to skip it when you only want identifiers). -- **`DescribeEc2`** - when the identifier you need is not in the resource's - CloudFormation schema (a route table's **main association ID** is the - canonical case), or when you need an **authoritative** answer. It reads only - what the `filters` select, server-side, so a foreign resource cannot fail the +- **`DescribeRouteTables` / `DescribeSubnets` / `DescribeSecurityGroupRules`** - + when the identifier you need is not in the resource's CloudFormation schema (a + route table's **main association ID** is the canonical case), or when you need + an **authoritative** answer. Needs `ec2:DescribeRouteTables`, + `ec2:DescribeSubnets` and `ec2:DescribeSecurityGroupRules` respectively; + without them the call fails at reconcile with `UnauthorizedOperation`. It + reads only what the `filters` select, server-side, so a foreign resource cannot fail the query. Always filter (`vpc-id`, `group-id`): unfiltered, these are region-wide reads. Filter *values* are not validated - an id that does not exist yields an empty result, not an error - but an unrecognised filter *name* is fatal. Both alternatives can mislead here: @@ -59,7 +64,17 @@ result is written to `target` (`status.` or `context.`). Rule of thumb: *IDs by tag →* `GetResources`; *attributes / full inventory of a type →* `ListResources`; *EC2 identifiers CloudFormation does not model, or an -authoritative VPC-scoped read →* `DescribeEc2`. +authoritative VPC-scoped read →* the direct `Describe*` query types. + +On absent values, the direct EC2 describes take two deliberate policies. Fields +AWS always returns are projected with their zero value (`isEgress: false`, +`main: false`, `cidrBlock: ""`). Fields that are genuinely optional are +**omitted** rather than faked: `fromPort`/`toPort` are absent on a rule that has +no ports, and `referencedGroup*` only appears on a group-referencing rule. +Omitting is the honest choice - `fromPort: 0` is a real port - so guard in +templates (`{{ if .fromPort }}`) rather than assuming the key exists. Note real +EC2 does send `fromPort: -1, toPort: -1` for an all-protocol rule, so a truly +absent port is rarer than it looks. ## Input reference @@ -73,7 +88,6 @@ filters: # or filtersRef: (status./context values: ["prod"] parameters: # or parametersRef: ; scalar args per queryType: # allRegions, allAvailabilityZones (bool); owners, imageIds (csv) [EC2] - # operation: RouteTables | SecurityGroupRules | Subnets (DescribeEc2) [EC2] # serviceCode, quotaCode [Service Quotas] # typeName, resourceModel, roleArn, hydrate (default true) [Cloud Control] # resourceTypeFilters (csv) [Tagging API] diff --git a/aws.go b/aws.go index 640f1d3..490929b 100644 --- a/aws.go +++ b/aws.go @@ -53,15 +53,17 @@ type handler func(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, func (q *AWSQuery) registry() map[string]handler { return map[string]handler{ - "GetCallerIdentity": q.getCallerIdentity, - "DescribeRegions": q.describeRegions, - "DescribeAvailabilityZones": q.describeAvailabilityZones, - "DescribeImages": q.describeImages, - "DescribeEc2": q.describeEc2, - "ListServiceQuotas": q.listServiceQuotas, - "GetServiceQuota": q.getServiceQuota, - "ListResources": q.listResources, - "GetResources": q.getResources, + "GetCallerIdentity": q.getCallerIdentity, + "DescribeRegions": q.describeRegions, + "DescribeAvailabilityZones": q.describeAvailabilityZones, + "DescribeImages": q.describeImages, + "DescribeRouteTables": q.describeRouteTables, + "DescribeSubnets": q.describeSubnets, + "DescribeSecurityGroupRules": q.describeSecurityGroupRules, + "ListServiceQuotas": q.listServiceQuotas, + "GetServiceQuota": q.getServiceQuota, + "ListResources": q.listResources, + "GetResources": q.getResources, } } @@ -309,29 +311,33 @@ func (q *AWSQuery) describeImages(ctx context.Context, cfg aws.Config, in *v1bet return res, nil } -// describeEc2 runs one read-only EC2 Describe*, selected by -// parameters.operation. Filters are server-side, so unlike ListResources it -// never enumerates the account. -func (q *AWSQuery) describeEc2(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, error) { +// ec2Client validates the shared preconditions for the direct EC2 describes and +// returns a client. The filter guard is not optional hygiene: these calls are +// paginated and unbounded, so an empty filter set pages an entire region into XR +// status. It is reachable without a user typo - toFilters returns a non-nil +// empty slice, so a filtersRef that resolves to [] arrives here as len 0. +// describeImages guards the same way for the same reason. +// +// hint names the filters that operation actually accepts; they differ per +// operation and an unrecognised filter NAME is fatal, so a generic message +// would send the reader in the wrong direction. +func ec2Client(cfg aws.Config, in *v1beta1.Input, queryType, hint string) (*ec2.Client, error) { if cfg.Region == "" { - return nil, errRegionRequired("DescribeEc2") - } - client := ec2.NewFromConfig(cfg) - switch op := in.Parameters["operation"]; op { - case "RouteTables": - return describeRouteTables(ctx, client, in) - case "SecurityGroupRules": - return describeSecurityGroupRules(ctx, client, in) - case "Subnets": - return describeSubnets(ctx, client, in) - default: - return nil, errors.Errorf("DescribeEc2 requires parameters.operation to be one of RouteTables, SecurityGroupRules, Subnets (got %q)", op) + return nil, errRegionRequired(queryType) + } + if len(in.Filters) == 0 { + return nil, errors.Errorf("%s requires filters to avoid an unbounded region-wide read (e.g. %s)", queryType, hint) } + return ec2.NewFromConfig(cfg), nil } // describeRouteTables lists route tables with their associations (EC2, // paginated). The main association ID is not in the CloudFormation schema. -func describeRouteTables(ctx context.Context, client *ec2.Client, in *v1beta1.Input) (any, error) { +func (q *AWSQuery) describeRouteTables(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, error) { + client, err := ec2Client(cfg, in, "DescribeRouteTables", "vpc-id, route-table-id, association.subnet-id, tag:") + if err != nil { + return nil, err + } p := ec2.NewDescribeRouteTablesPaginator(client, &ec2.DescribeRouteTablesInput{Filters: toEC2Filters(in.Filters)}) res := []any{} for p.HasMorePages() { @@ -400,8 +406,12 @@ func routeTableRoutes(routes []ec2types.Route) []any { } // describeSecurityGroupRules lists security group rules (EC2, paginated). -// Scope with a group-id filter. -func describeSecurityGroupRules(ctx context.Context, client *ec2.Client, in *v1beta1.Input) (any, error) { +// Note the filter names: this operation does NOT accept vpc-id. +func (q *AWSQuery) describeSecurityGroupRules(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, error) { + client, err := ec2Client(cfg, in, "DescribeSecurityGroupRules", "group-id, security-group-rule-id, tag:") + if err != nil { + return nil, err + } p := ec2.NewDescribeSecurityGroupRulesPaginator(client, &ec2.DescribeSecurityGroupRulesInput{Filters: toEC2Filters(in.Filters)}) res := []any{} for p.HasMorePages() { @@ -423,8 +433,13 @@ func describeSecurityGroupRules(ctx context.Context, client *ec2.Client, in *v1b "description": aws.ToString(r.Description), "tags": ec2TagsToMap(r.Tags), } + // The full referenced-group identity, not just the id: a + // cross-account rule is otherwise indistinguishable from a local + // one, since sg-abc in another account is a different group. if r.ReferencedGroupInfo != nil { m["referencedGroupId"] = aws.ToString(r.ReferencedGroupInfo.GroupId) + m["referencedGroupUserId"] = aws.ToString(r.ReferencedGroupInfo.UserId) + m["referencedGroupVpcId"] = aws.ToString(r.ReferencedGroupInfo.VpcId) } putInt32(m, "fromPort", r.FromPort) putInt32(m, "toPort", r.ToPort) @@ -436,7 +451,11 @@ func describeSecurityGroupRules(ctx context.Context, client *ec2.Client, in *v1b // describeSubnets lists subnets (EC2, paginated). Returns only live subnets, // unlike the Tagging API. -func describeSubnets(ctx context.Context, client *ec2.Client, in *v1beta1.Input) (any, error) { +func (q *AWSQuery) describeSubnets(ctx context.Context, cfg aws.Config, in *v1beta1.Input) (any, error) { + client, err := ec2Client(cfg, in, "DescribeSubnets", "vpc-id, subnet-id, availability-zone, tag:") + if err != nil { + return nil, err + } p := ec2.NewDescribeSubnetsPaginator(client, &ec2.DescribeSubnetsInput{Filters: toEC2Filters(in.Filters)}) res := []any{} for p.HasMorePages() { @@ -458,6 +477,25 @@ func describeSubnets(ctx context.Context, client *ec2.Client, in *v1beta1.Input) "mapPublicIpOnLaunch": aws.ToBool(s.MapPublicIpOnLaunch), "tags": ec2TagsToMap(s.Tags), } + // IPv6 addressing. An IPv6-only subnet has no CidrBlock at all, so + // without these it projects cidrBlock:"" and is indistinguishable + // from a projection failure. AWS::EC2::Subnet models Ipv6CidrBlock, + // so omitting it would make this query strictly worse than the + // Cloud Control alternative it is meant to replace. + m["ipv6Native"] = aws.ToBool(s.Ipv6Native) + ipv6 := make([]any, 0, len(s.Ipv6CidrBlockAssociationSet)) + for _, a := range s.Ipv6CidrBlockAssociationSet { + state := "" + if a.Ipv6CidrBlockState != nil { + state = string(a.Ipv6CidrBlockState.State) + } + ipv6 = append(ipv6, map[string]any{ + "associationId": aws.ToString(a.AssociationId), + "ipv6CidrBlock": aws.ToString(a.Ipv6CidrBlock), + "state": state, + }) + } + m["ipv6CidrBlockAssociationSet"] = ipv6 putInt32(m, "availableIpAddressCount", s.AvailableIpAddressCount) res = append(res, m) } diff --git a/aws_test.go b/aws_test.go index 78cb2e8..be683fe 100644 --- a/aws_test.go +++ b/aws_test.go @@ -326,20 +326,21 @@ func TestHandlerValidationGuards(t *testing.T) { noRegion := aws.Config{} // empty region triggers the region-required guard cases := map[string]func() (any, error){ - "AZsNoRegion": func() (any, error) { return q.describeAvailabilityZones(ctx, noRegion, &v1beta1.Input{}) }, - "ImagesNoRegion": func() (any, error) { return q.describeImages(ctx, noRegion, &v1beta1.Input{}) }, - "QuotasNoRegion": func() (any, error) { return q.listServiceQuotas(ctx, noRegion, &v1beta1.Input{}) }, - "GetQuotaNoRegion": func() (any, error) { return q.getServiceQuota(ctx, noRegion, &v1beta1.Input{}) }, - "ListResNoRegion": func() (any, error) { return q.listResources(ctx, noRegion, &v1beta1.Input{}) }, - "GetResNoRegion": func() (any, error) { return q.getResources(ctx, noRegion, &v1beta1.Input{}) }, - "ImagesNoFilter": func() (any, error) { return q.describeImages(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, - "QuotasNoService": func() (any, error) { return q.listServiceQuotas(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, - "GetQuotaNoCodes": func() (any, error) { return q.getServiceQuota(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, - "ListResNoTypeName": func() (any, error) { return q.listResources(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, - "Ec2NoRegion": func() (any, error) { return q.describeEc2(ctx, noRegion, &v1beta1.Input{}) }, - "Ec2NoOperation": func() (any, error) { return q.describeEc2(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, - "Ec2BadOperation": func() (any, error) { - return q.describeEc2(ctx, stubCfg(&respStub{}), &v1beta1.Input{Parameters: map[string]string{"operation": "Vpcs"}}) + "AZsNoRegion": func() (any, error) { return q.describeAvailabilityZones(ctx, noRegion, &v1beta1.Input{}) }, + "ImagesNoRegion": func() (any, error) { return q.describeImages(ctx, noRegion, &v1beta1.Input{}) }, + "QuotasNoRegion": func() (any, error) { return q.listServiceQuotas(ctx, noRegion, &v1beta1.Input{}) }, + "GetQuotaNoRegion": func() (any, error) { return q.getServiceQuota(ctx, noRegion, &v1beta1.Input{}) }, + "ListResNoRegion": func() (any, error) { return q.listResources(ctx, noRegion, &v1beta1.Input{}) }, + "GetResNoRegion": func() (any, error) { return q.getResources(ctx, noRegion, &v1beta1.Input{}) }, + "ImagesNoFilter": func() (any, error) { return q.describeImages(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, + "QuotasNoService": func() (any, error) { return q.listServiceQuotas(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, + "GetQuotaNoCodes": func() (any, error) { return q.getServiceQuota(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, + "ListResNoTypeName": func() (any, error) { return q.listResources(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, + "RouteTablesNoRegion": func() (any, error) { return q.describeRouteTables(ctx, noRegion, &v1beta1.Input{}) }, + "SubnetsNoRegion": func() (any, error) { return q.describeSubnets(ctx, noRegion, &v1beta1.Input{}) }, + "SGRulesNoRegion": func() (any, error) { return q.describeSecurityGroupRules(ctx, noRegion, &v1beta1.Input{}) }, + "SubnetsNoFilters": func() (any, error) { + return q.describeSubnets(ctx, stubCfg(&respStub{}), &v1beta1.Input{}) }, } for name, call := range cases { @@ -530,32 +531,37 @@ const ( `` ) -func ec2Input(operation string) *v1beta1.Input { - return &v1beta1.Input{ - Parameters: map[string]string{"operation": operation}, - Filters: []v1beta1.Filter{{Name: "vpc-id", Values: []string{"vpc-1"}}}, +// ec2Input builds an input for one query type with a filter that query type +// actually supports. This is not cosmetic: DescribeSecurityGroupRules accepts +// only group-id, security-group-rule-id and tag:, and an unrecognised +// filter NAME is fatal - so a shared "vpc-id" would put a request on the wire +// that AWS rejects, while respStub's canned body made the suite pass anyway. +func ec2Input(queryType string) *v1beta1.Input { + name, value := "vpc-id", "vpc-1" + if queryType == "DescribeSecurityGroupRules" { + name, value = "group-id", "sg-1" } + return &v1beta1.Input{Filters: []v1beta1.Filter{{Name: name, Values: []string{value}}}} } -// TestDescribeEc2Dispatches proves every allow-listed operation reaches its own -// describe call, and that an unsupported one names the supported values. +// TestDescribeEc2Dispatches proves each direct EC2 describe is registered under +// its own queryType and reaches its own describe call. An unsupported value no +// longer needs a runtime case: queryType is CRD-enum validated, so a typo is +// rejected at admission instead of aborting a composition at reconcile. func TestDescribeEc2Dispatches(t *testing.T) { - if newQuery().registry()["DescribeEc2"] == nil { - t.Fatal("DescribeEc2 is not wired into the handler registry") - } - - cases := map[string]struct { - operation string - body string - }{ - "RouteTables": {operation: "RouteTables", body: routeTablesPage2}, - "SecurityGroupRules": {operation: "SecurityGroupRules", body: securityGroupRulesBody}, - "Subnets": {operation: "Subnets", body: subnetsBody}, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - got, err := newQuery().describeEc2(context.Background(), - stubCfg(&respStub{bodies: []string{tc.body}, contentType: "text/xml"}), ec2Input(tc.operation)) + cases := map[string]string{ + "DescribeRouteTables": routeTablesPage2, + "DescribeSubnets": subnetsBody, + "DescribeSecurityGroupRules": securityGroupRulesBody, + } + for queryType, body := range cases { + t.Run(queryType, func(t *testing.T) { + h := newQuery().registry()[queryType] + if h == nil { + t.Fatalf("%s is not wired into the handler registry", queryType) + } + got, err := h(context.Background(), + stubCfg(&respStub{bodies: []string{body}, contentType: "text/xml"}), ec2Input(queryType)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -565,25 +571,32 @@ func TestDescribeEc2Dispatches(t *testing.T) { } }) } +} - t.Run("Unsupported", func(t *testing.T) { - _, err := newQuery().describeEc2(context.Background(), stubCfg(&respStub{}), ec2Input("Vpcs")) - if err == nil { - t.Fatal("expected an error for an unsupported operation") - } - for _, want := range []string{"RouteTables", "SecurityGroupRules", "Subnets"} { - if !strings.Contains(err.Error(), want) { - t.Errorf("error should name %s: %v", want, err) +// TestDescribeEc2FilterGuard pins the must-filter guard. These calls are +// paginated and unbounded, so an empty filter set would page a whole region +// into XR status. It is reachable without a typo: toFilters returns a non-nil +// empty slice, so a filtersRef resolving to [] arrives with len 0. +func TestDescribeEc2FilterGuard(t *testing.T) { + for _, queryType := range []string{"DescribeRouteTables", "DescribeSubnets", "DescribeSecurityGroupRules"} { + t.Run(queryType, func(t *testing.T) { + h := newQuery().registry()[queryType] + _, err := h(context.Background(), stubCfg(&respStub{}), &v1beta1.Input{}) + if err == nil { + t.Fatal("expected the filter guard to reject an empty filter set") } - } - }) + if !strings.Contains(err.Error(), "requires filters") { + t.Errorf("expected the filter guard, got: %v", err) + } + }) + } } // TestDescribeEc2RouteTablesPaginates covers the projection (associations, incl. // the main association ID) across two pages. func TestDescribeEc2RouteTablesPaginates(t *testing.T) { stub := &respStub{bodies: []string{routeTablesPage1, routeTablesPage2}, contentType: "text/xml"} - got, err := newQuery().describeEc2(context.Background(), stubCfg(stub), ec2Input("RouteTables")) + got, err := newQuery().describeRouteTables(context.Background(), stubCfg(stub), ec2Input("DescribeRouteTables")) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -621,6 +634,9 @@ func TestDescribeEc2RouteTablesPaginates(t *testing.T) { t.Fatalf("expected 2 requests (one per page), got %d", len(stub.requests)) } // vpc-id must go server-side - that is the whole point over ListResources. + if len(stub.requests) == 0 { + t.Fatal("no request recorded") + } if !strings.Contains(stub.requests[0], "Filter.1.Name=vpc-id") { t.Errorf("vpc-id filter not sent server-side: %s", stub.requests[0]) } @@ -629,8 +645,7 @@ func TestDescribeEc2RouteTablesPaginates(t *testing.T) { // Isolates the region guard: the shared guards table only asserts err != nil, // which the SDK's endpoint-resolution error satisfies on its own. func TestDescribeEc2RegionGuard(t *testing.T) { - in := &v1beta1.Input{Parameters: map[string]string{"operation": "Subnets"}} - _, err := newQuery().describeEc2(context.Background(), aws.Config{}, in) + _, err := newQuery().describeSubnets(context.Background(), aws.Config{}, ec2Input("DescribeSubnets")) if err == nil { t.Fatal("expected an error, got nil") } @@ -641,7 +656,7 @@ func TestDescribeEc2RegionGuard(t *testing.T) { func TestDescribeEc2Subnets(t *testing.T) { stub := &respStub{bodies: []string{subnetsBody}, contentType: "text/xml"} - got, err := newQuery().describeEc2(context.Background(), stubCfg(stub), ec2Input("Subnets")) + got, err := newQuery().describeSubnets(context.Background(), stubCfg(stub), ec2Input("DescribeSubnets")) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -650,11 +665,17 @@ func TestDescribeEc2Subnets(t *testing.T) { "vpcId": "vpc-1", "ownerId": "123456789012", "availabilityZone": "eu-central-1a", "availabilityZoneId": "euc1-az2", "cidrBlock": "10.0.1.0/24", "state": "available", "defaultForAz": false, "mapPublicIpOnLaunch": true, "availableIpAddressCount": int64(250), + // Always projected, so an IPv6-only subnet is distinguishable from a + // projection failure. This fixture is IPv4-only, hence the empty set. + "ipv6Native": false, "ipv6CidrBlockAssociationSet": []any{}, "tags": map[string]any{"Name": "public-a"}, }} if diff := cmp.Diff(want, got); diff != "" { t.Errorf("-want +got:\n%s", diff) } + if len(stub.requests) == 0 { + t.Fatal("no request recorded") + } if !strings.Contains(stub.requests[0], "Filter.1.Name=vpc-id") { t.Errorf("filter not sent server-side: %s", stub.requests[0]) } @@ -664,7 +685,7 @@ func TestDescribeEc2Subnets(t *testing.T) { // on the wire - a live all-protocol rule reports -1/-1, not nothing. func TestDescribeEc2SecurityGroupRules(t *testing.T) { stub := &respStub{bodies: []string{securityGroupRulesBody}, contentType: "text/xml"} - got, err := newQuery().describeEc2(context.Background(), stubCfg(stub), ec2Input("SecurityGroupRules")) + got, err := newQuery().describeSecurityGroupRules(context.Background(), stubCfg(stub), ec2Input("DescribeSecurityGroupRules")) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -683,6 +704,9 @@ func TestDescribeEc2SecurityGroupRules(t *testing.T) { "isEgress": true, "ipProtocol": "-1", "fromPort": int64(-1), "toPort": int64(-1), "cidrIpv4": "", "cidrIpv6": "", "prefixListId": "", "description": "", "referencedGroupId": "sg-2", + // Kept alongside the id so a cross-account reference is not + // mistaken for a local group. Empty in this fixture. + "referencedGroupUserId": "", "referencedGroupVpcId": "", "tags": map[string]any{}, }, map[string]any{ @@ -695,7 +719,10 @@ func TestDescribeEc2SecurityGroupRules(t *testing.T) { if diff := cmp.Diff(want, got); diff != "" { t.Errorf("-want +got:\n%s", diff) } - if !strings.Contains(stub.requests[0], "Filter.1.Name=vpc-id") { + if len(stub.requests) == 0 { + t.Fatal("no request recorded") + } + if !strings.Contains(stub.requests[0], "Filter.1.Name=group-id") { t.Errorf("filter not sent server-side: %s", stub.requests[0]) } } diff --git a/example/Makefile b/example/Makefile index b49f6ca..35accd6 100644 --- a/example/Makefile +++ b/example/Makefile @@ -37,6 +37,9 @@ service-quotas|xr.yaml|composition-service-quotas.yaml| cloudcontrol-vpcs|xr.yaml|composition-cloudcontrol-vpcs.yaml| cloudcontrol-ids|xr.yaml|composition-cloudcontrol-ids.yaml| tagging-subnets|xr.yaml|composition-tagging-subnets.yaml| +ec2-route-tables|xr.yaml|composition-ec2-route-tables.yaml| +ec2-subnets|xr.yaml|composition-ec2-subnets.yaml| +ec2-security-group-rules|xr.yaml|composition-ec2-security-group-rules.yaml| dynamic-refs|xr-dynamic.yaml|composition-dynamic-refs.yaml| dynamic-context|xr.yaml|composition-dynamic-context.yaml|--context-values='$(CTX)' endef diff --git a/example/README.md b/example/README.md index fdac288..3d14be6 100644 --- a/example/README.md +++ b/example/README.md @@ -47,7 +47,9 @@ status: | `composition-caller-identity.yaml` | GetCallerIdentity | STS | `status.callerIdentity` | | `composition-availability-zones.yaml` | DescribeAvailabilityZones | EC2 | `status.availabilityZones` | | `composition-ami-lookup.yaml` | DescribeImages | EC2 | `status.amis` | -| `composition-ec2-route-tables.yaml` | DescribeEc2 (`operation=RouteTables`) | EC2 | `status.routeTables` | +| `composition-ec2-route-tables.yaml` | DescribeRouteTables | EC2 | `status.routeTables` | +| `composition-ec2-subnets.yaml` | DescribeSubnets | EC2 | `status.subnets` | +| `composition-ec2-security-group-rules.yaml` | DescribeSecurityGroupRules | EC2 | `status.securityGroupRules` | | `composition-service-quotas.yaml` | ListServiceQuotas | Service Quotas | `status.ec2Quotas` | | `composition-cloudcontrol-vpcs.yaml` | ListResources | Cloud Control | `status.prodVpcs` | | `composition-cloudcontrol-ids.yaml` | ListResources (`hydrate=false`) | Cloud Control | `status.vpcIds` | diff --git a/example/composition-ec2-route-tables.yaml b/example/composition-ec2-route-tables.yaml index 45d9b71..bb301cc 100644 --- a/example/composition-ec2-route-tables.yaml +++ b/example/composition-ec2-route-tables.yaml @@ -17,10 +17,8 @@ spec: input: apiVersion: aws.fn.crossplane.io/v1beta1 kind: Input - queryType: DescribeEc2 + queryType: DescribeRouteTables region: eu-central-1 - parameters: - operation: RouteTables filters: - name: vpc-id values: ["vpc-0123456789abcdef0"] diff --git a/example/composition-ec2-security-group-rules.yaml b/example/composition-ec2-security-group-rules.yaml new file mode 100644 index 0000000..8f82ce8 --- /dev/null +++ b/example/composition-ec2-security-group-rules.yaml @@ -0,0 +1,35 @@ +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: function-aws-query-ec2-security-group-rules +spec: + compositeTypeRef: + apiVersion: example.io/v1alpha1 + kind: XAccount + mode: Pipeline + pipeline: + # Rules of one security group via EC2. + # + # Note the filter name. This operation accepts only group-id, + # security-group-rule-id and tag: - NOT vpc-id, which the other two EC2 + # describes do accept. An unrecognised filter name is fatal and aborts the + # whole composition, so the filters are not interchangeable between these + # query types. + - step: rules-by-group + functionRef: + name: function-aws-query + input: + apiVersion: aws.fn.crossplane.io/v1beta1 + kind: Input + queryType: DescribeSecurityGroupRules + region: eu-central-1 + filters: + - name: group-id + values: ["sg-0123456789abcdef0"] + target: status.securityGroupRules + credentials: + - name: aws-creds + source: Secret + secretRef: + namespace: crossplane-system + name: aws-creds diff --git a/example/composition-ec2-subnets.yaml b/example/composition-ec2-subnets.yaml new file mode 100644 index 0000000..1d484ed --- /dev/null +++ b/example/composition-ec2-subnets.yaml @@ -0,0 +1,31 @@ +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: function-aws-query-ec2-subnets +spec: + compositeTypeRef: + apiVersion: example.io/v1alpha1 + kind: XAccount + mode: Pipeline + pipeline: + # Subnets for one VPC via EC2. Unlike the Tagging API this returns only live + # subnets, so a deleted subnet cannot shadow its live replacement under the + # same tag. The vpc-id filter is server-side and required. + - step: subnets-by-vpc + functionRef: + name: function-aws-query + input: + apiVersion: aws.fn.crossplane.io/v1beta1 + kind: Input + queryType: DescribeSubnets + region: eu-central-1 + filters: + - name: vpc-id + values: ["vpc-0123456789abcdef0"] + target: status.subnets + credentials: + - name: aws-creds + source: Secret + secretRef: + namespace: crossplane-system + name: aws-creds diff --git a/input/v1beta1/input.go b/input/v1beta1/input.go index bc67da6..6bbdf3a 100644 --- a/input/v1beta1/input.go +++ b/input/v1beta1/input.go @@ -21,7 +21,7 @@ type Input struct { metav1.ObjectMeta `json:"metadata,omitempty"` // QueryType selects the AWS read operation to perform. - // +kubebuilder:validation:Enum=GetCallerIdentity;DescribeRegions;DescribeAvailabilityZones;DescribeImages;DescribeEc2;ListServiceQuotas;GetServiceQuota;ListResources;GetResources + // +kubebuilder:validation:Enum=GetCallerIdentity;DescribeRegions;DescribeAvailabilityZones;DescribeImages;DescribeRouteTables;DescribeSubnets;DescribeSecurityGroupRules;ListServiceQuotas;GetServiceQuota;ListResources;GetResources QueryType string `json:"queryType"` // Region to target. Optional for global-ish calls (GetCallerIdentity, @@ -34,10 +34,19 @@ type Input struct { // +optional RegionRef *string `json:"regionRef,omitempty"` - // Filters are name/values pairs. Their interpretation depends on QueryType: - // - EC2 ops (DescribeRegions, DescribeAvailabilityZones, DescribeImages, - // DescribeEc2): EC2 filter names such as "tag:Name", "state", - // "architecture", "vpc-id", "group-id" (server-side). + // Filters are name/values pairs. Their interpretation depends on QueryType. + // For every EC2 query these are native EC2 filter names, applied + // server-side, and an unrecognised NAME is fatal - so they are listed per + // query type rather than generically. They are NOT interchangeable: + // - DescribeRegions, DescribeAvailabilityZones, DescribeImages: + // "tag:Name", "state", "architecture", ... + // - DescribeRouteTables (required): "vpc-id", "route-table-id", + // "association.subnet-id", "tag:", ... + // - DescribeSubnets (required): "vpc-id", "subnet-id", + // "availability-zone", "tag:", ... + // - DescribeSecurityGroupRules (required): "group-id", + // "security-group-rule-id", "tag:". This operation does NOT + // accept "vpc-id". // - GetResources (Tagging API): each entry is a tag filter where name is // the tag key and values are the tag values (server-side). // - ListResources (Cloud Control): client-side property match where name @@ -53,9 +62,6 @@ type Input struct { // Parameters carries scalar/string-list args specific to each QueryType: // allRegions, allAvailabilityZones (bool); owners, imageIds (csv) [EC2] - // operation: which EC2 describe to run - RouteTables (incl. their - // associations, i.e. the main association ID), SecurityGroupRules, - // Subnets. Required by DescribeEc2; bound by "filters" [EC2] // serviceCode, quotaCode [ServiceQuotas] // typeName (e.g. AWS::EC2::VPC), resourceModel (json), roleArn, // hydrate (bool, default true: GetResource each item for full props) [Cloud Control] diff --git a/package/crossplane.yaml b/package/crossplane.yaml index e9ab39a..1559d52 100644 --- a/package/crossplane.yaml +++ b/package/crossplane.yaml @@ -15,7 +15,7 @@ metadata: queries (caller identity, regions, availability zones, AMI lookups, service quotas) and generic existing-resource discovery (AWS Cloud Control ListResources, the Resource Groups Tagging API GetResources, and direct - EC2 describes via DescribeEc2). + EC2 describes for route tables, subnets and security group rules). Authentication mirrors the Official AWS Provider: the credentials secret is a shared-credentials INI compatible with provider-upjet-aws, and the diff --git a/package/input/aws.fn.crossplane.io_inputs.yaml b/package/input/aws.fn.crossplane.io_inputs.yaml index ccf5387..f4689ab 100644 --- a/package/input/aws.fn.crossplane.io_inputs.yaml +++ b/package/input/aws.fn.crossplane.io_inputs.yaml @@ -32,10 +32,19 @@ spec: type: string filters: description: |- - Filters are name/values pairs. Their interpretation depends on QueryType: - - EC2 ops (DescribeRegions, DescribeAvailabilityZones, DescribeImages, - DescribeEc2): EC2 filter names such as "tag:Name", "state", - "architecture", "vpc-id", "group-id" (server-side). + Filters are name/values pairs. Their interpretation depends on QueryType. + For every EC2 query these are native EC2 filter names, applied + server-side, and an unrecognised NAME is fatal - so they are listed per + query type rather than generically. They are NOT interchangeable: + - DescribeRegions, DescribeAvailabilityZones, DescribeImages: + "tag:Name", "state", "architecture", ... + - DescribeRouteTables (required): "vpc-id", "route-table-id", + "association.subnet-id", "tag:", ... + - DescribeSubnets (required): "vpc-id", "subnet-id", + "availability-zone", "tag:", ... + - DescribeSecurityGroupRules (required): "group-id", + "security-group-rule-id", "tag:". This operation does NOT + accept "vpc-id". - GetResources (Tagging API): each entry is a tag filter where name is the tag key and values are the tag values (server-side). - ListResources (Cloud Control): client-side property match where name @@ -164,9 +173,6 @@ spec: description: |- Parameters carries scalar/string-list args specific to each QueryType: allRegions, allAvailabilityZones (bool); owners, imageIds (csv) [EC2] - operation: which EC2 describe to run - RouteTables (incl. their - associations, i.e. the main association ID), SecurityGroupRules, - Subnets. Required by DescribeEc2; bound by "filters" [EC2] serviceCode, quotaCode [ServiceQuotas] typeName (e.g. AWS::EC2::VPC), resourceModel (json), roleArn, hydrate (bool, default true: GetResource each item for full props) [Cloud Control] @@ -190,7 +196,9 @@ spec: - DescribeRegions - DescribeAvailabilityZones - DescribeImages - - DescribeEc2 + - DescribeRouteTables + - DescribeSubnets + - DescribeSecurityGroupRules - ListServiceQuotas - GetServiceQuota - ListResources