Infer additional secret properties in engine, from schemata #16187
Conversation
Changelog[uncommitted] (2024-05-15)Features
|
lunaris
force-pushed
the
wjones/engine-schema
branch
from
38aa4af
to
a714606
Compare
| Args: args, | ||
| Target: target, | ||
| }, defaultProviderVersions, dryRun), nil | ||
| schemaLoader := schema.NewPluginLoader(plugctx.Host) |
There was a problem hiding this comment.
Does this do any caching or do we reload the schema each time we get a resource for it?
lunaris
force-pushed
the
wjones/engine-schema
branch
3 times, most recently
from
1bf571a
to
fff22a6
Compare
pkg/resource/deploy/source_eval.go
Outdated
| var additionalSecretOutputs []string | ||
| if additionalSecretOutputsSet.Cardinality() > 0 { | ||
| additionalSecretOutputs = additionalSecretOutputsSet.ToSlice() | ||
| } else { | ||
| additionalSecretOutputs = nil | ||
| } |
There was a problem hiding this comment.
Does this not work?
| var additionalSecretOutputs []string | |
| if additionalSecretOutputsSet.Cardinality() > 0 { | |
| additionalSecretOutputs = additionalSecretOutputsSet.ToSlice() | |
| } else { | |
| additionalSecretOutputs = nil | |
| } | |
| additionalSecretOutputs = additionalSecretOutputsSet.ToSlice() |
There was a problem hiding this comment.
Alas no, because that will return an empty slice for nil, which is not the current behaviour (nil stays as nil). I'd like to believe that semantically it will make no difference, but for "perfect" backwards compatibility I've opted for this. Happy to go either way though.
pkg/resource/deploy/source_eval.go
Outdated
| //if parsedVersion, err := semver.Parse(req.GetVersion()); err == nil { | ||
| // version = &parsedVersion | ||
| //} | ||
|
|
||
| //if typeToken, err := tokens.ParseTypeToken(req.Type); err == nil { | ||
| // packageName := typeToken.Package().String() | ||
| // if pkgReference, err := rm.schemaLoader.LoadPackageReference(packageName, version); err == nil { | ||
| // if resourceSchema, found, err := pkgReference.Resources().Get(req.Type); err == nil && found { | ||
| // for _, outputProperty := range resourceSchema.Properties { | ||
| // if outputProperty.Secret { | ||
| // additionalSecretOutputsSet.Add(outputProperty.Name) | ||
| // } | ||
| // } | ||
| // } | ||
| // } | ||
| //} |
There was a problem hiding this comment.
We are making the schema load bearing. Going between having and not having a schema will start to cause diffs on some providers, since [secret] => "foo" is a change that should show up in state.
I would be very careful to distinguish between tolerable errors (there is no schema for this resource, technically OK) and non-tolerable errors (we tried to load the schema, but failed to launch), correctly erroring for non-tolerable errors.
We should log for tolerable errors.
There was a problem hiding this comment.
A) This shouldn't actually cause diffs because SDK-gen should have been setting the same things secret anyway
B) A provider returning an empty schema is fine and we should tolerate that and just do nothing. I think anything else is an error, I don't expect any providers to error on this method or return invalid schemas.
There was a problem hiding this comment.
A) is true only as long as the generated SDK agrees with the provider. This may not hold now for Go, since they provider may be a different version then the SDK used to consume it.
B) Yes
There was a problem hiding this comment.
A) True, but I'd consider that already buggy and the diff would be more correct. I don't think we should try to avoid that.
This commit modifies calls to `RegisterResource` so that they load resource schemata, enabling the engine to modify resource properties according to the schema instead of the caller (e.g. the SDK). As part of this commit, the engine will now extend `additionalSecretOutputs` options with `Secret` properties it finds in the schema. In a later commit, this will allow us to stop generating SDK code to set these properties correctly client-side. The schema may be used in subsequent pieces of work to handle other concerns, such as defaults, though this is not tackled in this commit.
lunaris
force-pushed
the
wjones/engine-schema
branch
from
fff22a6
to
21db4f3
Compare
This commit modifies calls to
RegisterResourceso that they load resource schemata, enabling the engine to modify resource properties according to the schema instead of the caller (e.g. the SDK). As part of this commit, the engine will now extendadditionalSecretOutputsoptions withSecretproperties it finds in the schema. In a later commit, this will allow us to stop generating SDK code to set these properties correctly client-side. The schema may be used in subsequent pieces of work to handle other concerns, such as defaults, though this is not tackled in this commit.Note: This work is almost entirely aped from master...zaid/additional-secret-properties-from-schema as preparation for looking at how we handle other things (names, defaulting, etc.) in engine.