API changes

[justinvp]

Apologies for the long description. This PR implements the latest API proposal, addressing some feedback since we discussed earlier this week. Please read it over and take a look at the changes in this PR and the associated PR that updates the examples pulumi/examples#441

Some changes since we discussed earlier this week:

• This demonstrates a validateTypedResource helper for stronger typed resource checking, similar to the previous typedRule helper.

  Here's an example:

  new PolicyPack("policy-pack-typescript", {
      policies: [{
          name: "s3-no-public-read",
          description: "Prohibits setting the publicRead or publicReadWrite permission on AWS S3 buckets.",
          enforcementLevel: "mandatory",
          validateResource: validateTypedResource(aws.s3.Bucket.isInstance, (it, args, reportViolation) => {
              if (it.acl === "public-read" || it.acl === "public-read-write") {
                  reportViolation(
                      "You cannot set public-read or public-read-write on an S3 bucket. " +
                      "Read more about ACLs here: https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html");
              }
          }),
      }],
  });

• I keep waffling on changing EnforcementLevel, "advisory", and "mandatory" to Severity, "warning", and "error". I don't feel strongly about it, but was initially leaning towards the less verbose Severity. However, Cameron pointed out we might want to add a "softMandatory" or "requiresApproval" value in the future, which doesn't work as well with Severity, so I've left it EnforcementLevel for now. Still open to changing.

• Originally, I specified the Policy as:

  export interface Policy {
      name: string;
      description: string;
      enforcementLevel: EnforcementLevel;
  
      validateResource?: ResourceValidation;
      validateStack?: StackValidation;
  }

  Which would allow setting either validateResource or validateStack on a policy. However, Cameron's feedback was that we should not allow both.

  To address, I've separated these out as follows so that the compiler will enforce defining either a validateResource or validatStack:

  export interface Policy {
      name: string;
      description: string;
      enforcementLevel: EnforcementLevel;
  }
  
  export interface ResourceValidationPolicy extends Policy {
      validateResource: ResourceValidation | ResourceValidation[];
  }
  
  export interface StackValidationPolicy extends Policy {
      validateStack: StackValidation;
  }
  
  export type Policies = (ResourceValidationPolicy | StackValidationPolicy)[];

  If the additional types add too much cognitive load, we could consider going back to the simpler design with two optional properties on the main Policy interface. We could also consider making it a runtime check to enforce only setting one of the two properties (if we felt strongly about enforcing this). Or, we could allow both to be set, but generally recommend setting one or the other.

• You'll probably notice that I changed:

  validateResource: ResourceValidation;

  to:

  validateResource: ResourceValidation | ResourceValidation[];

  I know we discussed how it didn't make a lot of sense to have rules which accepted a single rule (despite using this pattern elsewhere like in AWSX), and how the assumption is that most polices will have a single rule. When updating the examples, I found it was just easier to be able to provide multiple validation functions to be called for certain policies that are checking similar things across multiple resource types (e.g. EC2 Instance, LaunchConfiguration, or LaunchTemplate are using only approved AMIs).

  {
      name: name,
      description: "Instances should use approved AMIs.",
      enforcementLevel: "mandatory",
      validateResource: [
          validateTypedResource(aws.ec2.Instance.isInstance, (it, args, reportViolation) => {
              if (amis && !amis.has(it.ami)) {
                  reportViolation("EC2 Instances should use approved AMIs.");
              }
          }),
          validateTypedResource(aws.ec2.LaunchConfiguration.isInstance, (it, args, reportViolation) => {
              if (amis && !amis.has(it.imageId)) {
                  reportViolation("EC2 LaunchConfigurations should use approved AMIs.");
              }
          }),
          validateTypedResource(aws.ec2.LaunchTemplate.isInstance, (it, args, reportViolation) => {
              if (amis && it.imageId && !amis.has(it.imageId)) {
                  reportViolation("EC2 LaunchTemplates should use approved AMIs.");
              }
          }),
      ],
  };

  This is largely due to the way the validateTypedResource helper works to provide stronger typed resource objects to check since it only allows converting a single validation function into a strongly-typed resource. However, instead of making the property name plural (like with the previous rules), I've left it singular as the majority of policies will only have a single validateResource validation function -- but if it's simpler to implement a policy using multiple validation functions, the option is available.

  Though, we could go about strongly typing the resource in a different way which could obviate the need for multiple validation functions for such policies. More below.

