cloudformation-cli

cloudformation-cli copied to clipboard

Hey, I'm trying to run the handler_create.py::contract_create_delete contract test, but it is failing because

All properties specified in the request MUST be present in the model returned, and they MUST match exactly, with the exception of properties defined as writeOnlyProperties in the resource schema

Specifically, the test tries to create the resource with model:

{'Id': 'N', 'Authorization': None, 'Tags': [], 'EgressAccessLogs': {'LogGroupName': '/aws/MediaPackage/0'}}

Note the empty Tags list and null Authorization. The test gets back this as a response from our resource provider:

{'Id': 'N', 'EgressAccessLogs': {'LogGroupName': '/aws/MediaPackage/0'}}

My question:

Should this test ignore null value inputs? Using the java rpdk, I'm not sure how to differentiate between null and nonexistant in responses.

How about empty lists? The API backing the resource provider returns Tags as an empty map/dict no matter whether the request was with an empty dict or null/nonexistant. Customers can craft CFN templates such that Tags is either an empty list or non existant/null. Is it valid to treat them the same (ie only return nonexistant or only return empty list), or do we need to handle both cases in our resource provider?

Code in question: https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/contract/suite/handler_commons.py#L176

According to the contract documentation that test is correct.

It is a bit annoying that the read handler has a different requirement.

A read handler MUST return a model representation that conforms to the shape of the resource schema. [...] The model MUST NOT return any properties that are null or do not have values.

So while the current test is correct "should the contract change?" is a valid question.

Unfortunatly I can't help you with the java rpdk.

Thanks for pointing me to that doc @benbridts!

This is probably a question for the CloudFormation team:

The combination of A)

All properties specified in the create [or update] request MUST be present in the model returned, and they MUST match exactly, with the exception of properties defined as writeOnlyProperties in the resource schema.

and B)

The model MUST NOT return any properties that are null or do not have values.

seems to suggest we should instead be checking that the output does not contain any null params. (As opposed to how it currently works - expecting null input params to be in null in the output as well.) Do you agree?

Also, if I'm reading this right, maybe we should consider adding null input params to the list of exceptions in requirement A for clarity?

seems to suggest we should instead be checking that null input params are not in the output. Do you agree?

I read it differently. Since the CloudFormation engine (or in this case the contract test) sends all keys in the Create request, all the keys should be returned in the Create response. I don't think the value of the property should make a difference.

Note that A) is for the create request and B) for the read request. They have different requirements about which properties should be returned (which I find annoying, but might be to remove ambiguity between Null and an empty list?)

@benbridts I also assumed it should just be the keys, but this code looks like it is actually comparing values.

It does, Some clearer languages in the documentation could help here (in addition to the null/empty problem)