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

Non-Routable Swagger #2

Closed
AlskiOnTheWeb opened this issue Jun 13, 2017 · 16 comments · Fixed by #11
Closed

Non-Routable Swagger #2

AlskiOnTheWeb opened this issue Jun 13, 2017 · 16 comments · Fixed by #11

Comments

@AlskiOnTheWeb
Copy link

Hello,

It appears that the Swagger/OpenAPI definitions you generate for the AWS APIs are "client side only". We have a need for a "server side" version. The difference can be seen in APIs like ACM where the Swagger is using the root (/) path with a number of hashes (/#DescribeCertificate) to differentiate the supplied JSON request body. Well, this hash-based path isn't "routable" in terms of a server as clients are really going to supply the hash, this is more a mechanism for you to overload the definition of the root path. Can you think of a way to make these Swagger files support both a client with the proper body but also the routable server? I realize that the Swagger specification does a real poor job of defining what the paths should be but to me, hashes are sort of abusing it.

Thanks.

@MikeRalphson
Copy link
Contributor

Hi @AlskiOnTheWeb. The OpenAPI definitions generated are a compromise in that many of the AWS definitions do overload paths as you say. Including fragment identifiers is an abuse of the OpenAPI specification (as the path object key should be a path, which does not allow the # character).

The intention wasn't to favour client-generation over server-generation (or documentation generation, mocking or anything else), but some mechanism had to be chosen so as to be able to represent the API described by the source definition. Creating API definitions which passed (multiple) existing validators was a concern.

Where you say:

Well, this hash-based path isn't "routable" in terms of a server as clients are really going to supply the hash

Do you mean "are really going to" or "aren't really going to"? My understanding is that the fragment identifier is never sent from client to server, so the server must interpret either the request body properties or the presence/absence of query-string or header parameters to decide which overloaded method was actually intended.

The real-world AWS API servers must be doing that inference today when talking to existing clients, so I think the definition is "routable", but possibly not compatible with the expectations of all server-generation tools.

It would theoretically be possible to merge all parameters and request body models of overloaded paths/operations into one large mega-operation, but at the cost of losing most of the expressivity of the API description. The logic to untangle the overloaded requests would still be necessary, only it would would be far harder to determine what the logic should actually be from the API definition alone.

If you could tell me a little more about the technologies you're using, perhaps we can come up with another solution, but with regards to OpenAPI 2.0 at least, I don't hold out much hope.

@AlskiOnTheWeb
Copy link
Author

Do you mean "are really going to" or "aren't really going to"? My understanding is that the fragment identifier is never sent from client to server, so the server must interpret either the request body properties or the presence/absence of query-string or header parameters to decide which overloaded method was actually intended.

Correct, "aren't" was my intention.

The real-world AWS API servers must be doing that inference today when talking to existing clients, so I think the definition is "routable", but possibly not compatible with the expectations of all server-generation tools.

It looks like they mostly trigger rather than infer on the "X-Amz-Target" header that isn't properly modeled (should be an enum?) in the Swagger.

POST / HTTP/1.1
Host: acm.us-east-1.amazonaws.com
X-Amz-Target: CertificateManager.DeleteCertificate
X-Amz-Date: 20151222T164207Z
User-Agent: aws-cli/1.9.7 Python/2.7.3 Linux/3.13.0-73-generic botocore/1.3.7
Content-Type: application/x-amz-json-1.1
Authorization: AWS4-HMAC-SHA256 Credential=AKIAIOSFODNN7EXAMPLE/20151222/us-east-1/acm/aws4_request, SignedHeaders=content-type;host;user-agent;x-amz-date;x-amz-target, Signature=0b29b04bb5f1ebb5fe9e6b1cbcdeda903b4ed2e06f3abe8a092c0ed1193b4dfc
{
"CertificateArn": "arn:aws:acm:us-east-1:111122223333:certificate/12345678-1234-1234-1234-123456789012"
}

@MikeRalphson
Copy link
Contributor

MikeRalphson commented Jun 13, 2017

It looks like they mostly trigger rather than infer on the "X-Amz-Target" header that isn't properly modeled (should be an enum?) in the Swagger.

Interesting, unfortunately it seems there is some inconsistency in the formatting of the x-amz-target header, it sometimes requires the API version...

X-Amz-Target: DynamoDB_20120810.GetItem

Though that is present in the targetPrefix metadata property:

  "metadata":{
    "apiVersion":"2012-08-10",
    "endpointPrefix":"dynamodb",
    "jsonVersion":"1.0",
    "protocol":"json",
    "serviceAbbreviation":"DynamoDB",
    "serviceFullName":"Amazon DynamoDB",
    "signatureVersion":"v4",
    "targetPrefix":"DynamoDB_20120810",
    "uid":"dynamodb-2012-08-10"
  }

It doesn't seem to be 'globally' documented. As with the signing logic, there is a level of 'known magic' required to access AWS APIs which can't always be expressed in an OpenAPI definition.

Would it help you if the x-amz-target header was defined with a default / single-value enum per operation?

@AlskiOnTheWeb
Copy link
Author

Not really. The duplicate / overlapping paths is more the real issue on my end. I'm just not sure what to do. I think I'm going to try and collapse them and do something less intelligent when I see them. As you said, this stuff doesn't model in OpenAPI...which is part of why I dislike OpenAPI. They wrote it for greenfield usage, not for real world usage.

@MikeRalphson
Copy link
Contributor

If you think it would be easier to collapse the source definition before it is converted to OpenAPI I would happily accept a PR (controlled by a command-line option) :)

@AlskiOnTheWeb
Copy link
Author

OK, I'll take a look to see if there, at the end of the day, is a way I can make it reasonably consistent.

@pimterry
Copy link
Contributor

I'm considering taking a look at this. I haven't got deeply into it yet, but I think in theory we should be able to solve this with OpenAPI 3, by heavily discriminating schemas everywhere based on the Action parameter: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#discriminator-object.

That would mean:

  • One path at /, one operation per method
  • Non-schema properties (description etc) have to be generic for all actions I think, you can't discriminate them.
  • An Action param, as an enum between the list of valid action strings
  • A params param, as an exploded form query parameter (based on Any of - while selecting the query parameters in Swagger using Open API 3.0.0 OAI/OpenAPI-Specification#1500 (comment)), discriminated to a different schema for each action.
    • This means OpenAPI thinks of it as one parameter, which an object type, that has a different schema with different props for each action
    • By making it an exploded form query param, when it's serialized/deserialized to a query string you break out a separate query parameter for each key of the top level object (i.e. the top level keys become like any other separate parameters)
  • One response per status code
  • Response content discriminated by action, giving a different schema for each action.

This sounds a bit mad, but in theory it accurately represents what's going on here, and any OpenAPI 3 compatible parser should be able to build a full & correct JSON schema for any request (as a client or a server), and can then validate against that correctly. The main downside is that some fields can't be discriminated, but that mainly just affects the path/operation descriptions. It's annoying, but I can't see a nice way around it.

I've put together a quick example below based on the existing cloudformation schema, for just two actions (with the schemas for each action's individual params omitted). Does it make sense? Do you think it'd work? I'm not too sure how to test this.

openapi: 3.0.0
info:
  version: '2010-05-15'
  title: AWS CloudFormation
paths:
  /:
    post:
      operationId: POST
      parameters:
        - $ref: '#/components/parameters/Action'
        - in: query
          name: params
          required: true
          style: form
          explode: true
          schema:
            oneOf:
              - $ref: '#/components/schemas/ContinueUpdateRollbackParams'
              - $ref: '#/components/schemas/CreateChangeSetParams'
            discriminator:
              propertyName: Action
              mapping:
                ContinueUpdateRollback: '#/components/schemas/ContinueUpdateRollbackParams'
                CreateChangeSet: '#/components/schemas/CreateChangeSetParams'
      responses:
        '200':
          description: Success
          content:
            '*/*':
              schema:
                oneOf:
                  - $ref: '#/components/schemas/ContinueUpdateRollbackOutput'
                  - $ref: '#/components/schemas/CreateChangeSetOutput'
                discriminator:
                  propertyName: Action
                  mapping:
                    ContinueUpdateRollback: '#/components/schemas/ContinueUpdateRollbackOutput'
                    CreateChangeSet: '#/components/schemas/CreateChangeSetOutput'
servers:
  - url: 'https://cloudformation.amazonaws.com/'
components:
  parameters:
    Action:
      name: Action
      in: query
      required: true
      schema:
        type: string
        enum:
          - ContinueUpdateRollback
          - CreateChangeSet
  schemas:
    ContinueUpdateRollbackOutput:
      type: object
      properties: {}
      description: The output for a <a>ContinueUpdateRollback</a> action.
    ContinueUpdateRollbackParams:
      type: object
      required:
        - StackName
      properties:
        StackName:
          $ref: '#/components/schemas/StackNameOrId'
        RoleARN:
          $ref: '#/components/schemas/RoleARN'
        ResourcesToSkip:
          $ref: '#/components/schemas/ResourcesToSkip'
        ClientRequestToken:
          $ref: '#/components/schemas/ClientRequestToken'
      description: The input for the <a>ContinueUpdateRollback</a> action.
    CreateChangeSetOutput:
      type: object
      properties:
        Id:
          $ref: '#/components/schemas/ChangeSetId'
        StackId:
          $ref: '#/components/schemas/StackId'
      description: The output for the <a>CreateChangeSet</a> action.
    CreateChangeSetParams:
      type: object
      required:
        - StackName
        - ChangeSetName
      properties:
        StackName:
          $ref: '#/components/schemas/StackNameOrId'
        TemplateBody:
          $ref: '#/components/schemas/TemplateBody'
        TemplateURL:
          $ref: '#/components/schemas/TemplateURL'
        UsePreviousTemplate:
          $ref: '#/components/schemas/UsePreviousTemplate'
        Parameters:
          $ref: '#/components/schemas/Parameters'
        Capabilities:
          $ref: '#/components/schemas/Capabilities'
        ResourceTypes:
          $ref: '#/components/schemas/ResourceTypes'
        RoleARN:
          $ref: '#/components/schemas/RoleARN'
        RollbackConfiguration:
          $ref: '#/components/schemas/RollbackConfiguration'
        NotificationARNs:
          $ref: '#/components/schemas/NotificationARNs'
        Tags:
          $ref: '#/components/schemas/Tags'
        ChangeSetName:
          $ref: '#/components/schemas/ChangeSetName'
        ClientToken:
          $ref: '#/components/schemas/ClientToken'
        Description:
          $ref: '#/components/schemas/Description'
        ChangeSetType:
          $ref: '#/components/schemas/ChangeSetType'
      description: The input for the <a>CreateChangeSet</a> action.

@pimterry
Copy link
Contributor

Alternatively OAI/OpenAPI-Specification#182 would solve this, and it sounds like that still an option for OpenAPI 3.1 (OAI/OpenAPI-Specification#1466). Presumably that's a way off though, so a short-term workaround like this would be nice.

@MikeRalphson
Copy link
Contributor

We're going to discuss that in an upcoming OpenAPI TSC meeting.

3.1 is hopefully due in May, and this issue has been open much longer than 3 months.

Maybe the first step should be migrating the converter to OAS v3?

@MikeRalphson
Copy link
Contributor

Couple of questions re: your first proposal @pimterry

One path at /, one operation per method

Is this always (necessarily) the case for every AWS API with this problem, or should each set of 'conflicting' operations be coalesced under whatever path value makes sense?

Response content discriminated by action, giving a different schema for each action.

If action is a parameter or request header, can you use it in a discriminator object? Wouldn't the action have to be returned by AWS in each response? Is that the case?

Maybe allowing callback-style runtime expressions in discriminator objects would be a useful extension? Then they could point back to the request... But that would be another proposed change to OAS 3.x

@pimterry
Copy link
Contributor

Is this always (necessarily) the case for every AWS API with this problem, or should each set of 'conflicting' operations be coalesced under whatever path value makes sense?

I'm not super familiar with the AWS APIs, so I'm not totally sure, but it does look like in practice many of them have many operations which all share one single path, with with an action param. It's certainly not necessarily always one operation though, I'm just suggesting collapsing any conflicting operations like this.

If action is a parameter or request header, can you use it in a discriminator object? Wouldn't the action have to be returned by AWS in each response? Is that the case?

Hmm, yes, I think I've misinterpreted this. I was hoping that you could discriminate responses on request params, but I don't think that's true.

From the AWS APIs themselves, I think you can always find a part of the response to discriminate from instead, but it is more complicated. E.g. CreateStackInstances returns an XML body with a CreateStackInstancesResponse root element. I'm not sure if that's easily matchable, but in theory that might work.

Maybe allowing callback-style runtime expressions in discriminator objects would be a useful extension? Then they could point back to the request... But that would be another proposed change to OAS 3.x

I'm a bit cautious about runtime expressions, rather than generic pattern matching in some form. I think one nice way to improve all this would be:

  • Allow the discriminator to be some kind of path into the request/response details, e.g. a response body discriminator like #/request/parameters/Action, to discriminate the body schema based on an input parameter.
  • Make it possible to discriminate larger parts of the spec (e.g. make more of the spec follow JSON schema rules somehow), so we can discriminate a whole operation at one, and thereby have different descriptions based on specific parameters values. Does that make sense?

@MikeRalphson
Copy link
Contributor

Allow the discriminator to be some kind of path into the request/response details

Sorry, that's exactly what I meant by "runtime expression" as used in the OAS:

Runtime expressions allow defining values based on information that will only be available within the HTTP message in an actual API call. This mechanism is used by Link Objects and Callback Objects.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#runtimeExpression

@pimterry
Copy link
Contributor

pimterry commented Mar 1, 2019

I've been thinking more about this. Some kind of runtime expression discriminator would be good, but for my case at least it'd be very useful to discriminate the entire operation (the description especially). Long-term OpenAPI spec changes might help there, but I'd love to come up with a short term solution that makes it possible to route/match requests in the meantime, under the current spec.

Right now the specs here are not only not automatically routed by existing tools, it's effectively impossible to build or change tools to start routing these correctly without a special case for each spec here, as there's no info on how to decide which #Value path is relevant.

I think the algorithm to find the relevant path for a request is usually to match the Action param to the name of an action, or in some cases to match the X-Amx-Target header. Does that sound correct?

Given that, would you be open to updating the format to something like this:

openapi: 3.0.0
info:
  version: '2010-05-15'
  title: AWS CloudFormation
paths:
  /#Action=CreateChangeSetParams:
    post:
      operationId: CreateChangeSetParams
      parameters:
        - $ref: '#/components/parameters/Action'
      responses:
        '200':
          description: Success
  /#Action=ContinueUpdateRollbackParams:
    post:
      operationId: ContinueUpdateRollbackParams
      parameters:
        - $ref: '#/components/parameters/Action'
      responses:
        '200':
          description: Success

The only real difference is the Action= in each fragment. The idea here is that each fragment could be in the format of a key-value query string, and the relevant path is the one where the corresponding named parameter of that operation matches the value in the query string. That means you can match action params, as here, or header params etc too (by defining a header parameter for the operation that has the corresponding name).

This still validates just like the existing specs you're producing here, and existing tools should work fine (ignoring fragments and treating the paths as ambiguous), but it'd also be immediately possible to add some easy logic that can automatically route these requests. In future, when there's formal spec support for some kind of operation discriminator, it should be easy to convert this format into an officially supported runtime expression matching approach, and in the meantime it's much simpler than my previous suggestion.

What do you think? If that format would be acceptable, I'd be happy to investigate opening a PR here for this change.

@MikeRalphson
Copy link
Contributor

Yes, I think the explicit Action= key provides some benefits of clarity, rather than relying on the operationId. I'd be very happy to see a PR. Just let me know whether you want to base it on the OpenAPI v2 format as now, or you want me to update the 'template' to OpenAPI v3, or whether you're ok doing that too (in a separate PR)?

@pimterry
Copy link
Contributor

pimterry commented Mar 8, 2019

Ok, great, thanks! I'll try and put a PR together soon, probably next week.

I think OpenAPI v2 is fine for now, it's probably better to do that change independently.

@MikeRalphson
Copy link
Contributor

@AlskiOnTheWeb feel free to re-open this if it doesn't fix your issue or you can see further changes that are required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants