search

pplu / cfn-perl

An object model for CloudFormation documents
Other
0 stars 5 forks source link

Update Cfn to latest CF spec #69

Closed agimenez closed 3 years ago

agimenez commented 3 years ago

Hi,

Trying to deploy a Network Firewall, we're getting the following error:

Attribute (StatelessDefaultActions) does not pass the type constraint because: Validation failed for 'Cfn::Resource::Properties::AWS::NetworkFirewall::FirewallPolicy::StatelessActions' with value \[ "aws:pass" \] at /opt/capside/clouddeploy/lib/perl5/x86_64-linux/Moose/Meta/Class.pm line 275

The code from the tool that uses Cfn is the following (I think you'll be acquainted with the tool :P):

resource NetworkFirewallPolicy => "AWS::NetworkFirewall::FirewallPolicy", {
    FirewallPolicyName => CfString("#-#Parameter(firewallid)#-#-#-#Parameter(shortenv)#-#-policy"),
    Description => CfString("Firewall Policy for #-#Parameter(firewallid)#-#-#-#Parameter(shortenv)#-#"),
    FirewallPolicy => {
      StatelessDefaultActions =>         [ "aws:pass" ],
      StatelessFragmentDefaultActions => [ "aws:pass" ],
    },
  [...]
  };

The problem is that the current Cfn version is generated from an old spec that defines both Default and Custom actions as a list of StatelessActions objects:

    "AWS::NetworkFirewall::FirewallPolicy.FirewallPolicy": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-networkfirewall-firewallpolicy-firewallpolicy.html",
      "Properties": {
        "StatelessDefaultActions": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-networkfirewall-firewallpolicy-firewallpolicy.html#cfn-networkfirewall-firewallpolicy-firewallpolicy-statelessdefaultactions",
          "UpdateType": "Mutable",
          "Required": true,
          "Type": "StatelessActions"
        },

The latest spec defines all the "Default" actions as list of strings:

    "AWS::NetworkFirewall::FirewallPolicy.FirewallPolicy": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-networkfirewall-firewallpolicy-firewallpolicy.html",
      "Properties": {
        "StatelessDefaultActions": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-networkfirewall-firewallpolicy-firewallpolicy.html#cfn-networkfirewall-firewallpolicy-firewallpolicy-statelessdefaultactions",
          "UpdateType": "Mutable",
          "Required": true,
          "Type": "List",
          "PrimitiveItemType": "String",
          "DuplicatesAllowed": false
        },

Could the classes be regenerated to make Cfn compatible with the current spec?

Thanks!

pplu commented 3 years ago

Could the classes be regenerated to make Cfn compatible with the current spec?-

Yes. Make a clone of this repo, carton install, and use the Makefile: https://github.com/pplu/cfn-perl/blob/master/Makefile#L22 for generating the new classes.

Look at https://github.com/pplu/cfn-perl/network, where you will see how we branch out to update cloudformation definitions.

If you want to contribute a PR to update the schema: I'll be happy to accept it. I've added you as a contributor to the project so you can push your changes also.

Once its done: I can also give you COMAINT for the Cfn module on CPAN, so you can make dist to get that code to CPAN :)

pplu commented 3 years ago

You can request a PAUSE account (to upload dists to CPAN) here: https://pause.cpan.org/pause/query?ACTION=request_id

agimenez commented 3 years ago

Thanks! I tried buidling it, but I got this error:

carton exec build-bin/fix-spec spec/us-east-1.json spec/spec.json
OUTPUT_DIR=lib carton exec perl -I build-lib/ build-bin/build-cfn-classes
Attribute (Documentation) is required at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Object.pm line 24
        Moose::Object::new('AWSJsonSpec::Property', 'ItemType', 'Tag') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/MooseX/DataModel.pm line 89
        MooseX::DataModel::__ANON__('HASH(0x2397348)') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Meta/TypeCoercion.pm line 65
[...]
        Class::MOP::Class::new_object('Moose::Meta::Class=HASH(0x1884a50)', 'HASH(0x2aaa108)') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Meta/Class.
pm line 275
        Moose::Meta::Class::new_object('Moose::Meta::Class=HASH(0x1884a50)', 'HASH(0x2aaa108)') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Object.pm 
