From 13a3da0ce8b82ea3b394177d4a49c4d61ff3ba70 Mon Sep 17 00:00:00 2001 From: Mark Phelps <209477+markphelps@users.noreply.github.com> Date: Tue, 18 Feb 2025 15:00:27 -0500 Subject: [PATCH 1/5] chore: fix more unit tests Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --- .../config/testdata/marshal/yaml/default.yml | 4 +--- internal/info/flipt_test.go | 2 +- .../authz/middleware/grpc/middleware_test.go | 17 +++++--------- internal/storage/analytics/migrator_test.go | 9 ++++---- .../environments/fs/namespaces_test.go | 6 ++--- internal/storage/fs/snapshot_test.go | 22 +++--------------- rpc/flipt/auth/request.go | 2 +- rpc/flipt/request.go | 23 +------------------ rpc/v2/environments/request.go | 2 +- 9 files changed, 22 insertions(+), 65 deletions(-) diff --git a/internal/config/testdata/marshal/yaml/default.yml b/internal/config/testdata/marshal/yaml/default.yml index 92da16c8d1..be868068af 100644 --- a/internal/config/testdata/marshal/yaml/default.yml +++ b/internal/config/testdata/marshal/yaml/default.yml @@ -31,11 +31,9 @@ server: environments: default: + default: true name: default storage: default - second: - name: second - storage: default storage: default: diff --git a/internal/info/flipt_test.go b/internal/info/flipt_test.go index a850c7d8ba..705b8c13fe 100644 --- a/internal/info/flipt_test.go +++ b/internal/info/flipt_test.go @@ -38,5 +38,5 @@ func TestHttpHandler(t *testing.T) { w := httptest.NewRecorder() f.ServeHTTP(w, r) assert.Equal(t, http.StatusOK, w.Code) - assert.Equal(t, `{"updateAvailable":false,"isRelease":false,"authentication":{"required":false},"storage":{"type":"database"}}`, w.Body.String()) + assert.JSONEq(t, `{"updateAvailable":false,"isRelease":false,"authentication":{"required":false}}`, w.Body.String()) } diff --git a/internal/server/authz/middleware/grpc/middleware_test.go b/internal/server/authz/middleware/grpc/middleware_test.go index 264e0f5711..491fbb7eda 100644 --- a/internal/server/authz/middleware/grpc/middleware_test.go +++ b/internal/server/authz/middleware/grpc/middleware_test.go @@ -13,6 +13,7 @@ import ( "go.flipt.io/flipt/rpc/v2/environments" "go.uber.org/zap" "google.golang.org/grpc" + "google.golang.org/protobuf/types/known/anypb" ) type mockPolicyVerifier struct { @@ -73,6 +74,10 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) { Environment: "default", Namespace: "default", Key: "some_flag", + Payload: &anypb.Any{ + TypeUrl: "flipt.core.Flag", + Value: []byte(`{"key":"some_flag","name":"Some Flag","description":"Some description","enabled":true}`), + }, }, validatorAllowed: true, wantAllowed: true, @@ -80,8 +85,7 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) { "request": flipt.Request{ Namespace: "default", Resource: flipt.ResourceFlag, - Subject: flipt.SubjectFlag, - Action: flipt.ActionCreate, + Action: flipt.ActionUpdate, Status: flipt.StatusSuccess, }, "authentication": adminAuth, @@ -97,15 +101,6 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) { }, validatorAllowed: false, wantAllowed: false, - authzInput: map[string]any{ - "request": flipt.Request{ - Namespace: "default", - Resource: flipt.ResourceFlag, - Subject: flipt.SubjectFlag, - Action: flipt.ActionCreate, - }, - "authentication": adminAuth, - }, }, { name: "skips authz", diff --git a/internal/storage/analytics/migrator_test.go b/internal/storage/analytics/migrator_test.go index 6d332c4c5a..a82cdc3d53 100644 --- a/internal/storage/analytics/migrator_test.go +++ b/internal/storage/analytics/migrator_test.go @@ -83,14 +83,15 @@ func TestMigratorRun_NoChange(t *testing.T) { func TestMigratorExpectedVersions(t *testing.T) { for _, driver := range stringToDriver { - migrations, err := os.ReadDir(filepath.Join("../../../config/migrations", driver.Migrations())) + migrations, err := os.ReadDir(filepath.Join("../../migrations", driver.Migrations())) require.NoError(t, err) count := len(migrations) require.Positive(t, count, "no migrations found for %s", driver) - // migrations start at 0 - actual := uint(count - 1) - assert.Equal(t, actual, expectedVersions[driver], "expectedVersions for %s should be set to %d. you need to increment expectedVersions after adding a new migration", driver, actual) + require.Equal(t, count%2, 0, "migrations for %s should be even", driver) + // we only care about the up migrations not the down migrations + actual := count / 2 + assert.Equal(t, uint(actual), expectedVersions[driver], "expectedVersions for %s should be set to %d. you need to increment expectedVersions after adding a new migration", driver, actual) } } diff --git a/internal/storage/environments/fs/namespaces_test.go b/internal/storage/environments/fs/namespaces_test.go index 3609d09238..5fd4f6aace 100644 --- a/internal/storage/environments/fs/namespaces_test.go +++ b/internal/storage/environments/fs/namespaces_test.go @@ -16,14 +16,14 @@ import ( ) var ( - defaultContents = `version: "1.4" + defaultContents = `version: "1.5" namespace: key: default name: Default description: The default namespace ` - teamAContents = `version: "1.4" + teamAContents = `version: "1.5" namespace: key: team_a name: Team A @@ -130,7 +130,7 @@ func Test_NamespaceStorage_PutNamespace(t *testing.T) { data, err := io.ReadAll(fi) require.NoError(t, err) - assert.Equal(t, `version: "1.4" + assert.Equal(t, `version: "1.5" namespace: key: team_b name: Team B diff --git a/internal/storage/fs/snapshot_test.go b/internal/storage/fs/snapshot_test.go index 06aa3485a3..04eb34fbaf 100644 --- a/internal/storage/fs/snapshot_test.go +++ b/internal/storage/fs/snapshot_test.go @@ -651,22 +651,6 @@ func TestSnapshot_GetEvaluationRollouts(t *testing.T) { } } -// evaluationDistTransformer allows us to ensure the contents of the attachment -// is preserved as JSON. We cannot rely on protojson output to be stable -// as it is unstable by design. -func evaluationDistTransformer() cmp.Option { - return cmp.FilterPath(func(p cmp.Path) bool { - return p.String() == "flipt.evaluation.EvaluationDistribution.VariantAttachment" - }, cmp.Transformer("flipt.evaluation.EvaluationDistribution.VariantAttachment", func(a string) map[string]any { - attachment := make(map[string]any) - if err := json.Unmarshal([]byte(a), &attachment); err != nil { - panic(err) - } - - return attachment - })) -} - func TestSnapshot_EvaluationNamespaceSnapshot(t *testing.T) { snap, err := SnapshotFromFS(zaptest.NewLogger(t), DefaultFliptConfig(), testdata) require.NoError(t, err) @@ -826,11 +810,10 @@ func TestSnapshot_EvaluationNamespaceSnapshot(t *testing.T) { } } - // Ignore variant IDs and attachments in comparison since they're generated + // Ignore variant IDs in comparison since they're generated for _, flag := range snapshot.Flags { if flag.DefaultVariant != nil { flag.DefaultVariant.Id = "" - flag.DefaultVariant.Attachment = "" } } @@ -839,7 +822,8 @@ func TestSnapshot_EvaluationNamespaceSnapshot(t *testing.T) { protocmp.SortRepeated(func(a, b *evaluation.EvaluationFlag) bool { return a.Key < b.Key }), - evaluationDistTransformer(), + protocmp.IgnoreFields(&evaluation.EvaluationVariant{}, "attachment"), + protocmp.IgnoreFields(&evaluation.EvaluationDistribution{}, "variant_attachment"), } if !cmp.Equal(tt.wantSnap, snapshot, opts...) { diff --git a/rpc/flipt/auth/request.go b/rpc/flipt/auth/request.go index 7fa648ea63..98ea3d72c3 100644 --- a/rpc/flipt/auth/request.go +++ b/rpc/flipt/auth/request.go @@ -5,7 +5,7 @@ import ( ) func (req *CreateTokenRequest) Request() []flipt.Request { - return []flipt.Request{flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionCreate, flipt.WithSubject(flipt.SubjectToken))} + return []flipt.Request{flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionCreate)} } func (req *ListAuthenticationsRequest) Request() []flipt.Request { diff --git a/rpc/flipt/request.go b/rpc/flipt/request.go index 99a2ca134c..644a7485ce 100644 --- a/rpc/flipt/request.go +++ b/rpc/flipt/request.go @@ -29,16 +29,6 @@ const ( ResourceSegment Resource = "segment" ResourceAuthentication Resource = "authentication" - SubjectConstraint Subject = "constraint" - SubjectDistribution Subject = "distribution" - SubjectFlag Subject = "flag" - SubjectNamespace Subject = "namespace" - SubjectRollout Subject = "rollout" - SubjectRule Subject = "rule" - SubjectSegment Subject = "segment" - SubjectToken Subject = "token" - SubjectVariant Subject = "variant" - ActionCreate Action = "create" ActionDelete Action = "delete" ActionUpdate Action = "update" @@ -51,7 +41,6 @@ const ( type Request struct { Namespace string `json:"namespace,omitempty"` Resource Resource `json:"resource,omitempty"` - Subject Subject `json:"subject,omitempty"` Action Action `json:"action,omitempty"` Status Status `json:"status,omitempty"` } @@ -76,12 +65,6 @@ func WithStatus(s Status) func(*Request) { } } -func WithSubject(s Subject) func(*Request) { - return func(r *Request) { - r.Subject = s - } -} - func NewRequest(r Resource, a Action, opts ...func(*Request)) Request { req := Request{ Resource: r, @@ -97,10 +80,6 @@ func NewRequest(r Resource, a Action, opts ...func(*Request)) Request { return req } -func newFlagScopedRequest(ns string, s Subject, a Action) Request { - return NewRequest(ResourceFlag, a, WithNamespace(ns), WithSubject(s)) -} - func (req *ListFlagRequest) Request() []Request { - return []Request{newFlagScopedRequest(req.NamespaceKey, SubjectFlag, ActionRead)} + return []Request{NewRequest(ResourceFlag, ActionRead, WithNamespace(req.NamespaceKey))} } diff --git a/rpc/v2/environments/request.go b/rpc/v2/environments/request.go index 85e21f8c42..5de9674105 100644 --- a/rpc/v2/environments/request.go +++ b/rpc/v2/environments/request.go @@ -9,7 +9,7 @@ func (r *GetNamespaceRequest) Request() []flipt.Request { } func (r *ListNamespacesRequest) Request() []flipt.Request { - return []flipt.Request{flipt.NewRequest(flipt.ResourceNamespace, flipt.ActionRead)} + return []flipt.Request{flipt.NewRequest(flipt.ResourceNamespace, flipt.ActionRead, flipt.WithNoNamespace())} } func (r *UpdateNamespaceRequest) Request() []flipt.Request { From 2f4d3363c08daba9adf90faed2fef1500192b1df Mon Sep 17 00:00:00 2001 From: Mark Phelps <209477+markphelps@users.noreply.github.com> Date: Tue, 18 Feb 2025 15:53:39 -0500 Subject: [PATCH 2/5] chore: config/schema tests Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --- config/flipt.schema.cue | 22 ------- config/flipt.schema.json | 104 --------------------------------- config/schema_test.go | 5 +- internal/config/config_test.go | 8 --- 4 files changed, 4 insertions(+), 135 deletions(-) diff --git a/config/flipt.schema.cue b/config/flipt.schema.cue index a268116fee..52f42f323a 100644 --- a/config/flipt.schema.cue +++ b/config/flipt.schema.cue @@ -190,28 +190,6 @@ import "list" ca_cert_bytes?: string insecure_skip_tls?: bool | *false credentials?: string - publishers?: { - object?: { - type: "s3" | "azblob" | "googlecloud" | *"" - s3?: { - region: string - bucket: string - prefix?: string - endpoint?: string - poll_interval?: =~#duration | *"1m" - } - azblob?: { - container: string - endpoint?: string - poll_interval?: =~#duration | *"1m" - } - googlecloud?: { - bucket: string - prefix?: string - poll_interval?: =~#duration | *"1m" - } - } - } } #credentials: [string]: { diff --git a/config/flipt.schema.json b/config/flipt.schema.json index ea9288371b..084a81748f 100644 --- a/config/flipt.schema.json +++ b/config/flipt.schema.json @@ -765,110 +765,6 @@ }, "credentials": { "type": "string" - }, - "publishers": { - "type": [ - "object", - "null" - ], - "additionalProperties": false, - "properties": { - "object": { - "type": [ - "object", - "null" - ], - "additionalProperties": false, - "properties": { - "type": { - "type": "string", - "enum": [ - "", - "s3", - "azblob", - "googlecloud" - ], - "default": "" - }, - "s3": { - "type": [ - "object", - "null" - ], - "additionalProperties": false, - "required": [ - "region", - "bucket" - ], - "properties": { - "region": { - "type": "string" - }, - "bucket": { - "type": "string" - }, - "prefix": { - "type": "string" - }, - "endpoint": { - "type": "string" - }, - "poll_interval": { - "type": "string", - "pattern": "^([0-9]+(ns|us|µs|ms|s|m|h))+$", - "default": "1m" - } - } - }, - "azblob": { - "type": [ - "object", - "null" - ], - "additionalProperties": false, - "required": [ - "container" - ], - "properties": { - "container": { - "type": "string" - }, - "endpoint": { - "type": "string" - }, - "poll_interval": { - "type": "string", - "pattern": "^([0-9]+(ns|us|µs|ms|s|m|h))+$", - "default": "1m" - } - } - }, - "googlecloud": { - "type": [ - "object", - "null" - ], - "additionalProperties": false, - "required": [ - "bucket" - ], - "properties": { - "bucket": { - "type": "string" - }, - "prefix": { - "type": "string" - }, - "poll_interval": { - "type": "string", - "pattern": "^([0-9]+(ns|us|µs|ms|s|m|h))+$", - "default": "1m" - } - } - } - } - } - } } } } diff --git a/config/schema_test.go b/config/schema_test.go index a90061de08..971f390f6e 100644 --- a/config/schema_test.go +++ b/config/schema_test.go @@ -72,8 +72,11 @@ func defaultConfig(t *testing.T) (conf map[string]any) { DecodeHook: mapstructure.ComposeDecodeHookFunc(config.DecodeHooks...), Result: &conf, }) + config := config.Default() + // hack to get around validation not being able to handle types that map[string]*struct + config.Storage["default"].PollInterval = 0 require.NoError(t, err) - require.NoError(t, dec.Decode(config.Default())) + require.NoError(t, dec.Decode(config)) // adapt converts instances of time.Duration to their // string representation, which CUE is going to validate diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 1364fb64e9..7f493d9988 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -17,19 +17,11 @@ import ( "github.com/iancoleman/strcase" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/xeipuuv/gojsonschema" "gocloud.dev/blob" "gocloud.dev/blob/memblob" "gopkg.in/yaml.v2" ) -func TestJSONSchema(t *testing.T) { - schemaBytes, err := os.ReadFile("../../config/flipt.schema.json") - require.NoError(t, err) - _, err = gojsonschema.NewSchema(gojsonschema.NewBytesLoader(schemaBytes)) - require.NoError(t, err) -} - func TestScheme(t *testing.T) { tests := []struct { name string From 12a41ad8d37ad567fd43d65736554c880a5d895e Mon Sep 17 00:00:00 2001 From: Mark Phelps <209477+markphelps@users.noreply.github.com> Date: Tue, 18 Feb 2025 16:05:55 -0500 Subject: [PATCH 3/5] chore: fix linter issue Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --- internal/storage/analytics/migrator_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/analytics/migrator_test.go b/internal/storage/analytics/migrator_test.go index a82cdc3d53..aba861e9ae 100644 --- a/internal/storage/analytics/migrator_test.go +++ b/internal/storage/analytics/migrator_test.go @@ -92,6 +92,6 @@ func TestMigratorExpectedVersions(t *testing.T) { require.Equal(t, count%2, 0, "migrations for %s should be even", driver) // we only care about the up migrations not the down migrations actual := count / 2 - assert.Equal(t, uint(actual), expectedVersions[driver], "expectedVersions for %s should be set to %d. you need to increment expectedVersions after adding a new migration", driver, actual) + assert.Equal(t, expectedVersions[driver], uint(actual), "expectedVersions for %s should be set to %d. you need to increment expectedVersions after adding a new migration", driver, actual) } } From 6b3e0eb8d0c47273faf6c4c8f8eed23edf5da253 Mon Sep 17 00:00:00 2001 From: Mark Phelps <209477+markphelps@users.noreply.github.com> Date: Wed, 19 Feb 2025 16:36:09 -0500 Subject: [PATCH 4/5] chore: fix remaining test Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --- internal/server/ofrep/evaluation_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/server/ofrep/evaluation_test.go b/internal/server/ofrep/evaluation_test.go index 673d809194..4f21964d0f 100644 --- a/internal/server/ofrep/evaluation_test.go +++ b/internal/server/ofrep/evaluation_test.go @@ -18,7 +18,7 @@ import ( "github.com/stretchr/testify/assert" "go.flipt.io/flipt/internal/common" "go.flipt.io/flipt/internal/storage" - "go.flipt.io/flipt/rpc/flipt" + "go.flipt.io/flipt/rpc/flipt/core" rpcevaluation "go.flipt.io/flipt/rpc/flipt/evaluation" "go.flipt.io/flipt/rpc/flipt/ofrep" "go.uber.org/zap/zaptest" @@ -224,10 +224,10 @@ func TestEvaluateBulkSuccess(t *testing.T) { }, nil) store.On("ListFlags", mock.Anything, storage.ListWithOptions(storage.NewNamespace("default"))).Return( - storage.ResultSet[*flipt.Flag]{ - Results: []*flipt.Flag{ - {Key: flagKey, Type: flipt.FlagType_VARIANT_FLAG_TYPE, Enabled: true}, - {Key: "disabled", Type: flipt.FlagType_VARIANT_FLAG_TYPE}, + storage.ResultSet[*core.Flag]{ + Results: []*core.Flag{ + {Key: flagKey, Type: core.FlagType_VARIANT_FLAG_TYPE, Enabled: true}, + {Key: "disabled", Type: core.FlagType_VARIANT_FLAG_TYPE}, }, NextPageToken: "YmFy", }, nil).Once() @@ -246,7 +246,7 @@ func TestEvaluateBulkSuccess(t *testing.T) { t.Run("without flags in the evaluation request failed fetch the flags", func(t *testing.T) { store.On("ListFlags", mock.Anything, storage.ListWithOptions(storage.NewNamespace("default"))).Return( - storage.ResultSet[*flipt.Flag]{ + storage.ResultSet[*core.Flag]{ Results: nil, NextPageToken: "", }, errors.New("failed to fetch flags")).Once() From 1756d12affe8040cc9e22efcb9625c0af1cfb9a0 Mon Sep 17 00:00:00 2001 From: Mark Phelps <209477+markphelps@users.noreply.github.com> Date: Wed, 19 Feb 2025 16:48:26 -0500 Subject: [PATCH 5/5] chore: fix lint issue Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --- internal/storage/analytics/migrator_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/analytics/migrator_test.go b/internal/storage/analytics/migrator_test.go index aba861e9ae..7e85d11996 100644 --- a/internal/storage/analytics/migrator_test.go +++ b/internal/storage/analytics/migrator_test.go @@ -89,7 +89,7 @@ func TestMigratorExpectedVersions(t *testing.T) { count := len(migrations) require.Positive(t, count, "no migrations found for %s", driver) - require.Equal(t, count%2, 0, "migrations for %s should be even", driver) + require.Equal(t, 0, count%2, "migrations for %s should be even", driver) // we only care about the up migrations not the down migrations actual := count / 2 assert.Equal(t, expectedVersions[driver], uint(actual), "expectedVersions for %s should be set to %d. you need to increment expectedVersions after adding a new migration", driver, actual)