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

[cfnspec] CloudFormation Registry Schemas translation to CFN Specifications is lossy #9676

Closed
rix0rrr opened this issue Aug 13, 2020 · 2 comments
Labels
@aws-cdk/cfnspec bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/large Large work item – several weeks of effort management/devenv Related to CDK development/build environment p1

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 13, 2020

The issue

This surfaced in #9452. We have seen this before with other teams, but never investigated enough to figure out the root cause.

The problem is that, as AWS teams are moving to the CloudFormation Registry (internal codename more well-known but I'm not going to reproduce that here) they are rewriting their property schemas in the new schema language.

That language is JSON Schema, and is more expressive than the CFN Spec. What seems to be done at the moment is that this schema is being converted back into the CFN spec on a best-effort basis. However, since the upstream schema is more expressive, some things cannot be translated, and so happen to be translated in a way that completely breaks their interpretation in the CFN schema.

Motivating example, ECS' Options type.

It used to be defined as (some details elided):

     "Properties": {
        "Options": {
          "PrimitiveItemType": "String",
          "Type": "Map",
        },
     },

In the new CloudFormation Registry schema language, it is now defined as:

        "Options": {
          "$ref": "#/definitions/Options"
        },

// ...

    "Options": {
      "type": "object",
      "patternProperties": {
        ".{1,}": {
          "type": "string"
        }
      }
    },

Which should be interpreted as "the property Options is of a new type Options, which is a Map<String> containing keys of at least one character.

This cannot be expressed in the CFN Schema for two reasons:

  • Map<> types cannot be aliased.
  • The constraint that keys should have at least one character cannot be expressed.

The currently generated spec looks like this:

     "Properties": {
        "Options": {
          "Type": "Options",
        },
     },

    "AWS::ECS::TaskDefinition.Options": {
    },

What ends up happening is that we generate the definition of the Options type, but the Options type itself ends up as a record with no fields. CDK then promptly drops all data that used to go there, and breaks the interface definitions.

Solutions

There are a number of ways to address this:

  • patching it by hand: will become untenable as more and more teams will move to CloudFormation Registry implementations, and involves a lot of work on our part.
  • interpreting empty property types specially: we just assume that types that end up with zero properties were intended to be aliases for Map<>, so we just generate those. We won't be able to derive the element type for the map though, as that information is lost.
  • have CloudFormation update their CFN Spec generation logic: it is still true that JSON schema can express more than the CFN Spec can express, but commonly occurring cases such as this could be handled "more correctly" than they are being handled right now.
  • start parsing the new schema: we start parsing the new schema ourselves, probably in addition to the old schema. We need to set up tooling to import the new schema and since it's a lot more flexible, also harder to parse (and parse correctly), so a pretty big undertaking.

This is blocking/hampering CFNSpec imports until resolved.


This is 🐛 Bug Report

@rix0rrr rix0rrr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 13, 2020
@rix0rrr rix0rrr added management/devenv Related to CDK development/build environment p1 effort/large Large work item – several weeks of effort labels Aug 13, 2020
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 13, 2020

Internal reference: D17415152

mergify bot pushed a commit that referenced this issue Aug 13, 2020
Suspicious changes:

* IoT: **AWS::IoT::ProvisioningTemplate.Tags** used to be specified as a `List<ItemType=Json>`. They now moved that same broken definition into changed to a `Tags` property type.

It's still incorrect but in a different place. Updating the patch to match new the location of the broken definition.

* ECS: a number of `Options` types used to be defined as `Map<String>`, but are now typed as `Options` which is itself a property typed defined with 0 properties. This leads us to drop all properties added there on the floor (because they're all not part of the schema). See: #9676
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Aug 14, 2020
@eladb eladb assigned rix0rrr and unassigned eladb Aug 17, 2020
@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/cfnspec bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/large Large work item – several weeks of effort management/devenv Related to CDK development/build environment p1
Projects
None yet
Development

No branches or pull requests

3 participants