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

(apigateway): support deployment salting for imported RestApi #12417

Closed
taoatmars opened this issue Jan 8, 2021 · 11 comments
Closed

(apigateway): support deployment salting for imported RestApi #12417

taoatmars opened this issue Jan 8, 2021 · 11 comments
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway effort/medium Medium work item – several days of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. p1

Comments

@taoatmars
Copy link

taoatmars commented Jan 8, 2021

If the RestApi is define by RestApi.fromRestApiAttributes(), any deployment will not add salt to the deployment resource.

Reproduction Steps

This is my code.

const api = RestApi.fromRestApiAttributes(this, 'ImportedApi', {
      restApiId: XXX,
      rootResourceId: XXX,
    });

    const allResources = api.root.addResource('cdk');

    allResources.addCorsPreflight({ allowOrigins: Cors.ALL_ORIGINS });

    const method = allResources.addMethod('PUT', ifn, {
      methodResponses: [
        {
          statusCode: '200',
          responseParameters: {
            'method.response.header.Access-Control-Allow-Origin': true,
          },
        },
      ],
      authorizationType: AuthorizationType.IAM,
    });

    // Handle deployment
    const deployment = new Deployment(this, '123_deployment', {
      api: RestApi.fromRestApiId(this, 'abc', apiCDKApiApiId),
    });
    new Stage(this, 'CDKApiStageDev', { deployment, stageName: `${ENV}` });

"taotest123deploymentC642C343" is the resource name in the Cloudformation template.

What did you expect to happen?

add salt to the resource name

What actually happened?

no salt was added to the resource name

Environment

  • **CDK CLI Version :1.83
  • Framework Version:
  • Node.js Version:
  • **OS :MAC
  • **Language (Version):TypeScript

Other

WORKAROUND: Just in case anyone ran into the same problem, you can force a deployment by manually adding salt to deployment id. in above example, 123_deployment is my deployment id, you can change the id 1234_deployment to force new deployment to deploy.


This is 🐛 Bug Report

@taoatmars taoatmars added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 8, 2021
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Jan 8, 2021
@nija-at
Copy link
Contributor

nija-at commented Jan 12, 2021

Unfortunately, this isn't easy to implement since import APIs are intended to create API Gateway resources that are unaware of others. This is how we are able to split off these resources into different stacks and not cause cyclic dependencies.

Manually salting the deployment (as described in your workaround) is the best option available.

Moving this to a feature request.

@nija-at nija-at changed the title (apigateway): new deployment does not work on imported RestApi (apigateway): support deployment salting for imported RestApi Jan 12, 2021
@nija-at nija-at added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 effort/large Large work item – several weeks of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. effort/large Large work item – several weeks of effort labels Jan 12, 2021
@taoatmars taoatmars reopened this Jan 17, 2021
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@tmclaugh
Copy link

tmclaugh commented Mar 8, 2021

Per #13383, if latestDeployment were to be set when calling RestApi.fromRestApiAttributes() this issue should be resolved without even needing to create a Deployment resource manually within the stack.

@nija-at
Copy link
Contributor

nija-at commented Mar 22, 2021

Hi @tmclaugh - I'm keen to explore your statement here.

The addToLogicalId() method is the key to determine when a Deployment's logical id should be modified. You can see all the uses of this method that tells the Deployment when it should be updated.

Let's take the simple case where a RestApi is defined in one stack along with a number of Resources and Methods, and later imported into another stack using the RestApi.fromRestApiAttributes() where further Methods and Resources are defined.
Unless deploy is unset, the first stack would create a Deployment construct set to latestDeployment. However, it would not be possible for this Deployment construct to know of the Methods and Resources in the second stack and therefore cannot correctly determine when its logical id should be mutated.

How do you propose your approach tackle this case?

@montoyan877
Copy link

Any news on this topic?

@Durgaprasad-Budhwani
Copy link

Durgaprasad-Budhwani commented Nov 11, 2022

Hey Team,

I am using the below code which is working for me.

    const deployment = new Deployment(this, `deployment-${new Date().toISOString()}`, { api: apiGateway });

    // @ts-ignore
    deployment.resource.stageName = ENV!;

    const logicalId = Date.now().toString();// salted id
    const cfnDeployment = deployment.node.defaultChild as CfnDeployment;
    cfnDeployment.overrideLogicalId(logicalId);
    

I am doing deployment two times to make it work.
In the first deployment, it is creating resources and in the second deployment, it is updating stage.

This will be like

          npx aws-cdk synth
          npx aws-cdk deploy --require-approval never
          npx aws-cdk deploy --require-approval never       

@rodrigomata
Copy link
Contributor

Hey Team,

I am using the below code which is working for me.

    const deployment = new Deployment(this, `deployment-${new Date().toISOString()}`, { api: apiGateway });

    // @ts-ignore
    deployment.resource.stageName = ENV!;

    const logicalId = Date.now().toString();// salted id
    const cfnDeployment = deployment.node.defaultChild as CfnDeployment;
    cfnDeployment.overrideLogicalId(logicalId);
    

I am doing deployment two times to make it work. In the first deployment, it is creating resources and in the second deployment, it is updating stage.

This will be like

          npx aws-cdk synth
          npx aws-cdk deploy --require-approval never
          npx aws-cdk deploy --require-approval never       

Just ran into this issue, deploying two times does the trick but doesn't seem ideal to me, neither creating a separate Stage when the underlying issue seems to be related to the interface of the imported RestApi being IRestApi.

@nicks6853
Copy link

+1 to this. It's the only reason that is preventing us from using AWS CDK for projects at work. Creating a new deployment every time by changing the deployment id seems overkill.

@otaviomacedo
Copy link
Contributor

I don't see how we can solve this without doing hacks. The core problem, as @nija-at explained, is that, by importing a Rest API, you don't have access to its Deployment.

I'm closing this issue for now, but if someone has an idea for a solution, please share it with us, either by creating a new ticket or submitting a PR, and we may come back to this.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@robert-pitt-foodhub
Copy link

robert-pitt-foodhub commented Aug 15, 2023

I overcome this by overwriting the _latestDeployment variable on the RestApi, this needs to be done prior to adding any methods.

  //...
  /**
   * Initialise the rest api
   */
  public readonly restApi = new RestApi(this, 'api', {
    restApiName: `falcon-${this.props.envName}-poshub`,
    deploy: false,
    defaultCorsPreflightOptions,
  })

  /**
   * Documentation Version
   */
  public readonly version = new CfnDocumentationVersion(this, 'version', {
    documentationVersion: '1.1.44',
    restApiId: this.restApi.restApiId,
    description: 'this is a test of documentation',
  })

  /**
   * Rest API Deployment
   */
  public readonly deployment = new Deployment(this, 'deployment', {
    api: this.restApi,
    description: `Deployment: ${this.props.envName}/${this.version.documentationVersion}`,
  })

  // This patch needs to be applied prior to mounting the ApiMethods construct
  // This being present on the restApi allows for all methods to be bound to the deployment
  protected forceLatestDeploymentOverride = (this.restApi['_latestDeployment'] =
    this.deployment)
  //...

This may work for imported reset api's as well but I haven't tested, this resolves two issues with my setup:

  1. Ensuring that the all the methods created in my API effect the deployments logical id to ensure redeployment when methods change
  2. Ensuring that all methods are bound to the deployment as a dependancy, this will ensure that my post deployment constructs to extract documetation run against the very latest deployment.

Hope this helps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway effort/medium Medium work item – several days of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. p1
Projects
None yet
Development

No branches or pull requests

10 participants