SerializeError for a ResourceStatus in DescribeStackEventsResponse

[jdreaver]

Hello!

I started using amazonka to deploy our CloudFormation templates, and it's been awesome so far. However, I've hit a small snag. I'm getting a really long serialization error, but the relevant part is this (I'm using markdown quotes because verbatim makes a really long line):

_serializeMessage = "Failed reading: Failure parsing ResourceStatus from value: 'update_complete_cleanup_in_progress'. Accepted values: create_complete, create_failed, create_in_progress, delete_complete, delete_failed, delete_in_progress, delete_skipped, update_complete, update_failed, update_in_progress"

It seems that the ResourceStatus enum does not include all the possible values for resource statuses. It looks like the official schema got this wrong too. This ResourceStatus field can also include anything from StackStatus enum because when the stack changes phases (for example, enters the cleanup phase) CloudFormation will emit an event on the current AWS::CloudFormation::Stack "resource" with the status update.

Here is an example from one of my stacks:

As you can see, there is the offending UPDATE_COMPLETE_CLEANUP_IN_PROGRESS in the middle.

Obviously AWS should update their official schema, but is there anything I can do in the meantime to work around this? Is there any way I can make a newtype wrapper or something around the request type so I can make a custom response type as an escape hatch? If so, are there any examples of that?

Should I submit a PR with a temporary escape hatch like adding a constructor to ResourceStatus like UnknownResourceStatus Text?

Thanks again for your work on amazonka and for any help you can provide!

[jdreaver]

If anyone is interested, I have the following vendored code in my codebase that copies much of what amazonka has done, except it replaces StackEvent with a custom CustomStackEvent that uses Text instead of ResourceStatus. It also has new request/response types so it's possible to use this new stack event type.

