botocore icon indicating copy to clipboard operation
botocore copied to clipboard

Cloudformation service-2.js is missing valid ResourceStatus values

Open mbj opened this issue 6 years ago • 12 comments

It appears that data/cloudformation/2010-05-15/service-2.json is missing the following enum values in the ResourceStatus type:

"ROLLBACK_COMPLETE"
"ROLLBACK_FAILED"
"ROLLBACK_IN_PROGRESS"
"UPDATE_COMPLETE_CLEANUP_IN_PROGRESS"
"UPDATE_ROLLBACK_COMPLETE"
"UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESS"
"UPDATE_ROLLBACK_FAILED"
"UPDATE_ROLLBACK_IN_PROGRESS"

Note that all of these are present in the StackStatus type.

But when DescribeStackEvents reports the status of the stack when the event resource is the stack itself: the stack status becomes the resource status.

I suspect this was not found so far as your internal generation of the service-2.json only consults the resource statuses, excluding the stack statuses. And many downstream libraries only expose the stack / resource status as strings. So it could slip through for a long time.

To reproduce the missing ResourceStatus enum values: Run the stack into any of the states missing from ResourceStatus and call DescribeStackEvents.

mbj avatar Aug 10 '19 00:08 mbj

@mbj - Thank you for your post. service-2.json file comes from upstream. And how the api works and which parameter it includes all those are determined by the service team. We can't change the parameter returned by any api call or modify it. First it should be updated on the service side before sdk can use it.

I will let our service team know about the issue .

swetashre avatar Aug 12 '19 23:08 swetashre

@sanathkr Just to avoid any miscommunication:

  • I do not request that you change the service-2.json unilaterally, getting upstream to fix the generation of service-2.json was my intention.
  • The service-2.json generated from upstream is currently a subset of the upstream behavior. Upstream allows values to be present in ResourceStatus that are not specified by service-2.json
  • I've given a reproduction vector to observe ResourceStatus values being returned that are currently being outside the spec of service-2.json.
  • After consulting the contribution guidelines here I opened this issue, as I read them in a way that say: Should service-2.json be incorrect, do not open a PR that fixes it, as we want it to be fixed upstream.

I suspect we are already in sync, but I want to be sure.

A question:

Is a report to the forum the better way to communicate bugs in service-2.json, should I avoid opening issues like this one in the future and go directly to the respective forum?

mbj avatar Aug 13 '19 00:08 mbj

@swetashre As you edited out your referral to the forums, can I conclude that opening the issue here was correct?

mbj avatar Aug 14 '19 10:08 mbj

AWS Forum is basically for service specific questions. We use this GitHub repo for tracking bug and feature request related to botocore.

I have already escalated this issue to the service team. I will post here if i get any update.

swetashre avatar Aug 14 '19 17:08 swetashre

@swetashre Thx.

mbj avatar Aug 14 '19 19:08 mbj

@swetashre Just checking in. Almost 12 month have passed. I still have to carry around a set of patches to patch either the API spec or the generated code to accept the unspecified cloudformation resource states. This comment is not a complaint, but an attempt to raise awareness.

In many languages developers have no other choice but to represent these as a string, but as I'm using Haskell, a relatively strongly typed language, and thus generate a strict sum type, my code will constantly find the issue again should I use the official API spec. This is likely also true for other typed languages. Not unique to Haskell.

I'd appreciate if you could reach out to the team again, and potentially ask them kindly to fix their reflection / update their spec.

I'd even sign an NDA and do it for free. Carrying around: https://github.com/brendanhay/amazonka/commit/c9c339e63efdd2c9443f1b3105d6aca34902f17e is unpleasant. And the low amount of hours to read your CF source / find and fix the reflection is likely cheaper for me than to carry this patch to my projects.

More than a decade of consulting let me even attempt predict the mistake your CF team made:

I assume that you use Java, and the type used to reflect the field contents for the API field ResourceStatus looks like this:

enum ResourceStatus {
  CREATE_COMPLETE,
  CREATE_FAILED,
  CREATE_IN_PROGRESS,
  DELETE_COMPLETE,
  DELETE_FAILED,
  DELETE_IN_PROGRESS,
  DELETE_SKIPPED
}

Which defines all values in the ResourceStatus field for regular resources. But should the resource to report in the stack events be the CF stack itself, not the java type ResourceStatus will get rendered to the API response field ResourceStatus but the status of the stack itself which is likely defined like this:

enum StackStatus {
  // some enums shared with ResourceStatus I deliberately skip here
  ROLLBACK_COMPLETE
  ROLLBACK_FAILED
  ROLLBACK_IN_PROGRESS
  UPDATE_COMPLETE_CLEANUP_IN_PROGRESS
  UPDATE_FAILED
  UPDATE_IN_PROGRESS
  UPDATE_ROLLBACK_COMPLETE
  UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESS
  UPDATE_ROLLBACK_FAILED
  UPDATE_ROLLBACK_IN_PROGRESS
}

Your code at the time of rendering the ResourceStatus API field will likely not notice, as you simply transform both Java types StackStatus and ResourceStatus to Strings. And this so far this bug could slip past your internal quality controls and even lots of SDKs, as they just trust your API and would use any string.

The fix would be to union (plus dedup) the reflected enum values of the java types ResourceStatus and StackStatus than render them to the ResourceStatus field in the events API respose.

My offer to sign an NDA and fix it by myself is no joke. Just hit me up via the email in my GH profile.

mbj avatar Jun 29 '20 20:06 mbj

Any updates on getting this fixed? ETA?

ITProKyle avatar Jun 10 '21 14:06 ITProKyle

@ITProKyle At the current rate: Heat death of the universe.

mbj avatar Jun 10 '21 16:06 mbj

@ITProKyle, @mbj,

Checking in here - I've requested an update from the CloudFormation team.

kdaily avatar Jun 10 '21 16:06 kdaily

P42150073

kdaily avatar Jun 10 '21 16:06 kdaily

Sorry to be so snarky earlier. Thanks for taking a look.

mbj avatar Jun 10 '21 19:06 mbj

@kdaily this can be closed. Looks like cloudformation team updated ResourceStatus.

vemel avatar Mar 28 '22 03:03 vemel

As indicated by the last comment, ResourceStatus has been updated; closing this issue.

RyanFitzSimmonsAK avatar Nov 09 '22 21:11 RyanFitzSimmonsAK