• The implementation of ResourceValidationPolicy has been hooked-up, but StackValidationPolicy has not yet. @chrsmith, want me to do it, now that your change in pulumi/pulumi has been merged?

• Here's an example of a policy that is looking for potentially multiple violations, but something where you don't necessarily have to exit early when the first violation is found:

      export function requireEbsEncryption(name: string, kmsKeyId?: string): ResourceValidationPolicy {
      return {
          name: name,
          description: "EBS volumes should be encrypted",
          enforcementLevel: "mandatory",
          validateResource: validateTypedResource(aws.ebs.Volume.isInstance, (it, args, reportViolation) => {
              if (!it.encrypted) {
                  reportViolation("EBS volumes should be encrypted.");
              }
              if (kmsKeyId !== undefined && it.kmsKeyId !== kmsKeyId) {
                  reportViolation(`EBS volumes should be encrypted with KMS ID '${kmsKeyId}'.`);
              }
          }),
      };
  }

A different way to do stronger-typed resources

While the validateTypedResource approach to providing a strongly typed resource to program against works, it has two problems:

1. For policies that are looking for similar things over multiple resource types, it necessitates multiple validation functions (one per resource). It'd be nice if we could keep it a single validation function, and do the stronger typed filtering inside that.

2. The approach won't extend to the validateStack check, where you'll likely also want stronger-typed resources when looking at various resources in the stack.

One way to solve this would be to provide a helper like:

export function asResource<TResource extends Resource>(
    isInstance: (o: any) => o is TResource,
    args: { type: string, props: Record<string, any> },
): q.ResolvedResource<TResource> | undefined {
    args.props.__pulumiType = args.type;
    return isInstance(args.props) ? args.props : undefined;
}

Basically, if we can convert the resource, we will return it as the stronger typed q.ResolvedResource<TResource>, otherwise undefined.

Using it in a validateResource function would look like:

new PolicyPack("policy-pack-typescript", {
    policies: [{
        name: "s3-no-public-read",
        description: "Prohibits setting the publicRead or publicReadWrite permission on AWS S3 buckets.",
        enforcementLevel: "mandatory",
        validateResource: (args, reportViolation) => {
            const it = asResource(aws.s3.Bucket.isInstance, args);
            if (it && (it.acl === "public-read" || it.acl === "public-read-write")) {
                reportViolation(
                    "You cannot set public-read or public-read-write on an S3 bucket. " +
                    "Read more about ACLs here: https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html");
            }
        },
    }],
});

And you could have multiple calls to asResource inside the single validation function, to cast to as many resource types as you'd like. Though, it's a bit more verbose and cumbersome than the validateTypedResource helper.

But the same asResource helper function could be used inside validateStack to get stronger typing for the resources there.

What do folks think?

Part of #87

[ekrengel]

In regards to

validateResource: ResourceValidation | ResourceValidation[];

I think the example doesn't really make sense to me. I see these three rules as separate concerns and hence they should be separate policies.

---

I do like the "A different way to do stronger-typed resources" section. I like that it can also be used for the validateStack, but I am not sure how I feel about it for the validateResource case. I am wondering if it makes sense to have both asResource and validateTypedResource?

I think we may also need to support casting to a new interface. IE for all resources that have tags, the tags should be in this format.

[justinvp]

I see these three rules as separate concerns and hence they should be separate policies.

OK. I'll remove the array and update the examples to use separate policies.

I am wondering if it makes sense to have both asResource and validateTypedResource?

Yeah, I think so -- they aren't mutually exclusive.

I think we may also need to support casting to a new interface. IE for all resources that have tags, the tags should be in this format.

Hmm, yeah, we do have the following example, which kind of does this already manually without any additional built-in helpers: https://github.com/pulumi/examples/blob/567ecaf3972a48e6d5bac090c1d845d9aa1f5fbb/policy-packs/aws-advanced/compute.ts#L233-L267

[clstokes]

Regarding validateResource: ResourceValidation | ResourceValidation[];, I think the "Instances should use approved AMIs." example is a great use case for having the option to do multiple validations in a single policy. This is similar to how I've expressed checking for the existence of the same set of tags across multiple resource types too - below in the old format.

