fix(apigateway): SpecRestApi ignores disableExecuteApiEndpoint property

[caldermc-at-amazon]

Fixes #21295

I came upon this bug when trying to create a SpecRestApi with its execute API endpoint disabled so that it could only be reached via a custom domain name. I set the disableExecuteApiEndpoint property to true (because it defaults to false) but the endpoint was still enabled. Upon diving deeper I realized this was due to an inconsistency: both the RestApi and SpecRestApi constructors create a CfnRestApi, but only the RestApi constructor passes along the property.

When looking at the commit that introduced the disableExecuteApiEndpoint property into the RestApiBaseProps (used by both RestApi and SpecRestApi) it seems like failing to pass it along to the CfnRestApi in the SpecRestApi constructor was simply an oversight. This commit fixes the issue by passing disableExecuteApiEndpoint into the CfnRestApi in the SpecRestApi constructor. I am not aware of any alternatives that I could have considered for this fix.

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[TheRealAmazonKendra]

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests (https://github.com/aws/aws-cdk/blob/v1-main/CONTRIBUTING.md#step-4-pull-request))

[caldermc-at-amazon]

Thank you, just updated the description. Please let me know if any additional information is required!

[TheRealAmazonKendra]

Thank you for your contribution. Just one small note below and we'll also need the conflicts resolved.

[TheRealAmazonKendra]

Let's also make sure that this properly sets this as false when it's false and that it's set as false when the property isn't set.

Pull request has been modified.

[caldermc-at-amazon]

Sorry for the delay, I have updated the diff to include true/false tests and resolve conflicts. Also it turns out that the CFN property should not be false (it will just be absent) when unspecified in the RestApi/SpecRestApi, so I removed the tests I originally added to check for that case.

[TheRealAmazonKendra]

This looks great, however, I'm concerned about the fact that this change doesn't break any integration tests. This means that we're either lacking test coverage or that it's not passing this property correctly to a template. I suspect the former. To ease my mind, please add an integration test that uses this prop.

[caldermc-at-amazon]

Sorry I'm not familiar with the process to do that. How would the integration test you're suggesting be any different than my tests in this pull request? To me the issue was a lack of explicit tests for this property in SpecRestApi, which I've now added. If you think this is a blocking concern please further elaborate on what you need changed. Thanks!

[TheRealAmazonKendra]

Here is our documentation on integration testing. These tests will actually perform the deployment of this template and ensure that the deployment succeeds. This both tests that our templates are written correctly and ensures that changes don't have unintended consequences when they're in other packages that are used by this package. They are extremely important and I consider them mandatory when we have a change like this that isn't detected by an existing integration test.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 4049d3a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[caldermc-at-amazon]

Hi @TheRealAmazonKendra, I've spent the last few days trying and failing to create/modify integration tests for the RestApi/SpecRestApi constructs. I'm sure with enough time I could figure it out, but even the existing integration tests are very minimalistic and don't cover true/false scenarios for most of the other fields. I appreciate insistence on high standards but in this case they are inconsistent because we recently allowed a nearly-identical change through without integration test coverage. Please provide that same exemption here because the alternative is leaving this bug in CDK knowing there is an outstanding one-line PR that fixes it. An effort to improve the robustness of the RestApi/SpecRestApi integration tests sounds like a great idea, but is unnecessary scope-creep and not something I have the time to get done myself.

[TheRealAmazonKendra]

Hi @TheRealAmazonKendra, I've spent the last few days trying and failing to create/modify integration tests for the RestApi/SpecRestApi constructs. I'm sure with enough time I could figure it out, but even the existing integration tests are very minimalistic and don't cover true/false scenarios for most of the other fields. I appreciate insistence on high standards but in this case they are inconsistent because we recently allowed a nearly-identical change through without integration test coverage. Please provide that same exemption here because the alternative is leaving this bug in CDK knowing there is an outstanding one-line PR that fixes it. An effort to improve the robustness of the RestApi/SpecRestApi integration tests sounds like a great idea, but is unnecessary scope-creep and not something I have the time to get done myself.

@caldermc-at-amazon Can you tell me more about what's causing the issue creating a test for this? I can help with troubleshooting/getting the test working. The fact that our tests are very minimalistic and inconsistent is precisely what concerns me.

Looking back at the previous exemption I provided, I think that I was in the wrong there. We have also been increasing our testing requirements on PRs recently so that one may have slipped in just before that change was made. Given that we only release once a week (usually on Thursdays but it can vary), we have some time to get tests in place before this would make it into a release anyway.

[caldermc-at-amazon]

Hi @TheRealAmazonKendra, I've spent the last few days trying and failing to create/modify integration tests for the RestApi/SpecRestApi constructs. I'm sure with enough time I could figure it out, but even the existing integration tests are very minimalistic and don't cover true/false scenarios for most of the other fields. I appreciate insistence on high standards but in this case they are inconsistent because we recently allowed a nearly-identical change through without integration test coverage. Please provide that same exemption here because the alternative is leaving this bug in CDK knowing there is an outstanding one-line PR that fixes it. An effort to improve the robustness of the RestApi/SpecRestApi integration tests sounds like a great idea, but is unnecessary scope-creep and not something I have the time to get done myself.

@caldermc-at-amazon Can you tell me more about what's causing the issue creating a test for this? I can help with troubleshooting/getting the test working. The fact that our tests are very minimalistic and inconsistent is precisely what concerns me.

Looking back at the previous exemption I provided, I think that I was in the wrong there. We have also been increasing our testing requirements on PRs recently so that one may have slipped in just before that change was made. Given that we only release once a week (usually on Thursdays but it can vary), we have some time to get tests in place before this would make it into a release anyway.

Sorry but again, I personally do not have additional cycles to spend on this. Gatekeeping one-line bug fixes is not a customer-obsessed mechanism to address technical debt. I agree that somebody should write a suite of thorough integration tests for this package, but that effort should be a decoupled "chore" task. In the context of this PR, CDK is strictly better with this change as-is than without it. If you'd prefer to leave this PR blocked on principle that's fine but someone else will need to continue it.

[TheRealAmazonKendra]

It's been over a month so I'm going to go ahead and close this, with a few notes. As a reviewer to code changes, gate keeping is precisely my job description. While one-line changes may seem innocuous, we've all had that time where we made such a change and caused wide reaching customer impact. This is not a risk that we are choosing to take on behalf of our customers when we have a mechanism, i.e., integration tests, to guard against such a thing.

While I laud your emphasis on customer obsession, we too are customer obsessed in that we don't want to merge a change that could be a breaking change for our customers due to its untested state. In this case, we would need to show even more caution given that tests were attempted and the build failed each time.

I think we can agree that the CDK is, in your words "strictly better" waiting for verification of working code than merging code that previously showed failures when tests were written. This is not a matter of blocking on principal, but blocking on failed builds.