From 7f237dc04a27019639e87ad327cea71ada5e33c4 Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Wed, 24 Jul 2024 12:13:27 -0700 Subject: [PATCH] [BREAKING] `DefaultCheck` now accepts a `context.Context` Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets application. The new signature for `DefaultCheck` is easier to extend without additional breaking changes. The downside to this design is that this makes `DefaultCheck` special. I think it's worth living with that until #212 is resolved. I'd like to merge shortly after #252 (and before #252 is released) so that user's don't rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not called. --- examples/auto-naming/main.go | 2 +- examples/file/main.go | 2 +- infer/README.md | 2 +- infer/configuration.go | 8 +++- infer/resource.go | 31 ++++++++++--- infer/resource_test.go | 2 +- tests/config_test.go | 85 ++++++++++++++++++++++++------------ 7 files changed, 91 insertions(+), 41 deletions(-) diff --git a/examples/auto-naming/main.go b/examples/auto-naming/main.go index b4ee1dd..9f30ff3 100644 --- a/examples/auto-naming/main.go +++ b/examples/auto-naming/main.go @@ -47,7 +47,7 @@ func (*User) Check( ctx context.Context, name string, oldInputs, newInputs resource.PropertyMap, ) (UserArgs, []p.CheckFailure, error) { // Apply default arguments - args, failures, err := infer.DefaultCheck[UserArgs](newInputs) + args, failures, err := infer.DefaultCheck[UserArgs](ctx, newInputs) if err != nil { return args, failures, err } diff --git a/examples/file/main.go b/examples/file/main.go index 658973a..208f5da 100644 --- a/examples/file/main.go +++ b/examples/file/main.go @@ -112,7 +112,7 @@ func (*File) Check(ctx context.Context, name string, oldInputs, newInputs resour if _, ok := newInputs["path"]; !ok { newInputs["path"] = resource.NewStringProperty(name) } - return infer.DefaultCheck[FileArgs](newInputs) + return infer.DefaultCheck[FileArgs](ctx, newInputs) } func (*File) Update(ctx context.Context, id string, olds FileState, news FileArgs, preview bool) (FileState, error) { diff --git a/infer/README.md b/infer/README.md index 18cee39..2fec6c9 100644 --- a/infer/README.md +++ b/infer/README.md @@ -231,7 +231,7 @@ func (*File) Check(ctx context.Context, name string, oldInputs, newInputs resour if _, ok := newInputs["path"]; !ok { newInputs["path"] = resource.NewStringProperty(name) } - return infer.DefaultCheck[FileArgs](newInputs) + return infer.DefaultCheck[FileArgs](ctx, newInputs) } ``` diff --git a/infer/configuration.go b/infer/configuration.go index b99849f..5fdbd73 100644 --- a/infer/configuration.go +++ b/infer/configuration.go @@ -93,17 +93,23 @@ func (c *config[T]) checkConfig(ctx context.Context, req p.CheckRequest) (p.Chec if req.Urn != "" { name = req.Urn.Name() } + defaultCheckEncoder := new(defaultCheckEncoderValue) + ctx = context.WithValue(ctx, defaultCheckEncoderKey{}, defaultCheckEncoder) i, failures, err := t.Check(ctx, name, req.Olds, req.News) if err != nil { return p.CheckResponse{}, err } + if defaultCheckEncoder.enc != nil { + encoder = *defaultCheckEncoder.enc + } + inputs, err := encoder.Encode(i) if err != nil { return p.CheckResponse{}, err } return p.CheckResponse{ - Inputs: applySecrets[T](inputs), + Inputs: inputs, Failures: failures, }, nil } diff --git a/infer/resource.go b/infer/resource.go index 372d20b..4f5b64f 100644 --- a/infer/resource.go +++ b/infer/resource.go @@ -880,12 +880,18 @@ func (rc *derivedResourceController[R, I, O]) Check(ctx context.Context, req p.C if r, ok := ((interface{})(r)).(CustomCheck[I]); ok { // The user implemented check manually, so call that. // - // We do not apply defaults if the user has implemented Check - // themselves. Defaults are applied by [DefaultCheck]. + // We do not apply defaults or secrets if the user has implemented Check + // themselves. Defaults and secrets are applied by [DefaultCheck]. + defaultCheckEncoder := new(defaultCheckEncoderValue) + ctx = context.WithValue(ctx, defaultCheckEncoderKey{}, defaultCheckEncoder) i, failures, err := r.Check(ctx, req.Urn.Name(), req.Olds, req.News) if err != nil { return p.CheckResponse{}, err } + if defaultCheckEncoder.enc != nil { + encoder = *defaultCheckEncoder.enc + } + inputs, err := encoder.Encode(i) return p.CheckResponse{ Inputs: inputs, @@ -902,12 +908,23 @@ func (rc *derivedResourceController[R, I, O]) Check(ctx context.Context, req p.C return p.CheckResponse{Inputs: applySecrets[I](inputs)}, err } -// DefaultCheck verifies that `inputs` can deserialize cleanly into `I`. This is the default -// validation that is performed when leaving `Check` unimplemented. -// It also adds defaults to `inputs` as necessary, as defined by `Annotator.SetDefault“. -func DefaultCheck[I any](inputs resource.PropertyMap) (I, []p.CheckFailure, error) { +type ( + defaultCheckEncoderKey struct{} + defaultCheckEncoderValue struct{ enc *ende.Encoder } +) + +// DefaultCheck verifies that inputs can deserialize cleanly into I. This is the default +// validation that is performed when leaving Check unimplemented. +// +// It also adds defaults to inputs as necessary, as defined by [Annotator.SetDefault]. +func DefaultCheck[I any](ctx context.Context, inputs resource.PropertyMap) (I, []p.CheckFailure, error) { inputs = applySecrets[I](inputs) - _, i, failures, err := decodeCheckingMapErrors[I](inputs) + enc, i, failures, err := decodeCheckingMapErrors[I](inputs) + + if v, ok := ctx.Value(defaultCheckEncoderKey{}).(*defaultCheckEncoderValue); ok { + v.enc = &enc + } + if err != nil || len(failures) > 0 { return i, failures, err } diff --git a/infer/resource_test.go b/infer/resource_test.go index aeab8a3..af1af75 100644 --- a/infer/resource_test.go +++ b/infer/resource_test.go @@ -528,7 +528,7 @@ func TestCheck(t *testing.T) { t.Run("DefaultCheck "+tcName, func(t *testing.T) { t.Parallel() - in, failures, err := DefaultCheck[checkResource](tc.input.Copy()) + in, failures, err := DefaultCheck[checkResource](context.Background(), tc.input.Copy()) require.NoError(t, err) assert.Empty(t, failures) assert.Equal(t, tc.expected, in.P1) diff --git a/tests/config_test.go b/tests/config_test.go index b126171..5ed0eb6 100644 --- a/tests/config_test.go +++ b/tests/config_test.go @@ -149,12 +149,9 @@ func TestInferCheckConfigSecrets(t *testing.T) { } type config struct { - Field string `pulumi:"field" provider:"secret"` - Nested struct { - Int int `pulumi:"int" provider:"secret"` - NotSecret string `pulumi:"not-nested"` - } `pulumi:"nested"` - NotSecret string `pulumi:"not"` + Field string `pulumi:"field" provider:"secret"` + NotSecret string `pulumi:"not"` + ApplyDefaults bool `pulumi:"applyDefaults,optional"` } var _ infer.CustomCheck[*config] = &config{} @@ -166,34 +163,64 @@ func (c *config) Check( return c, nil, fmt.Errorf("found secrets") } - d, f, err := infer.DefaultCheck[config](newInputs) - return &d, f, err + if v, ok := newInputs["applyDefaults"]; ok && v.IsBool() && v.BoolValue() { + d, f, err := infer.DefaultCheck[config](ctx, newInputs) + *c = d + return &d, f, err + } + + // No defaults, so apply manually + if v := newInputs["field"]; v.IsString() { + c.Field = v.StringValue() + } + if v := newInputs["not"]; v.IsString() { + c.NotSecret = v.StringValue() + } + if v := newInputs["apply-defaults"]; v.IsBool() { + c.ApplyDefaults = v.BoolValue() + } + return c, nil, nil } func TestInferCustomCheckConfig(t *testing.T) { t.Parallel() - resp, err := integration.NewServer("test", semver.MustParse("0.0.0"), infer.Provider(infer.Options{ + s := integration.NewServer("test", semver.MustParse("0.0.0"), infer.Provider(infer.Options{ Config: infer.Config[*config](), - })).CheckConfig(p.CheckRequest{ - Urn: resource.CreateURN("p", "pulumi:providers:test", "", "test", "dev"), - News: resource.PropertyMap{ - "field": resource.NewProperty("value"), - "nested": resource.NewProperty(resource.PropertyMap{ - "int": resource.NewProperty(1.0), - "not-nested": resource.NewProperty("not-secret"), - }), - "not": resource.NewProperty("not-secret"), - }, + })) + + t.Run("with-default-check", func(t *testing.T) { + resp, err := s.CheckConfig(p.CheckRequest{ + Urn: resource.CreateURN("p", "pulumi:providers:test", "", "test", "dev"), + News: resource.PropertyMap{ + "field": resource.NewProperty("value"), + "not": resource.NewProperty("not-secret"), + "applyDefaults": resource.NewProperty(true), + }, + }) + require.NoError(t, err) + require.Empty(t, resp.Failures) + assert.Equal(t, resource.PropertyMap{ + "field": resource.MakeSecret(resource.NewProperty("value")), + "not": resource.NewProperty("not-secret"), + "applyDefaults": resource.NewProperty(true), + }, resp.Inputs) + }) + + t.Run("without-default-check", func(t *testing.T) { + resp, err := s.CheckConfig(p.CheckRequest{ + News: resource.PropertyMap{ + "field": resource.NewProperty("value"), + "not": resource.NewProperty("not-secret"), + "applyDefaults": resource.NewProperty(false), + }, + }) + require.NoError(t, err) + require.Empty(t, resp.Failures) + assert.Equal(t, resource.PropertyMap{ + "field": resource.NewProperty("value"), + "not": resource.NewProperty("not-secret"), + "applyDefaults": resource.NewProperty(false), + }, resp.Inputs) }) - require.NoError(t, err) - require.Empty(t, resp.Failures) - assert.Equal(t, resource.PropertyMap{ - "field": resource.MakeSecret(resource.NewProperty("value")), - "nested": resource.NewProperty(resource.PropertyMap{ - "int": resource.MakeSecret(resource.NewProperty(1.0)), - "not-nested": resource.NewProperty("not-secret"), - }), - "not": resource.NewProperty("not-secret"), - }, resp.Inputs) }