Wire policy evaluation into Terraform core runtime#38516
Conversation
ac6ebf9 to
fb1a082
Compare
c3a1b4b to
f309492
Compare
fabac57 to
7f791cd
Compare
e691a08 to
a6d3871
Compare
35ef0a8 to
def6dde
Compare
82875dc to
d4ca814
Compare
SarahFrench
left a comment
There was a problem hiding this comment.
I'm still looking through this PR, and it's a lot easier after the meeting we had yesterday (thank you!). I'll continue looking but I figured I'd share an initial round of comments.
|
The equivalence tests failed. Please investigate here. |
2676716 to
104673a
Compare
Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
104673a to
1e97ca1
Compare
SarahFrench
left a comment
There was a problem hiding this comment.
I'm still looking through, but here are some more comments 📝
2e848ac to
acbabc3
Compare
SarahFrench
left a comment
There was a problem hiding this comment.
Approved - I've looked through all the changes and have no further questions or feedback.
austinvalle
left a comment
There was a problem hiding this comment.
Apologies for the late review 😅 , left a few comments/questions
| addrs := s.state.allResourceInstanceObjectAddrs(func(objAddr addrs.AbsResourceInstanceObject) bool { | ||
| return objAddr.ResourceInstance.ConfigResource().Equal(addr) | ||
| }) | ||
| s.lock.RUnlock() |
There was a problem hiding this comment.
Was this unlock meant to be deferred? Looks like we're accessing s.state after the mutex been unlocked 👀
| if policyGraph := ctx.PolicyGraph(); policyGraph != nil && !n.preDestroyRefresh { | ||
| policyGraph.Add(policyNodeFromChange(change)) | ||
| } |
There was a problem hiding this comment.
Are we expecting that data sources should also have a policy node added? It looks like they also use this writeChange method to plan 👀
There was a problem hiding this comment.
Good catch. This never caused a problem because data source changes are denoted as Read, so we never send them for policy evaluation. However it does make sense to not have a node for them in the first place
| default: | ||
| return nil |
There was a problem hiding this comment.
nit: Is this switch statement meant to be exhaustive? If not, a log mentioning why we're skipping policy evaluation could be helpful for future debuggers 👀
There was a problem hiding this comment.
It isn't exhaustive, as there are other actions that we do not care about. Adding a log here.
| if modCfg == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: Is this expected? Feels like it should either be a panic (internal implementation error) or a log (expected skip of policy evaluation) 🤔
| readResp := provider.ReadDataSource(providers.ReadDataSourceRequest{ | ||
| TypeName: target, | ||
| Config: configVal, | ||
| ProviderMeta: meta, |
There was a problem hiding this comment.
Very interesting 👀 , was it specifically requested that we add provider meta here? I'm not 100% sure how it's being used today by the providers, but I believe the usage of it was more analytics then functional behavior of ReadDataSource (I also vaguely remember it being related to module usage, but here the context is the resource policy is using it, not the actual configuration... not sure if the distinction is important 🤔 )
There was a problem hiding this comment.
Yeah, meta only has to do with tracking module usage. If this config isn't coming directly from module usage, this probably shouldn't be used here.
There was a problem hiding this comment.
Right.. I dont have much knowledge about the origins of that object in the request. However, this data source evaluation uses the same provider as the module of the current resource whose policy is being evaluated. So I am not sure it would be a bad idea when we send this meta and the provider translates it as part of the module usage.
| func getDataSourceForPolicyCallback(ctx EvalContext, provider providers.Interface, schema providers.GetProviderSchemaResponse, meta cty.Value) func(datasource string, attrs cty.Value) (cty.Value, error) { | ||
| return func(target string, attrs cty.Value) (cty.Value, error) { | ||
| if datasource, ok := schema.DataSources[target]; ok { | ||
| configVal, err := datasource.Body.CoerceValue(attrs) |
There was a problem hiding this comment.
Why would the config require CoerceValue? It looks like it's patching over a typing bug elsewhere.
There was a problem hiding this comment.
The config here does not come from a parsed terraform configuration. It comes from the policy engine, which means it has not been validated, and is merely denoted as an object thus far. CoerceValue would allow us to validate it against the schema.
This is part of a stacked series to upstream the policy work in smaller, reviewable pieces:
This PR wires policy evaluation into Terraform core itself. It adds the runtime behavior for evaluating policy during graph execution, passes the relevant metadata and values into the policy engine, and records the resulting policy outcomes back onto the run so later layers can surface them.
This is the core of the stack. The main thing happening here is not CLI behavior yet, but rather defining where policy runs in the Terraform runtime, what objects get evaluated, and how those results are tracked.
Included here
Target Release
1.16.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry