Report violation when accessing unknowns during stack validation

[justinvp]

For resource validations, an advisory violation is reported when accessing unknown values. We weren't doing this for stack validations.

This change fixes it for stack validations, and adds an integration test that ensures we're doing it for both resource and stack validations.

Fixes #178

[chrsmith]

So the PR seems reasonable enough, but is this the actual behavior we want? Let me see if I understand what's going on here:

• I write a resource or stack policy that is perfect and wonderful, and does everything I want it to.
• I then write a Pulumi program where one of a resource's input properties is another resource's output property. (So the underlying type would be Computed<T>, and therefore an "unknown", right?)
• When I run the same resource/stack policy on this Pulumi program, I now get these advisory warnings. Because the policy can't actually check the resource's settings, because it isn't actually knowable for previews.

Do I have that all right? If so, to be clear, this means:

• Any resource policy will now emit these advisory warnings when any of its specific fields that are accessed during the policy rule are unknown. (e.g. in previews.)

Assuming I have that all right, here's the thing that doesn't quite sit right with me. (But let me know if I'm missing something, or this is already settled, etc.) I assumed that we'd have some knob somewhere that was the equivalent of "treat warnings as errors". e.g. a way to ensure that any policy violations -- even advisory ones -- lead to an update failure. (As a way some administrator could try to ensure a dev team is writing the highest quality code.)

If we emit these advisory violations, then such a configuration setting doesn't make sense. Right? And that's just something we need to live with, since the alternative is to silently ignore/skip these rules that access unknown values. And that's probably much worse.

[justinvp]

Assuming I have that all right, here's the thing that doesn't quite sit right with me.

@chrsmith, I am glad you raised this, because the current behavior doesn't sit right with me either. This PR makes stack validations behave the same as resource validations, but I was not part of the original design for it always resulting in an advisory violation. I had the same thoughts about, "but what if I actually want to treat these as mandatory violations (i.e. "treat warnings as errors")?"

Instead of always emitting these as advisory, would it make more sense to always emit them as the configured enforcement level for the policy? So if my policy is configured to be mandatory, they would be emitted as mandatory as well?

[justinvp]

@ekrengel, do you have any context on the current behavior?

[chrsmith]

Happy to defer to you and Erin on the design here, but here's my opinion:

Instead of always emitting these as advisory, would it make more sense to always emit them as the configured enforcement level for the policy?

I don't think that's the right way to go. Instead, I think we'd have some other error level like "information" or "warning" and use that.

Because this particular class of violation -- "accessing an output/computed/unknown value" -- isn't really a policy violation at all. It's just an indication that the policy you want to run couldn't. And unfortunately, there isn't anything we can do about it.

If we were to emit a "mandatory" violation, and fail the preview in these cases, the end user would be unable to use PAC. Since it could just happen that resource X depends on the output of resource Y. And so it would be lame for that particular stack to be prevented from updating because of the organization's policy group settings. (Since it isn't that there is a problem with the stack, just a lack of confirmation.)

Does that make sense?

The thing I personally would like to avoid is having these unpreventable policy violations essentially prevent people from being able to use PAC in for everyday use. And so while it's important we call these situations out, I think we should do so in a way that doesn't cause any undo friction. (Though perhaps we could offer a "treat-info/warning-violations-as-errors" setting too... so that we would prevent stack previews if they ever needed to access an output property, etc.)

[justinvp]

I don't think that's the right way to go. Instead, I think we'd have some other error level like "information" or "warning" and use that.

This makes sense and I agree. Though, I am inclined to still move forward with this PR to make validateStack behave the same as validateResource, and then open a separate issue to track changing both to using some other type of info/warning level.

[ekrengel]

Though, I am inclined to still move forward with this PR to make validateStack behave the same as validateResource, and then open a separate issue to track changing both to using some other type of info/warning level.

I would agree this would be the correct was to proceed.

I was not super involved in the discussions / decisions of what behavior to take when accessing outputs BUT I believe @lukehoban and @pgavlin were.

[justinvp]

Opened #181 to track changing the behavior here