line 24
        Moose::Object::new('AWSJsonSpec', 'HASH(0x1e095a8)') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/MooseX/DataModel.pm line 171
        MooseX::DataModel::new_from_json('AWSJsonSpec', '{^J   "PropertyTypes" : {^J      "AWS::ACMPCA::Certificate.ApiPassthrough" : {^J         "Documentation" : "http://docs.aws.amazon.com/AWSC
loudFormation/latest/UserGuide/aws-properties-acmpca-certificate-apipassthrough.html",^J         "Properties" : {^J            "Extensions" : {^J               "Documentation" : "http://docs.aws.a
mazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-acmpca-certificate-apipassthrough.html#cfn-acmpca-certificate-apipassthrough-extensions",^J               "Required" : false,^J 
[...]
loudFormation/latest/UserGuide/aws-resource-ask-skill.html#cfn-ask-skill-skillpackage",^J               "Required" : true,^J               "Type" : "SkillPackage",^J               "UpdateType" : "Mutable"^J            },^J            "VendorId" : {^J               "Documentation" : "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ask-skill.html#cfn-ask-skill-vendorid",^J               "PrimitiveType" : "String",^J               "Required" : true,^J               "UpdateType" : "Immutable"^J            }^J         }^J      }^J   }^J}^J') called at build-lib/CfnModel.pm line 21
    CfnModel::__ANON__('CfnModel=HASH(0x22bf488)') called at reader CfnModel::spec (defined at build-lib/CfnModel.pm line 22) line 6
    CfnModel::spec('CfnModel=HASH(0x22bf488)') called at build-lib/CfnModel.pm line 27
    CfnModel::__ANON__('CfnModel=HASH(0x22bf488)') called at reader CfnModel::classes_in_spec (defined at build-lib/CfnModel.pm line 28) line 6
    CfnModel::classes_in_spec('CfnModel=HASH(0x22bf488)') called at build-lib/CfnModel.pm line 44
    CfnModel::__ANON__('CfnModel=HASH(0x22bf488)') called at reader CfnModel::classes (defined at build-lib/CfnModel.pm line 51) line 6
    CfnModel::classes('CfnModel=HASH(0x22bf488)') called at build-lib/CfnModel.pm line 87
    CfnModel::write_classes('CfnModel=HASH(0x22bf488)') called at build-bin/build-cfn-classes line 10
Makefile:14: recipe for target 'gen-classes' failed
make: *** [gen-classes] Error 255

I can see that the get-all-specs is misbehaving, leaving some specfiles empty. May be something related to my network, will keep you posted.

Thanks!

agimenez commented 3 years ago

Retried with the full specs downloaded, and I get an error. Not the same, though:

OUTPUT_DIR=lib carton exec perl -I build-lib/ build-bin/build-cfn-classes
Attribute (UpdateType) is required at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Object.pm line 24
        Moose::Object::new('AWSJsonSpec::Property', 'ItemType', 'Tag') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/MooseX/DataModel.pm line 89
        MooseX::DataModel::__ANON__('HASH(0x3b14880)') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Meta/TypeCoercion.pm line 65
[...]
loudFormation/latest/UserGuide/aws-resource-ask-skill.html#cfn-ask-skill-skillpackage",^J               "Required" : true,^J               "Type" : "SkillPackage",^J               "UpdateType" : "Mutable"^J            },^J            "VendorId" : {^J               "Documentation" : "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ask-skill.html#cfn-ask-skill-vendorid",^J               "PrimitiveType" : "String",^J               "Required" : true,^J               "UpdateType" : "Immutable"^J            }^J         }^J      }^J   }^J}^J') called at build-lib/CfnModel.pm line 21
    CfnModel::__ANON__('CfnModel=HASH(0x2174438)') called at reader CfnModel::spec (defined at build-lib/CfnModel.pm line 22) line 6
    CfnModel::spec('CfnModel=HASH(0x2174438)') called at build-lib/CfnModel.pm line 27
    CfnModel::__ANON__('CfnModel=HASH(0x2174438)') called at reader CfnModel::classes_in_spec (defined at build-lib/CfnModel.pm line 28) line 6
    CfnModel::classes_in_spec('CfnModel=HASH(0x2174438)') called at build-lib/CfnModel.pm line 44
    CfnModel::__ANON__('CfnModel=HASH(0x2174438)') called at reader CfnModel::classes (defined at build-lib/CfnModel.pm line 51) line 6
    CfnModel::classes('CfnModel=HASH(0x2174438)') called at build-lib/CfnModel.pm line 87
    CfnModel::write_classes('CfnModel=HASH(0x2174438)') called at build-bin/build-cfn-classes line 10
Makefile:14: recipe for target 'gen-classes' failed

I guess we need some "fix-spec" thingie here, but I'm not sure where to start looking.

agimenez commented 3 years ago

Opened PR #70 in case you want to take a look.

agimenez commented 3 years ago

Could pinpoint the problem to "AWS::ACMPCA::Certificate". I've tested deleting the whole resource from the document tree in build-bin/fix-spec and now CfnModel doesn't complain, but I still get a bunch of errors:

OUTPUT_DIR=lib carton exec perl -I build-lib/ build-bin/build-cfn-classes
AWS::CloudWatch::InsightRule.Tags doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::SageMaker::ModelExplainabilityJobDefinition.Environment doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::Macie::FindingsFilter.Criterion doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::MWAA::Environment.TagMap doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::Transfer::Server.Protocol doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::SageMaker::MonitoringSchedule.Environment doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::StepFunctions::StateMachine.Definition doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::LakeFormation::DataLakeSettings.Admins doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::SageMaker::ModelBiasJobDefinition.Environment doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::Glue::SecurityConfiguration.S3Encryptions doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::CodeBuild::Project.FilterGroup doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::SSM::PatchBaseline.PatchStringDate doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::S3::StorageLens.Encryption doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::Transfer::User.SshPublicKey doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::SageMaker::DataQualityJobDefinition.Environment doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::AppSync::GraphQLApi.AdditionalAuthenticationProviders doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::AppSync::GraphQLApi.Tags doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
AWS::SageMaker::ModelQualityJobDefinition.Environment doesn't have properties in the spec at build-lib/CfnModel/CfnSubtype.pm line 28.
Error processing template undef error - Don't know how to handle type for property Parameters on AWS::DataBrew::Recipe.Action at build-lib/CfnModel/CfnProperty.pm line 104.
Makefile:14: recipe for target 'gen-classes' failed
make: *** [gen-classes] Error 255

I guess that they should be "fixed" using the script, but I'm not sure how. I will keep investigating, but guidance would be appreciated :)

agimenez commented 3 years ago

Found AWS::DataBrew::Recipe.Action. It seems that this spec is weird.

pplu commented 3 years ago

The messages before the error are just warnings (which one day should be addressed)

The error is because in the definition for AWS::DataBrew::Recipe.Action the type of the Parameters property is not recognized by the code generator. If you know of a type that it should be generated as, you can patch the specification in the fix-specs script, where you will see that we have traditionally had to correct stuff in the spec.

Seeing that https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-databrew-recipe-action.html the actions object has no type specified (docs are generated from the spec), it seems like AWS has an error in the spec (not uncommon). Here you have something of a guide into what Parameters can hold. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-databrew-recipe.html. I think you can add an object type to the spec hold the Parameters object, or make the type of the Parameters attribute a free key-value object that we already have in Cfn.

agimenez commented 3 years ago

Thanks! I've been doing more digging, and something seems to be off and I don't quite get why. After understanding the process, I re-launched the fix-spec and the gen-classes without any modification, just from the latest spec. I tget the following error:

Attribute (Documentation) is required at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/x86_64-linux/Moose/Object.pm line 24
        Moose::Object::new('AWSJsonSpec::Property', 'ItemType', 'Tag') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/MooseX/DataModel.pm line 89
[...]
Moose::Object::new('AWSJsonSpec', 'HASH(0x292e340)') called at /home/lagimenez/git/github-agimenez/cfn-perl/local/lib/perl5/MooseX/DataModel.pm line 171
        MooseX::DataModel::new_from_json('AWSJsonSpec', '{^J   "PropertyTypes" : {^J      "AWS::ACMPCA::Certificate.ApiPassthrough" : {^J         "Documentation" : "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-acmpca-certificate-apipassthrough.html",^J         "Properties" : {^J            "Extensions" : {^J               "Documentation" : "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-acmpca-certificate-apipassthrough.html#cfn-acmpca-certificate-apipassthrough-extensions",^J               "Required" : false,^J 
[...]

At first I thought that the problem was in AWS::ACMPCA::Certificate.ApiPassthrough, but I just realised that that's the whole JSON, and that is the very first entry. It is not the one causing the error because it has all the Documentation fields present.

Is there any way to debug what exact resource is the one that is causing the validation to fail? AWSJsonSpec seems to be a wrapper of MooseX::DataModel, and I'm not sure how to debug that?

pplu commented 3 years ago

The AWS JSON specs are getting sloppier by the day :(. It seems like different teams are developing the specs, and they aren't getting linted very well: I'm getting new surprises every time I generate a new Cfn from specs. Time ago it was quite smooth.

The problem comes from one of the objects coming from the spec.json missing a required Documentation field. This is controlled by: https://github.com/pplu/cfn-perl/blob/master/build-lib/AWSJsonSpec.pm. The difficulty of tracing the error down to the object causing the error is because how MooseX::DataModel is built (using Moose coercions). The Moose coercions have no idea of "where" inside the datastructure being converted to object model they are, so errors about missing fields and such are not helpful :(

You can also try unrequiring the documentation field to be present, and maybe later, in the code generation phase die on an undefined Documentation field. That may point you to the error, and the most probable: it may generate all classes successfully, because not all fields in the spec are used.

agimenez commented 3 years ago

Thanks, I had to remove the required attribute from all fields from AWSJsonSpec::Property for it not to crap out on loading the JSON (now I got another issue, but I know how to address that, I think).

What makes me think, why those fields were marked as required? Is there any "CloudFormation json schema" out there that defines the structure? Maybe we are imposing some rules that shouldn't be there, making it harder than it really is?

pplu commented 3 years ago

There is a formal spec here https://docs.aws.amazon.com/en_us/AWSCloudFormation/latest/UserGuide/cfn-resource-specification-format.html

I can't remember if I built the object model before there was a formal spec published or not.

agimenez commented 3 years ago

Hi, I did some digging and added some validation to both AWSJsonSpec::Property and CfnModel. It seems that the problem is with the PropertyTypes section of the JSON. There are weird properties that don't follow the usual structure. An example of a cannonical one is AWS::S3::Bucket.Metrics:

$VAR1 = bless( {
                 'Properties' => {
                                   'EventThreshold' => bless( {
                                                                'Documentation' => 'http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-metrics.html#cfn-s3-bucket-metrics-eventthreshold',
                                                                'UpdateType' => 'Mutable',
                                                                'Type' => 'ReplicationTimeValue',
                                                                'Required' => bless( do{\(my $o = 0)}, 'JSON::PP::Boolean' )
                                                              }, 'AWSJsonSpec::Property' ),
                                   'Status' => bless( {
                                                        'PrimitiveType' => 'String',
                                                        'Required' => bless( do{\(my $o = 1)}, 'JSON::PP::Boolean' ),
                                                        'Documentation' => 'http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-metrics.html#cfn-s3-bucket-metrics-status',
                                                        'UpdateType' => 'Mutable'
                                                      }, 'AWSJsonSpec::Property' )
                                 },
                 'Documentation' => 'http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-metrics.html'
               }, 'AWSJsonSpec::PropertyType' );

But I found at least one AWS::Glue::SecurityConfiguration.S3Encryptions which doesn't have the Properties field:

$VAR1 = bless( {
                 'Documentation' => 'http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-glue-securityconfiguration-s3encryptions.html'
               }, 'AWSJsonSpec::PropertyType' );

The documentation just yields an "enpty" property: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-glue-securityconfiguration-s3encryptions.html.

This has been more and more common recently. The question is what should we do in these cases. I have modified the AWSJsonSpec::Property to disable the built-in validation, and added a validate method so we can control the behaviour:

package AWSJsonSpec::Property {
  use MooseX::DataModel;

  #required
  key Required => (isa => 'Bool');
  key Documentation => (isa => 'Str');

  key PrimitiveType => (isa => 'Str');
  key PrimitiveItemType => (isa => 'Str');
  key Type => (isa => 'Str');
  key ItemType => (isa => 'Str');

  #required
  key UpdateType => (isa => 'Str');

  sub validate {
      my $self = shift;

      return 0 unless $self->can('Required');
      return 0 unless $self->can('Documentation');
      return 0 unless $self->can('UpdateType');
  }
}

We could just go ahead and don't impose this "validation", but I'm not sure if the class generators are ready for that and will do the right thing, or they will generate invalid code. What do you think?

agimenez commented 3 years ago

Oh, I just realised that the Properties field on AWSJsonSpec::PropertyType is not required, so that's not the validation was crapping out. Will keep checking.

agimenez commented 3 years ago

OK, started from clean state, removed all the "fixes" and added a needed one and it works with only one quirk. They have recently updated the spec to 35.0.0 (the one I started working with was 34.0.0), and now the class generation seems to leak, consuming all my computer memory.

I managed to recover the specs for 34.0.0 and reset my branch to that. The only modifications have been done to the fix script, and #70 is ready to be reviewed/merged

pplu commented 3 years ago

Cfn 0.15 just hit CPAN with your PR (spec 34)

Thanks for your contribution.