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

Incompatible tags definition for AWS::Athena::WorkGroup #6936

Closed
udondan opened this issue Mar 23, 2020 · 10 comments · Fixed by #9085
Closed

Incompatible tags definition for AWS::Athena::WorkGroup #6936

udondan opened this issue Mar 23, 2020 · 10 comments · Fixed by #9085
Assignees
Labels
@aws-cdk/aws-athena Related to AWS Athena bug This issue is a bug. in-progress This issue is being actively worked on. p1

Comments

@udondan
Copy link
Contributor

udondan commented Mar 23, 2020

Currently it is not possible to create an AWS::Athena::WorkGroup due to the way tags are defined.

The WorkGroup resource expects tags to be a list of objects. The definition in the autogenerated code though has another level. Something like:

"Tags": {
  "Tags": [{
    "key": "a",
    "value": "b"
  }]
}

Even if no tags are set for the resource, the generated template holds an empty object:

"Tags": {},

This format is not supported and CloudFormation fails with an Internal Failure.

Reproduction Steps

This code compiles. Note the nested tags definition.

    new athena.CfnWorkGroup(scope, 'Athena-Workgroup', {
        name: 'HelloWorld',
        description: 'A WorkGroup,
        recursiveDeleteOption: true,
        state: 'ENABLED',
        tags: {
            tags: [{
                key: 'a',
                value: 'b',
            }]
        },
        workGroupConfiguration: {
            requesterPaysEnabled: true,
            resultConfiguration: {
                outputLocation: `s3://${bucket.bucketName}/athena/results/`,
                encryptionConfiguration: {
                    encryptionOption: 'CSE_KMS',
                    kmsKey: key.keyArn,
                },
            },
        }
    });

If you remove the outer tags, it won't compile.

If you remove the whole tags definition it again compiles, but fails to create the resource due to the unexpected default value of {}.

Error Log

Internal Failure ;-)

Environment

All of them

Other

I wanted to go ahead and fix the problem myself but since this is part of the autogenerated code which even seems to be not in the repo, I wouldn't know how.


This is 🐛 Bug Report

@udondan udondan added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 23, 2020
@SomayaB SomayaB added the @aws-cdk/aws-athena Related to AWS Athena label Mar 25, 2020
@l8on
Copy link

l8on commented Mar 27, 2020

I have run into the same issue.

Thanks to this report, I was able I work around it by using one of the "Escape Hatches":

export class WorkAroundGroup extends CfnWorkGroup {
  constructor(scope: Construct, id: string, props?: CfnWorkGroupProps) {
    super(scope, id, props);
    // Work around issue with the way Tags are encoded
    // Tags needs to be a non-empty array, but the default CfnWorkGroup creates an empty object `{}`
    // This empty object fails the request
    this.addPropertyOverride('Tags', [{ key: 'foo', value: 'bar', }]);
  }
}

@udondan
Copy link
Contributor Author

udondan commented Mar 28, 2020

I love the name of your class 😂

@iliapolo
Copy link
Contributor

@udondan Interesting, since the code you are using is auto-generated, it implies either a bug in the CloudFormation spec, or in our code generator.

The documented CFN spec seems correct: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-athena-workgroup-tags.html

We will take a look. Thanks for reporting this!

@iliapolo iliapolo added p2 p1 and removed p2 labels Mar 29, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label May 19, 2020
@blimmer
Copy link
Contributor

blimmer commented Jun 16, 2020

I also ran into this today. Instead of creating a new construct, I just did it in inline.

const workgroup = new athena.CfnWorkGroup(this, "AthenaWorkGroup", {
  name: "my-workgroup",
  description: "my real description goes here",
});
// Workaround for a cdk bug. We can remove this when this issue is resolved:
// https://github.com/aws/aws-cdk/issues/6936
workgroup.addPropertyOverride("Tags", [{ key: "foo", value: "bar" }]);

@udondan
Copy link
Contributor Author

udondan commented Jun 30, 2020

@l8on @blimmer Is this still working for you? I just came back to my Athena project and now this seems to be broken beyond fix via overrides. I'm getting

Model validation failed (... expected type: Boolean, found: String

For RecursiveDeleteOption and RequesterPaysEnabled. The template for sure contains booleans, so there is nothing to override. Even when those are not set, RecursiveDeleteOption is automatically set to true.

@blimmer
Copy link
Contributor

blimmer commented Jul 1, 2020

The tags override I described in this post is still working for me as of 1.47.1.

@udondan
Copy link
Contributor Author

udondan commented Jul 3, 2020

@iliapolo I checked the CFN spec and indeeed it is wrong.

It should be

    "AWS::Athena::WorkGroup.Tags": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-athena-workgroup-tags.html#cfn-athena-workgroup-tags-tags",
      "UpdateType": "Mutable",
      "Required": false,
      "Type": "List",
          "ItemType": "Tag"
    },

instead of

    "AWS::Athena::WorkGroup.Tags": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-athena-workgroup-tags.html",
      "Properties": {
        "Tags": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-athena-workgroup-tags.html#cfn-athena-workgroup-tags-tags",
          "UpdateType": "Mutable",
          "Required": false,
          "Type": "List",
          "ItemType": "Tag"
        }
      }
    },

Nothing wrong with the generator.

@moofish32
Copy link
Contributor

moofish32 commented Jul 14, 2020

@SomayaB or @iliapolo is there an update about when the spec will be corrected? If not I can modify the generator for this exception case. @rix0rrr remember when we had the idea of "patching" the cfn spec? Sometimes I wish we still had that.

@iliapolo
Copy link
Contributor

@moofish32 You mean this sort of patching? b59aed0#diff-cf42064b222db2a3e52591d8d39203b0

We still do that from time to time, would you like to have a go at it?

@moofish32
Copy link
Contributor

yep -- let me take a look.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jul 16, 2020
@mergify mergify bot closed this as completed in #9085 Aug 3, 2020
mergify bot pushed a commit that referenced this issue Aug 3, 2020
Since Athena does not have AWS constructs the tests are empty. What does
the team think about me adding one test to verify this patch is
correctly applied for the cfn generated constructs?

Can I also get feedback on the file name choice I made or a pointer to
conventions on the patch file names?

Fixes #6936

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this issue Aug 11, 2020
Since Athena does not have AWS constructs the tests are empty. What does
the team think about me adding one test to verify this patch is
correctly applied for the cfn generated constructs?

Can I also get feedback on the file name choice I made or a pointer to
conventions on the patch file names?

Fixes aws#6936

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-athena Related to AWS Athena bug This issue is a bug. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants