From 7076cd0badeced34e776c0dc5d3e2ad87f274c1a Mon Sep 17 00:00:00 2001 From: Elim Tsiagbey Date: Wed, 6 Nov 2024 12:43:06 -0500 Subject: [PATCH] fix: filter building (#17) - update filter building for `dryRunValidation` to append with `AND` - Add test coverage for filter building and dryRunValidation --- plugin/service/host/attributes_test.go | 80 +++++++++ plugin/service/host/plugin.go | 6 +- plugin/service/host/plugin_test.go | 227 ++++++++++++++++++++++++- 3 files changed, 309 insertions(+), 4 deletions(-) diff --git a/plugin/service/host/attributes_test.go b/plugin/service/host/attributes_test.go index 83f4f37..8f43c0e 100644 --- a/plugin/service/host/attributes_test.go +++ b/plugin/service/host/attributes_test.go @@ -6,6 +6,7 @@ package host import ( "testing" + computepb "cloud.google.com/go/compute/apiv1/computepb" cred "github.com/hashicorp/boundary-plugin-gcp/internal/credential" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" @@ -126,3 +127,82 @@ func TestGetSetAttributes(t *testing.T) { }) } } + +func TestBuildListInstancesRequest(t *testing.T) { + cases := []struct { + name string + attributes *SetAttributes + catalog *CatalogAttributes + expected *computepb.ListInstancesRequest + expectedErrContains string + }{ + { + name: "valid request with single filter", + attributes: &SetAttributes{ + Filters: []string{"tags.items=my-tag"}, + }, + catalog: &CatalogAttributes{ + CredentialAttributes: &cred.CredentialAttributes{ + ProjectId: "test-12345", + Zone: "us-central-1a", + }, + }, + expected: &computepb.ListInstancesRequest{ + Project: "test-12345", + Zone: "us-central-1a", + Filter: stringPtr("tags.items=my-tag AND status = running"), + }, + }, + { + name: "valid request with multiple filters", + attributes: &SetAttributes{ + Filters: []string{"tags.items=my-tag", "tags.items=my-other-tag"}, + }, + catalog: &CatalogAttributes{ + CredentialAttributes: &cred.CredentialAttributes{ + ProjectId: "test-12345", + Zone: "us-central-1a", + }, + }, + expected: &computepb.ListInstancesRequest{ + Project: "test-12345", + Zone: "us-central-1a", + Filter: stringPtr("tags.items=my-tag AND tags.items=my-other-tag AND status = running"), + }, + }, + { + name: "error building filters", + attributes: &SetAttributes{ + Filters: []string{"invalid-filter"}, + }, + catalog: &CatalogAttributes{ + CredentialAttributes: &cred.CredentialAttributes{ + ProjectId: "test-12345", + Zone: "us-central-1a", + }, + }, + expectedErrContains: "error building filters", + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + + actual, err := buildListInstancesRequest(tc.attributes, tc.catalog) + if tc.expectedErrContains != "" { + require.Error(err) + require.Contains(err.Error(), tc.expectedErrContains) + return + } + + require.NoError(err) + require.Equal(tc.expected, actual) + }) + } +} + +func stringPtr(s string) *string { + return &s +} diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index 8d49055..c28521c 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -624,7 +624,7 @@ func dryRunValidation( var filters string if len(listFilters) > 0 { - filters = strings.Join(listFilters, " ") + filters = strings.Join(listFilters, " AND ") } it := instancesClient.List(ctx, &computepb.ListInstancesRequest{ @@ -649,10 +649,10 @@ func buildFilters(attrs *SetAttributes) ([]string, error) { // If filters are provided, validate them if len(attrs.Filters) != 0 { for _, filterAttr := range attrs.Filters { - key, operator, value, valid := extractFilterValue(filterAttr) + key, _, value, valid := extractFilterValue(filterAttr) switch { case !valid: - return nil, fmt.Errorf("filter %q contains invalid operator: %s. Use one of =, !=, >, <, <=, >=, :, eq, ne", filterAttr, operator) + return nil, fmt.Errorf("invalid filter %q. Ensure the operator is one of =, !=, >, <, <=, >=, :, eq, ne", filterAttr) case len(strings.TrimSpace(key)) == 0: return nil, fmt.Errorf("filter %q contains an empty filter key", filterAttr) case len(strings.TrimSpace(value)) == 0: diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index c62d94e..3954d6a 100644 --- a/plugin/service/host/plugin_test.go +++ b/plugin/service/host/plugin_test.go @@ -663,7 +663,7 @@ func TestListHosts(t *testing.T) { }, }, }, - expectedErr: "error building filters: filter \"not-a-filter\" contains invalid operator", + expectedErr: "error building filters: invalid filter \"not-a-filter\"", expectedErrCode: codes.InvalidArgument, }, } @@ -2191,3 +2191,228 @@ func TestNormalizeSetData(t *testing.T) { }) } } + +func TestBuildFilters(t *testing.T) { + cases := []struct { + name string + attrs *SetAttributes + expected []string + expectedErr string + }{ + { + name: "no filters provided", + attrs: &SetAttributes{ + Filters: []string{}, + }, + expected: []string{defaultStatusFilter}, + }, + { + name: "valid filters provided", + attrs: &SetAttributes{ + Filters: []string{"name = instance-1", "zone = us-central1-a"}, + }, + expected: []string{"name = instance-1", "zone = us-central1-a", defaultStatusFilter}, + }, + { + name: "filter with invalid operator", + attrs: &SetAttributes{ + Filters: []string{"name ~ instance-1"}, + }, + expectedErr: "invalid filter \"name ~ instance-1\"", + }, + { + name: "filter with empty key", + attrs: &SetAttributes{ + Filters: []string{" = instance-1"}, + }, + expectedErr: "invalid filter \" = instance-1\"", + }, + { + name: "filter with empty value", + attrs: &SetAttributes{ + Filters: []string{"name = "}, + }, + expectedErr: "filter \"name = \" contains an empty value", + }, + { + name: "filter with status key", + attrs: &SetAttributes{ + Filters: []string{"status = terminated"}, + }, + expected: []string{"status = terminated"}, + }, + { + name: "filter with multiple filters", + attrs: &SetAttributes{ + Filters: []string{"status = terminated", "name = instance-1"}, + }, + expected: []string{"status = terminated", "name = instance-1"}, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + + actual, err := buildFilters(tc.attrs) + if tc.expectedErr != "" { + require.Contains(err.Error(), tc.expectedErr) + return + } + require.NoError(err) + require.Equal(tc.expected, actual) + }) + } +} + +func TestDryRunValidation(t *testing.T) { + ctx := context.Background() + + cases := []struct { + name string + state *gcpCatalogPersistedState + listFilters []string + clientOptions []option.ClientOption + expectedErr string + expectedErrCode codes.Code + }{ + { + name: "nil state", + state: nil, + expectedErr: "persisted state is required", + expectedErrCode: codes.InvalidArgument, + }, + { + name: "error getting instances client", + state: &gcpCatalogPersistedState{ + PersistedState: &cred.PersistedState{ + CredentialsConfig: &cred.Config{ + ProjectId: "test-project", + Zone: "us-central1-a", + }, + }, + testInstancesAPIFunc: newTestMockInstances(ctx, + nil, + testMockInstancesWithListInstancesOutput(nil), + testMockInstancesWithListInstancesError(fmt.Errorf("failed to list instances")), + ), + }, + clientOptions: []option.ClientOption{ + option.WithHTTPClient(&http.Client{}), + }, + expectedErr: "gcp list instances failed", + expectedErrCode: codes.FailedPrecondition, + }, + { + name: "list instances error", + state: &gcpCatalogPersistedState{ + PersistedState: &cred.PersistedState{ + CredentialsConfig: &cred.Config{ + ProjectId: "test-project", + Zone: "us-central1-a", + }, + }, + testInstancesAPIFunc: newTestMockInstances(ctx, + nil, + testMockInstancesWithListInstancesOutput(nil), + testMockInstancesWithListInstancesError(fmt.Errorf("failed to list instances")), + ), + }, + expectedErr: "gcp list instances failed", + expectedErrCode: codes.FailedPrecondition, + }, + { + name: "success with filters", + state: &gcpCatalogPersistedState{ + PersistedState: &cred.PersistedState{ + CredentialsConfig: &cred.Config{ + ProjectId: "test-project", + Zone: "us-central1-a", + }, + }, + testInstancesAPIFunc: newTestMockInstances(ctx, + nil, + testMockInstancesWithListInstancesOutput(&computepb.InstanceList{ + Items: []*computepb.Instance{ + { + Name: pointer("boundary-1"), + Id: pointer(uint64(1)), + NetworkInterfaces: []*computepb.NetworkInterface{ + { + AccessConfigs: []*computepb.AccessConfig{ + { + NatIP: pointer("102.1.1.1"), + ExternalIpv6: pointer("2001:db8::1"), + }, + }, + Ipv6AccessConfigs: []*computepb.AccessConfig{ + { + ExternalIpv6: pointer("2001:db8::1"), + }, + }, + }, + }, + }, + }, + }), + testMockInstancesWithListInstancesError(nil), + ), + }, + listFilters: []string{"name = instance-1", "zone = us-central1-a"}, + }, + { + name: "success without filters", + state: &gcpCatalogPersistedState{ + PersistedState: &cred.PersistedState{ + CredentialsConfig: &cred.Config{ + ProjectId: "test-project", + Zone: "us-central1-a", + }, + }, + testInstancesAPIFunc: newTestMockInstances(ctx, + nil, + testMockInstancesWithListInstancesOutput(&computepb.InstanceList{ + Items: []*computepb.Instance{ + { + Name: pointer("boundary-1"), + Id: pointer(uint64(1)), + NetworkInterfaces: []*computepb.NetworkInterface{ + { + AccessConfigs: []*computepb.AccessConfig{ + { + NatIP: pointer("102.1.1.1"), + ExternalIpv6: pointer("2001:db8::1"), + }, + }, + Ipv6AccessConfigs: []*computepb.AccessConfig{ + { + ExternalIpv6: pointer("2001:db8::1"), + }, + }, + }, + }, + }, + }, + }), + testMockInstancesWithListInstancesError(nil), + ), + }, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + + st := dryRunValidation(ctx, tc.state, tc.listFilters, tc.clientOptions...) + if tc.expectedErr != "" { + require.Contains(st.Err().Error(), tc.expectedErr) + require.Equal(st.Code(), tc.expectedErrCode) + return + } + require.NoError(st.Err()) + }) + } +}