{-# LANGUAGE DeriveDataTypeable #-}
{-# LANGUAGE DeriveGeneric #-}
-- {-# LANGUAGE StandaloneDeriving #-}
{-# LANGUAGE TypeFamilies #-}

-- | Temporary module for amazonka fixes until they are fixed upstream.

module Network.AWS.CloudFormation.Fixes
  ( DescribeStackEvents'(..)
  , DescribeStackEventsResponse'(..)
  , dserspNextToken
  , dserspStackEvents
  , dserspResponseStatus
  , CustomStackEvent(..)
  , cseLogicalResourceId
  , csePhysicalResourceId
  , cseResourceType
  , cseResourceStatusReason
  , cseResourceProperties
  , cseResourceStatus
  , cseClientRequestToken
  , cseStackId
  , cseEventId
  , cseStackName
  , cseTimestamp
  ) where

import Network.AWS.CloudFormation
import Network.AWS.Lens
import Network.AWS.Pager
import Network.AWS.Prelude
import Network.AWS.Request
import Network.AWS.Response

-- | The 'ResourceStatus' enum included with amazonka intends to describe all
-- the possible states a CloudFormation resource can be in. It is used in the
-- DescribeStackEvents API as well so when an event comes in you can see the
-- resource's current state.
--
-- However, CloudFormation also includes states from the 'StackStatus' enum.
-- When we have a status that isn't in 'ResourceStatus', like
-- UPDATE_COMPLETE_CLEANUP_IN_PROGRESS, amazonka simply fails to parse it!
--
-- Below we make new types for this API and simply make ResourceStatus a 'Text'
-- value so we don't have to worry about this.
--
-- See https://github.com/brendanhay/amazonka/issues/418

-- | Wrapper around 'DescribeStackEvents'. Used so we can make a new instance
-- of 'AWSRequest'.
newtype DescribeStackEvents' = DescribeStackEvents' { unDescribeStackEvents' :: DescribeStackEvents }
  deriving (Eq, Read, Show, Data, Typeable, Generic, Hashable, NFData, ToHeaders, ToPath)

instance AWSPager DescribeStackEvents' where
  page (DescribeStackEvents' rq) rs
    | stop (rs ^. dserspNextToken) = Nothing
    | stop (rs ^. dserspStackEvents) = Nothing
    | otherwise =
      Just $ DescribeStackEvents' $ rq & dseNextToken .~ rs ^. dserspNextToken

instance AWSRequest DescribeStackEvents' where
  type Rs DescribeStackEvents' = DescribeStackEventsResponse'
  request = postQuery cloudFormation
  response =
    receiveXMLWrapper "DescribeStackEventsResult"
    (\s _ x ->
        DescribeStackEventsResponse' <$>
        (x .@? "NextToken") <*>
        (x .@? "StackEvents" .!@ mempty >>=
          may (parseXMLList "member"))
      <*> pure (fromEnum s))

instance ToQuery DescribeStackEvents' where
  toQuery (DescribeStackEvents' x) = toQuery x

-- | Alternative to 'DescribeStackEventsResponse' that allows us to use our
-- custom 'CustomStackEvent' type.
data DescribeStackEventsResponse'
  = DescribeStackEventsResponse'
  { _dserspNextToken :: !(Maybe Text)
  , _dserspStackEvents :: !(Maybe [CustomStackEvent])
  , _dserspResponseStatus :: !Int
  } deriving (Eq, Read, Show, Data, Typeable, Generic)

-- deriving instance NFData DescribeStackEventsResponse'

dserspNextToken :: Lens' DescribeStackEventsResponse' (Maybe Text)
dserspNextToken = lens _dserspNextToken (\ s a -> s{_dserspNextToken = a})

dserspStackEvents :: Lens' DescribeStackEventsResponse' [CustomStackEvent]
dserspStackEvents = lens _dserspStackEvents (\ s a -> s{_dserspStackEvents = a}) . _Default . _Coerce

dserspResponseStatus :: Lens' DescribeStackEventsResponse' Int
dserspResponseStatus = lens _dserspResponseStatus (\ s a -> s{_dserspResponseStatus = a})

data CustomStackEvent
  = CustomStackEvent
  { _cseLogicalResourceId :: !(Maybe Text)
  , _csePhysicalResourceId :: !(Maybe Text)
  , _cseResourceType :: !(Maybe Text)
  , _cseResourceStatusReason :: !(Maybe Text)
  , _cseResourceProperties :: !(Maybe Text)
  , _cseResourceStatus :: !(Maybe Text)
    -- ^ This is the reason this file exists
  , _cseClientRequestToken :: !(Maybe Text)
  , _cseStackId :: !Text
  , _cseEventId :: !Text
  , _cseStackName :: !Text
  , _cseTimestamp :: !ISO8601
  } deriving (Eq,Read,Show,Data,Typeable,Generic)

cseLogicalResourceId :: Lens' CustomStackEvent (Maybe Text)
cseLogicalResourceId = lens _cseLogicalResourceId (\ s a -> s{_cseLogicalResourceId = a})

csePhysicalResourceId :: Lens' CustomStackEvent (Maybe Text)
csePhysicalResourceId = lens _csePhysicalResourceId (\ s a -> s{_csePhysicalResourceId = a})

cseResourceType :: Lens' CustomStackEvent (Maybe Text)
cseResourceType = lens _cseResourceType (\ s a -> s{_cseResourceType = a})

cseResourceStatusReason :: Lens' CustomStackEvent (Maybe Text)
cseResourceStatusReason = lens _cseResourceStatusReason (\ s a -> s{_cseResourceStatusReason = a})

cseResourceProperties :: Lens' CustomStackEvent (Maybe Text)
cseResourceProperties = lens _cseResourceProperties (\ s a -> s{_cseResourceProperties = a})

cseResourceStatus :: Lens' CustomStackEvent (Maybe Text)
cseResourceStatus = lens _cseResourceStatus (\ s a -> s{_cseResourceStatus = a})

cseClientRequestToken :: Lens' CustomStackEvent (Maybe Text)
cseClientRequestToken = lens _cseClientRequestToken (\ s a -> s{_cseClientRequestToken = a})

cseStackId :: Lens' CustomStackEvent Text
cseStackId = lens _cseStackId (\ s a -> s{_cseStackId = a})

cseEventId :: Lens' CustomStackEvent Text
cseEventId = lens _cseEventId (\ s a -> s{_cseEventId = a})

cseStackName :: Lens' CustomStackEvent Text
cseStackName = lens _cseStackName (\ s a -> s{_cseStackName = a})

cseTimestamp :: Lens' CustomStackEvent UTCTime
cseTimestamp = lens _cseTimestamp (\ s a -> s{_cseTimestamp = a}) . _Time

instance FromXML CustomStackEvent where
  parseXML x
    = CustomStackEvent <$>
      (x .@? "LogicalResourceId") <*>
      (x .@? "PhysicalResourceId")
      <*> (x .@? "ResourceType")
      <*> (x .@? "ResourceStatusReason")
      <*> (x .@? "ResourceProperties")
      <*> (x .@? "ResourceStatus")
      <*> (x .@? "ClientRequestToken")
      <*> (x .@ "StackId")
      <*> (x .@ "EventId")
      <*> (x .@ "StackName")
      <*> (x .@ "Timestamp")

[brendanhay]

Apologies @jdreaver - and thanks alot for the detail and workaround.

This is fixed in #459 via an updated amazonka-cloudformation which now supports the additional enum branches. Regarding an escape hatch - it's been previously discussed to add the ...Unknown branch to all generated enums - I just haven't tracked the work but I think it's likely the most convenient/future-proof approach and worth implementing to avoid issues like this.

[jdreaver]

Thanks so much @brendanhay!

Indeed, I think an Unknown escape hatch is necessary for APIs like AWS that can evolve or change without much notice. It would be a shame to have a Haskell library not work because AWS added a feature and an older version of the library doesn't work at all because it lacked an escape hatch like Unknown. (This exact situation happened with me while working on stratoshere. AWS added a new Lambda runtime and my enum didn't support it.)

[dudebout]

I was about to open an issue suggesting the Unknown escape hatch as well. I ran a couple of times into the issue where running an instance from the Console or the CLI, of a type unsupported by amazonka, breaks seemingly safe invocations of DescribeInstances. @brendanhay if you have pointers about the work required, I'd be happy to take a stab at it.

[brendanhay]

Thanks for the offer @dudebout, I'll probably take it on myself due to worries about the stability of existing enum branch names (sum constructors.)

To illustrate - the renamer takes all of an AWS service definition's enum branches and attempts to disambiguate against a global (constructor) environment. Take the following examples of enums rendered to Haskell sums that the renamer sees:

data Foo
    = A
    | B
    | C

data Bar
    = C
    | D
    | E

In this case the renamer will rename all of Bars fields (for consistency) since the C constructor from Foo already exists. This results in:

data Foo
    = A
    | B
    | C

data Bar
    = BC -- BC, BarC, BARC, Bar_E, etc. could be chosen depending upon overlap.
    | BD
    | BE

The important piece is that the prefix is always the same for all of a sum's constructors. If one constructor overlaps, all constructors are renamed. Otherwise we end up with very hard to discover/intuit constructor names.

By adding an Unknown constructor, every enum now overlaps with each other, resulting in a completely different set of constructors for all enums, in every service. Obviously that's not ideal and there are some workarounds such as deliberately naming the Unknown as a prefix/suffix of the sum type's name or something else that breaks consistency with the other constructors, but I'd rather dig into it personally to try and ensure minimal breakage for users. Ideally the only breakage would be the non-exhaustive pattern matches from introducing the new constructor.

[charleso]

@brendanhay I don't want to derail this extremely important endeavour (it was the reason for Ambiata wanting amazonka upgrades 90% of the time). I just have one concern that I wanted to mention.

I do wonder slightly about forwards compatibility of adding the Unknown constructor, specifically around pattern matching. To illustrate, let's say you patterned matched on the unknown string type:

case status of
  SSUnknown "UPDATE_COMPLETE_CLEANUP_IN_PROGRESS" ->
    ...

And then at a future point in time you upgrade amazonka with that enum having being regenerated the code above would silently break.

Depending on how bad you think this is, the only vague suggestion I have is to newtype Text for all enum values, and provide conversion functions

newtype StackStatus = StackStatus Text
toStackStatusEnum :: StackStatus -> Maybe StackStatusEnum
fromStackStatusEnum :: StackStatusEnum -> StackStatus

This both clutters using the amazonka API and isn't backwards compatible, so I'm not a big fan of this idea.

Just something I've been thinking about.

[brendanhay]

Thanks for the input @charleso - the forwards compatibility of just adding Unknown everywhere is definitely wonky given your example. It's almost like the type in the slot of the Unknown constructor shouldn't have an Eq instance to prevent pattern matching. :sweat_smile: Wholly defeats the purpose of this proposed change, though.

Your suggested solution seems sound and covers both the desired forwards compatibility case and ensuring users can choose to pattern match on Text in the case of amazonka needing to be regenerated, or ensure they are exhaustively matching the valid 'known' patterns - but is definitely a step backwards from a UX perspective, I think.

[brendanhay]

Just observationally:

data Stack
    = SPartyAnimals
    | SDevBlops
    | UnknownStack !Text

is isomorphic to (minus the UnknownStack constructor):

type Stack' = Either Text Stack

Which seems to subsume the newtype StackStatus and toEnum :: StackStatus -> Maybe StackStatusEnum. It also obviates my concerns about the renamer.

[brendanhay]

Playing around, I have a branch that generates the following:

data ResourceStatus
    = CreateComplete
    | CreateFailed
    | CreateInProgress
    | DeleteComplete
    | DeleteFailed
    | DeleteInProgress
    | DeleteSkipped
    | UpdateComplete
    | UpdateFailed
    | UpdateInProgress
      deriving (Eq, Ord, Read, Show, Data, Typeable, Generic)

data StackResource = StackResource'
    { _srPhysicalResourceId   :: !(Maybe Text)
    , _srResourceStatusReason :: !(Maybe Text)
    , _srStackId              :: !(Maybe Text)
    , _srDescription          :: !(Maybe Text)
    , _srStackName            :: !(Maybe Text)
    , _srLogicalResourceId    :: !Text
    , _srResourceType         :: !Text
    , _srTimestamp            :: !ISO8601
    , _srResourceStatus       :: !(UnknownEnum ResourceStatus)
    } deriving (Eq, Read, Show, Data, Typeable, Generic)

stackResource
    :: Text -- ^ 'srLogicalResourceId'
    -> Text -- ^ 'srResourceType'
    -> UTCTime -- ^ 'srTimestamp'
    -> Either Text ResourceStatus -- ^ 'srResourceStatus'
    -> StackResource
stackResource pLogicalResourceId_ pResourceType_ pTimestamp_ pResourceStatus_ =
    StackResource'
        { _srPhysicalResourceId   = Nothing
        , _srResourceStatusReason = Nothing
        , _srStackId              = Nothing
        , _srDescription          = Nothing
        , _srStackName            = Nothing
        , _srLogicalResourceId    = pLogicalResourceId_
        , _srResourceType         = pResourceType_
        , _srTimestamp            = _Time # pTimestamp_
        , _srResourceStatus       = _UnknownEnum # pResourceStatus_
        }

srResourceStatus :: Lens' StackResource (Either Text ResourceStatus)
srResourceStatus = lens _srResourceStatus (\ s a -> s{_srResourceStatus = a})
    . _UnknownEnum

Where UnknownEnum resides in amazonka-core:

-- Exists internally for de/serialization purposes.
data UnknownEnum a = Unknown !Text | Known !a

_UnknownEnum :: Iso (UnknownEnum a) (Either Text a)
_UnknownEnum = ...

instance FromXML/JSON/Text/etc a => FromXML/JSON/Text/etc (UnknownEnum a) where
    parser = fmap Known parser <|> fmap Unknown parser

Subject to additional changes around parsing and less batshit naming.

[brendanhay]

Moving the discussion over to #460 :arrow_right: