Add metadata flags for drift

[anshikg]

Issue #, if available:

Description of changes: Add new metadata flags for Uluru schemas to make the schema more verbose for drift.

Test:

• build the package
• added unit test for new metadata flags

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[PatMyron]

Something unclear about these: are these all specifically about ReadHandlers? For example, currently AWS::ImageBuilder::Component.Platform must be input with the casing Linux for the CreateHandler to succeed, but the ReadHandler returns LINUX for that value. Is that property case sensitive or case insensitive? Should we consider an enum if this metadata cannot be expressed in just a boolean?

---

reminds me a bit of @aygold92's comment here if this case sensitivity is just about making drift detection work rather than the actual case sensitivity of the property itself: https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/pull/86#discussion_r394774579

Been thinking we should vend IAM policies that prevent modification of CloudFormation-managed resources based on the automatic resource tagging to prevent drift rather than just trying to detect it

---

For case sensitivity of properties themselves, this discussion around a similar proposal for JSON schema itself seems relevant

[anshikg]

Something unclear about these: are these specifically about ReadHandlers? For example, currently AWS::ImageBuilder::Component.Platform must be input with the casing Linux, but the ReadHandler returns LINUX for that value. Is that property case sensitive or case insensitive? Should we consider an enum instead of a boolean?

According to the above implementation if the team does not care wether the value is Linux/LINUX they case set this to the caseInsensitive flag to true. Enum is probably a good route if the property have finite set of values. In case, the property is accepting any values use the case sensitive flag is good route. Another thing, in the above case we just need to be careful we don't break the existing stacks. If the customer was providing Linux/LINUX as input going forward both should be acceptable

[benbridts]

I have been thinking about these changes a bit, and I have a few questions/concerns:

Some properties can be updated by the downstream service teams and these properties could have a prefix/postfix or both once returned from the Read Handler

I have no idea what this means in practice. From the perspective of a third-party resource provider, there is no such thing as a "downstream service team".

• If I'm writing a resource for our own products, there are no "service teams"
• If I'm writing a resource to plug a gap in coverage for AWS resources, the only thing I get to interact with is an API.

I also have concerns that relate a bit to the remark by @jotompki

are we planning on handling this sort of prefix additions handler side? if so would have to do for every language plugin

Mainly that I would expect that the output of the ReadHandler would always be a valid input for each property (assuming they're not readOnly).

The ignore case and null is empty list should fit that expectation. The suffix and prefix maybe do not.

• If I add this to a property that I can also !GetAtt or !Ref, will the output shown in the drift detection be meaningfully different than what I would see in an output? Having any difference between those sounds like something that might lead to problems. The difference changing the actual string even more so.
• A particular future problem would be a feature that automates the import of existing resources. If there is meta-data present, should it remove the suffix/prefix before generating a template? Depending on the schema, this might even break the contract [see below]
• If the underlying API changes, I would expect the handler to deal with that, the way I read the suffix/prefix (and I might be reading it wrong, see my first point), that responsibility is instead pushed to the caller.

---

from https://docs.aws.amazon.com/cloudformation-cli/latest/userguide/resource-type-test-contract.html#resource-type-test-contract-read

A read handler MUST return a model representation that conforms to the shape of the resource schema

If you specify a regex or similar for a property, the returned model might not fit in the shape

[PatMyron]

these properties could have a prefix/postfix or both once returned from the Read Handler

I have no idea what this means in practice

@ikben AWS::Route53::HostedZone.Name is a good example (optional . suffix):

[johnttompkins]

these properties could have a prefix/postfix or both once returned from the Read Handler

I have no idea what this means in practice

@ikben AWS::Route53::HostedZone.Name is a good example (optional . suffix):

Are there any other use cases other than this ? This could set a precedent of adding in metadata to the schema for one-off drift integration issues.

[PatMyron]

Are there any other use cases other than this ? This could set a precedent of adding in metadata to the schema for one-off drift integration issues.

I do worry if this problem is restricted to only prefixes and suffixes too. For example, many times . don't matter in the first half of email addresses, so this problem doesn't seem strictly limited to prefixes and suffixes

[anshikg]

these properties could have a prefix/postfix or both once returned from the Read Handler

I have no idea what this means in practice

@ikben AWS::Route53::HostedZone.Name is a good example (optional . suffix):

Are there any other use cases other than this ? This could set a precedent of adding in metadata to the schema for one-off drift integration issues.

IAM Policy document has values which expect an account Id but return Arn.

[PatMyron]

IAM Policy document has values which expect an account Id but return Arn.

@anshikg can these optional prefixes/suffixes be patterns instead of literal strings? This doesn't seem solvable by literal string prefixes/suffixes since ARNs differ per partition

[benbridts]

AWS::Route53::HostedZone.Name is a good example (optional . suffix):

That is a good example as the behavior is well defined by the API. If this is the kind of use case, I think it would make sense to define it in the readme as strings that are 100% equivalent.

IAM Policy document has values which expect an account Id but return Arn.

That sounds to me like a bug in the resource provider as it breaks the contract (AWS::IAM::Policy defines a regex that wouldn't match an ARN for eg. Groups)

For cases where it wouldn't break the contract (eg. the Property can be either a name or an ARN), you would have to be careful about how you do the conversion, as the conversion isn't necessarily symmetric (if you remove arn:aws:$service:.*:.*:thing/ from every input, things from different regions will collapse down to the same name)

[wbingli]

To address the drift issues, it would be good to provide multiple layers of solution. This is one of the solutions that to provide schema level metadata to address majority of drift issues without writing code. I agree with the opinion https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/pull/103#issuecomment-709625526 that the metadata name is confusing. Customer may think whether the property name is caseSensitive or not, and wont think it's for drift feature. Something I like is following:

drift-override : {
 "caseSensitive": false,
 "nullEqualsEmpty": true
 }

Other layer of solution is providing more flexible and dynamic solution so that resource handler can make decision.

As for Andrew's comment that "I think the real solution for drift is to take a baseline of the live state at the end of an operation and use that to compare against instead of the template."https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/pull/103#pullrequestreview-510740117. My main concern to this approach is that the live state of last operation is different from the CFN template. It will bring customer UX issue to map yet another model with CFN template to figure out what is changed or not.

[anshikg]

Closing this pull request. This is not a scalable mechanism to support this feature.