Skip to content

Commit

Permalink
[BREAKING] DefaultCheck now accepts a context.Context
Browse files Browse the repository at this point in the history
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.

Rational for the change:

Ideally, most users will not need to implement check. By design, not implementing
`CustomCheck` has the same behavior as implementing check with
`return infer.DefaultCheck(...)`. If users do implement `CustomCheck`, then we don't want
to make any assumptions about what behavior they want. This includes not applying defaults
and not injecting secrets.
  • Loading branch information
iwahbe committed Jul 27, 2024
1 parent 73b74eb commit 1807678
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 45 deletions.
2 changes: 1 addition & 1 deletion examples/auto-naming/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion examples/file/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion infer/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
```

Expand Down
8 changes: 6 additions & 2 deletions infer/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,21 @@ func (c *config[T]) checkConfig(ctx context.Context, req p.CheckRequest) (p.Chec
if req.Urn != "" {
name = req.Urn.Name()
}
i, failures, err := t.Check(ctx, name, req.Olds, req.News)
defCheckEnc, i, failures, err := callCustomCheck(ctx, t, name, req.Olds, req.News)
if err != nil {
return p.CheckResponse{}, err
}

if defCheckEnc != nil {
encoder = *defCheckEnc
}

inputs, err := encoder.Encode(i)
if err != nil {
return p.CheckResponse{}, err
}
return p.CheckResponse{
Inputs: applySecrets[T](inputs),
Inputs: inputs,
Failures: failures,
}, nil
}
Expand Down
6 changes: 6 additions & 0 deletions infer/internal/ende/ende.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ const AssetSignature = "a9e28acb8ab501f883219e7c9f624fb6"
// ArchiveSignature is a unique key for use for archives in the AssetOrArchive union type.
const ArchiveSignature = "195f3948f6769324d4661e1e245f3a4d"

// Encoder holds a look-aside table of information that can be encoded into a
// [resource.PropertyMap] but cannot be encoded into a plain Go struct.
//
// Encoder is a byproduct of [Decode], and a non-zero Encoder should be used whenever
// possible. If it is not possible to derive an Encoder, it safe to use the zero value of
// Encoder.
type Encoder struct{ *ende }

// Decode a property map to a `pulumi:"x"` annotated struct.
Expand Down
51 changes: 41 additions & 10 deletions infer/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,9 @@ type CustomCreate[I, O any] interface {
// actually happens.
type CustomCheck[I any] interface {
// Maybe oldInputs can be of type I
Check(ctx context.Context, name string, oldInputs resource.PropertyMap, newInputs resource.PropertyMap) (
I, []p.CheckFailure, error)
Check(
ctx context.Context, name string, oldInputs, newInputs resource.PropertyMap,
) (I, []p.CheckFailure, error)
}

// CustomDiff describes a resource that understands how to diff itself given a new set of
Expand Down Expand Up @@ -880,12 +881,17 @@ 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].
i, failures, err := r.Check(ctx, req.Urn.Name(), req.Olds, req.News)
// We do not apply defaults or secrets if the user has implemented Check
// themselves. Defaults and secrets are applied by [DefaultCheck].

defCheckEnc, i, failures, err := callCustomCheck(ctx, r, req.Urn.Name(), req.Olds, req.News)
if err != nil {
return p.CheckResponse{}, err
}
if defCheckEnc != nil {
encoder = *defCheckEnc
}

inputs, err := encoder.Encode(i)
return p.CheckResponse{
Inputs: inputs,
Expand All @@ -902,12 +908,37 @@ 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) {
// This (key,value) pair provide a mechanism for [DefaultCheck] to silently return the
// encoder it derives.
type (
defaultCheckEncoderKey struct{}
defaultCheckEncoderValue struct{ enc *ende.Encoder }
)

// callCustomCheck should be used to call [CustomCheck.Check].
//
// callCustomCheck facilitates extracting the encoder created with [DefaultCheck].
func callCustomCheck[T any](
ctx context.Context, r CustomCheck[T], name string, olds, news resource.PropertyMap,
) (*ende.Encoder, T, []p.CheckFailure, error) {
defaultCheckEncoder := new(defaultCheckEncoderValue)
ctx = context.WithValue(ctx, defaultCheckEncoderKey{}, defaultCheckEncoder)
i, failures, err := r.Check(ctx, name, olds, news)
return defaultCheckEncoder.enc, i, failures, 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](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
}
Expand Down
2 changes: 1 addition & 1 deletion infer/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
85 changes: 56 additions & 29 deletions tests/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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)
}

0 comments on commit 1807678

Please sign in to comment.