{
    name: "required-name-tag",
    description: "A 'Name' tag is required.",
    enforcementLevel: "mandatory",
    rules: [
        typedRule(aws.ec2.Instance.isInstance, it => {
            requireNameTag(it.tags);
        }),
        typedRule(aws.ec2.Vpc.isInstance, it => {
            requireNameTag(it.tags);
        }),
    ]
},

asResource seems helpful for validateStack but I'm failing to see how it could be used to help obviate the need for multiple validation functions. How would this work?

While we're doing API design, do we also need to consider #78 ? As an example, how would I write a rule "instance must contain a 'Name' tag that ends with pulumi.getStack()"? Or will this be addressed later?

All in all, I'm good with the changes.

[ekrengel]

ALSO can we come up with an example unit test?

I think if @clstokes likes the multiple rules approach we should listen to him since he is closest to our user base.. I just want to make sure theres still an easy way to test this!

[justinvp]

asResource seems helpful for validateStack but I'm failing to see how it could be used to help obviate the need for multiple validation functions. How would this work?

It'd look like:

{
    name: "required-name-tag",
    description: "A 'Name' tag is required.",
    enforcementLevel: "mandatory",
    validateResource: (args, reportViolation) => {
        const instance = asResource(aws.ec2.Instance.isInstance, args);
        if (instance) {
            requireNameTag(instance.tags, reportViolation);
        }
        const vpc = asResource(aws.ec2.Vpc.isInstance, args);
        if (vpc) {
            requireNameTag(vpc.tags, reportViolation);
        }
    },
},

Or shorter by tweaking requireNameTag:

{
    name: "required-name-tag",
    description: "A 'Name' tag is required.",
    enforcementLevel: "mandatory",
    validateResource: (args, reportViolation) => {
        requireNameTag(asResource(aws.ec2.Instance.isInstance, args), reportViolation);
        requireNameTag(asResource(aws.ec2.Vpc.isInstance, args), reportViolation);
    },
},

const requireNameTag = function (resource: any | undefined, reportViolation: ReportViolation) {
    if (resource && (!resource.tags || resource.tags["Name"] === undefined)) {
        reportViolation("A 'Name' tag is required.");
    }
};

[justinvp]

While we're doing API design, do we also need to consider #78 ? As an example, how would I write a rule "instance must contain a 'Name' tag that ends with pulumi.getStack()"? Or will this be addressed later?

This has been taken into consideration, but will be exposed later. We can add resource options, project name, and stack name to the args.

[justinvp]

ALSO can we come up with an example unit test?

I'll post a couple approaches here in a little bit. The approach Joe took in pulumi/pulumi-policy-aws#2 of separating out the checks to separate exportable functions allows easily testing resources in a strongly typed way. Testing a Policy itself can be done easily enough with a small amount of helper "runner" code (which we could include in @pulumi/policy/testing, if we wanted), but it isn't as nice of an experience because you have to specify a type string when passing args and we don't make it easy to get those type names for our resources.

[justinvp]

We discussed validateResource: ResourceValidation | ResourceValidation[]; at the PaC Check-In meeting yesterday and decided to allow setting multiple validation functions on a single policy, as it's an easy way to check multiple types of resources in a strongly typed way using the validateTypedResource helper.

{
    name: "required-name-tag",
    description: "A 'Name' tag is required.",
    enforcementLevel: "mandatory",
    validateResource: [
        validateTypedResource(aws.ec2.Instance.isInstance, (it, args, reportViolation) => {
            requireNameTag(it.tags, reportViolation);
        }),
        validateTypedResource(aws.ec2.Vpc.isInstance, (it, args, reportViolation) => {
            requireNameTag(it.tags, reportViolation);
        }),
    ],
},

Most policies will still have a single validation function, hence validateResource is singular, which should at least address the main issue with the previous API (rules) being plural but taking just a single rule most of the time.

[ekrengel]

LGTM in terms of the API itself.. do have some questions.

[ekrengel]

How do we test if this is working as expected?

[justinvp]

I've manually tested it, but I will add some actual integration tests.

[justinvp]

@ekrengel, I added some basic integration tests. Definitely room to add a lot more tests here, like validating unknowns, adding a Python program, etc., but we can add those subsequently as part of #9.

[ekrengel]

I added some basic integration tests.

Awesome thanks for taking the time to do this :)