Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Policy remediations SDK support #314

Merged
merged 30 commits into from
Oct 10, 2023
Merged

Policy remediations SDK support #314

merged 30 commits into from
Oct 10, 2023

Conversation

joeduffy
Copy link
Member

@joeduffy joeduffy commented Oct 1, 2023

This PR implements the TypeScript and Python policy SDK side of support for the new policy remediations feature. This lets developers implement new remediations callbacks, similar to policies, in their policy packs. See pulumi/pulumi#14080 for the engine/CLI support, which is a prerequisite for this working.

Note: This change depends on pulumi/pulumi#14080 and certain tests will fail until released.

This renames transforms to remediations in the Node.js SDK. It also
accomplishes the new design where a single policy can define both
logic to validate policy conformance *and* to remediate, based on
the enforcement level chosen. Python equivalent incoming.
This renames transforms to remediations, while also adopting the
new style of expressing remediations as an attribute of a policy.
@joeduffy joeduffy requested a review from justinvp October 6, 2023 02:07
@joeduffy
Copy link
Member Author

joeduffy commented Oct 6, 2023

@justinvp This is fairly close. Probably good time to take an early look. I am mostly writing tests at this point.

sdk/nodejs/policy/policy.ts Outdated Show resolved Hide resolved
sdk/nodejs/policy/policy.ts Show resolved Hide resolved
sdk/nodejs/policy/policy.ts Show resolved Hide resolved
sdk/python/lib/pulumi_policy/policy.py Outdated Show resolved Hide resolved
sdk/python/lib/pulumi_policy/serialize.py Outdated Show resolved Hide resolved
sdk/python/lib/pulumi_policy/serialize.py Outdated Show resolved Hide resolved
sdk/python/lib/pulumi_policy/serialize.py Outdated Show resolved Hide resolved
sdk/nodejs/policy/server.ts Show resolved Hide resolved
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we going to do about secrets? I haven’t tried it, but it looks like we’re going to lose those in a remediation, because when we deserialize the incoming properties here in the policy SDK, we strip the secretness of values, and then when we return the resulting properties, any secrets that were present will no longer be present going out. Perhaps we need a proxy object that can keep track of secrets?

Separate from that, we may also want to provide a way for a remediation to introduce a new value that’s a secret. Maybe we could allow users to use pulumi.secret/Output.secret from the core SDKs, and update the serialization code here to support serializing such outputs?

@joeduffy
Copy link
Member Author

joeduffy commented Oct 6, 2023

What are we going to do about secrets?

That is one of the few things not done just yet, but that I'll be completing over the weekend. I called out the approach in the design doc -- see "Dealing with Secrets". The idea was to automatically re-apply secret-ness to anything that was a secret prior to calling the remediation. This feels like it is the least surprising and does the right thing by default.

I had not considered a new proxy that can keep track of secrets ... That may be another approach to accomplish the same, although we'd need a change at the gRPC layer to send the secrets over to the analyzer instead of plaintexting them.

The nice thing about the engine doing this is a remediation can in fact just remediate a property like usual, even if it is secret, and the engine will just re-apply the secret-ness back on the engine side of the call.

That said, I do like your idea of being about to use pulumi.secret / Output.secret in addition to whatever we do here.

What do you think about this approach?

@justinvp
Copy link
Member

justinvp commented Oct 7, 2023

What do you think about this approach?

Yeah, I had that in mind as well. I think it's reasonable. One thing I'm curious about: Will deeply nested secrets be preserved or will the entire top-level property be made a secret when re-applying secret-ness back on the engine side of the call if the property had originally contained a secret? The reason I ask is because users are often frustrated when a secret in a nested array or map ends up "tainting" the entire property as a secret (because it can make diffs difficult to reason about). They don't want to see the whole property being a secret, they want the one secret value nested within a array/map to show as a secret, and not any other items that aren't secrets.

although we'd need a change at the gRPC layer to send the secrets over to the analyzer instead of plaintexting them

Note: we already keep secrets when sending properties to the analyzer, and it looks like the current implementation in your PR does for Remediate as well:

https://github.com/pulumi/pulumi/blob/daaaaea339fc9675b1fec1d9893791362ade7da9/sdk/go/common/resource/plugin/analyzer_plugin.go#L176

https://github.com/pulumi/pulumi/pull/14080/files#diff-dd48d49dec6689ee06eaea36251a5881a54353583e2a6e9e88498f8fdf38c424R289-R290

They're stripped in the deserialization code in the policy SDK:

case specialSecretSig:
return deserializeProperty(prop["value"]);

if sig == SPECIAL_SECRET_SIG:
if "value" not in prop:
raise AssertionError("Invalid secret encountered when unmarshaling resource property")
return _deserialize_property(prop["value"])

This change preserves secrets round-tripping through policy
remediations. This is the Node.js side of the change, and Python
will come shortly. This works by preserving the secret nature of
secrets as they are read from property state. All values are
proxied, however, so that they can still be used as plain values.
The proxying will trap all writes to objects and ensure that
secretness is preserved on all properties that were secret on their
way in to the remediation. The serialization code then detects and
preserves secret values as state is returned from a remediation.
@joeduffy
Copy link
Member Author

joeduffy commented Oct 7, 2023

The proxying approach worked quite well and it's easier to make it perfectly granular. I just pushed the changes. I explored just using pulumi.secret to represent the fact that properties are runtime secrets, which would have been nice because it would enable remediations to return pulumi.secrets explicitly to introduce new secrets into the state. Unfortunately, secrets are all wrapped up in the output machinery which entails promises, and this makes it impossible to promptly proxy values because even if we reached into its innards, we'd need to await something. It's possible we could do something dirty like monkey patch the pulumi.secret function to avoid all of that work in a way that remediations can recognize.

For now, I'll just go with the current approach as it at least does the job of preserving secrets. Feedback welcome!

@joeduffy joeduffy marked this pull request as ready for review October 8, 2023 15:20
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sdk/nodejs/policy/secret.ts Outdated Show resolved Hide resolved
sdk/nodejs/policy/tests/util.ts Show resolved Hide resolved
sdk/python/lib/pulumi_policy/secret.py Outdated Show resolved Hide resolved
tests/integration/resource_options/program/resource.ts Outdated Show resolved Hide resolved
joeduffy and others added 8 commits October 9, 2023 10:59
@joeduffy joeduffy merged commit 086a994 into master Oct 10, 2023
@joeduffy joeduffy deleted the joeduffy/policy_transforms branch October 10, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants