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

fix(ec2): private DNS for custom endpoints has incorrect default #5987

Merged

Conversation

flemjame-at-amazon
Copy link
Contributor

@flemjame-at-amazon flemjame-at-amazon commented Jan 27, 2020


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

Currently if I create an InterfaceVpcEndpoint to a non-AWS service, I must set the privateDnsEnabled field to be false, like so:

InterfaceVpcEndpoint vpce = new InterfaceVpcEndpoint(construct, "VPCe", InterfaceVpcEndpointProps.builder()
            // Private DNS is only supported by AWS services
            .privateDnsEnabled(Boolean.FALSE)
            .vpc(vpc)
            .service(providerService)
            .build());

This is because the default is "true", which works for AWS services but not for custom hosted services. This change exposes a privateDnsDefault for different IInterfaceVpcEndpointService implementations, and VpcEndpoint will use that as the default if privateDnsEnabled is not specified. This way, I could create a service like so, without having to specify private DNS settings:

    // Here I create a VPC endpoint to a custom hosted service
    IInterfaceVpcEndpointService providerService = new InterfaceVpcEndpointService(serviceName, servicePort);

    InterfaceVpcEndpoint vpce = new InterfaceVpcEndpoint(construct, "VPCe", InterfaceVpcEndpointProps.builder()
            .vpc(vpc)
            .service(providerService)
            .build());

This should break no existing functionality:

  • Users overriding privateDnsEnabled
    • Will continue to have their setting applied
  • Users not overriding privateDnsEnabled
    • If they are using InterfaceVpcEndpointAwsService, then the default associated with InterfaceVpcEndpointAwsService will be applied, which is true, which matches the current behavior
    • Users cannot use an InterfaceVpcEndpoint without overriding privateDnsEnabled, because only AWS services support private DNS

Commit Message

fix(ec2): private DNS for custom endpoints has incorrect default

Currently if I create an InterfaceVpcEndpoint to a non-AWS service, I must set the privateDnsEnabled field to be "false". This is because the default is "true", which works for AWS services but not for custom hosted services. This change exposes a privateDnsDefault for different IInterfaceVpcEndpointService implementations, and VpcEndpoint will use that as the default if privateDnsEnabled is not specified. This way, I could create a service without having to specify private DNS settings.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

rix0rrr
rix0rrr previously requested changes Feb 3, 2020
packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts Outdated Show resolved Hide resolved
/**
* Whether Private DNS is supported by default.
*/
readonly privateDnsDefault?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this optional for backwards compatibility -- any existing CDK code specifying a service like:

  const interfaceVpcEndpoint = new ec2.InterfaceVpcEndpoint(stack, 'InterfaceEndpoint', {
    vpc,
    service: {
      name: 'com.amazonaws.us-west-2.workspaces',
      port: 80
    }
  });

Will get compilation errors if I don't make the field optional:

Property 'privateDnsDefault' is missing in type '{ name: string; port: number; }' 
but required in type 'IInterfaceVpcEndpointService'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That's not how you're supposed to supply that value though, but I get it.

package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed rix0rrr’s stale review February 3, 2020 15:22

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@rix0rrr rix0rrr changed the title feat(ec2): default setting for VpcEndpoint PrivateDNS depends on service type fix(ec2): private DNS for custom endpoints has incorrect default Feb 4, 2020
@mergify
Copy link
Contributor

mergify bot commented Feb 4, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@@ -0,0 +1,111 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rix0rrr there's a package-lock.json committed here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh doh.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Feb 4, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit d681d96 into aws:master Feb 4, 2020
jogold added a commit to jogold/aws-cdk that referenced this pull request Feb 4, 2020
mergify bot added a commit that referenced this pull request Feb 4, 2020
See #5987 (comment)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@flemjame-at-amazon flemjame-at-amazon deleted the vpc-endpoint-private-dns-defaults branch February 7, 2020 13:48
mergify bot pushed a commit that referenced this pull request Apr 26, 2024
Bumps [axios](https://github.com/axios/axios) from 0.27.2 to 1.6.8.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/axios/axios/releases">axios's releases</a>.</em></p>
<blockquote>
<h2>Release v1.6.8</h2>
<h2>Release notes:</h2>
<h3>Bug Fixes</h3>
<ul>
<li><strong>AxiosHeaders:</strong> fix AxiosHeaders conversion to an object during config merging (<a href="https://github.com/axios/axios/issues/6243">#6243</a>) (<a href="https://github.com/axios/axios/commit/2656612bc10fe2757e9832b708ed773ab340b5cb">2656612</a>)</li>
<li><strong>import:</strong> use named export for EventEmitter; (<a href="https://github.com/axios/axios/commit/7320430aef2e1ba2b89488a0eaf42681165498b1">7320430</a>)</li>
<li><strong>vulnerability:</strong> update follow-redirects to 1.15.6 (<a href="https://github.com/axios/axios/issues/6300">#6300</a>) (<a href="https://github.com/axios/axios/commit/8786e0ff55a8c68d4ca989801ad26df924042e27">8786e0f</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li> <a href="https://github.com/jasonsaayman" title="+4572/-3446 ([#6238](axios/axios#6238) )">Jay</a></li>
<li> <a href="https://github.com/DigitalBrainJS" title="+30/-0 ([#6231](axios/axios#6231) )">Dmitriy Mozgovoy</a></li>
<li> <a href="https://github.com/Creaous" title="+9/-9 ([#6300](axios/axios#6300) )">Mitchell</a></li>
<li> <a href="https://github.com/mannoeu" title="+2/-2 ([#6196](axios/axios#6196) )">Emmanuel</a></li>
<li> <a href="https://github.com/ljkeller" title="+3/-0 ([#6194](axios/axios#6194) )">Lucas Keller</a></li>
<li> <a href="https://github.com/ADITYA-176" title="+1/-1 ()">Aditya Mogili</a></li>
<li> <a href="https://github.com/petrovmiroslav" title="+1/-1 ([#6243](axios/axios#6243) )">Miroslav Petrov</a></li>
</ul>
<h2>Release v1.6.7</h2>
<h2>Release notes:</h2>
<h3>Bug Fixes</h3>
<ul>
<li>capture async stack only for rejections with native error objects; (<a href="https://github.com/axios/axios/issues/6203">#6203</a>) (<a href="https://github.com/axios/axios/commit/1a08f90f402336e4d00e9ee82f211c6adb1640b0">1a08f90</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li> <a href="https://github.com/DigitalBrainJS" title="+30/-26 ([#6203](axios/axios#6203) )">Dmitriy Mozgovoy</a></li>
<li> <a href="https://github.com/zh-lx" title="+0/-3 ([#6186](axios/axios#6186) )">zhoulixiang</a></li>
</ul>
<h2>Release v1.6.6</h2>
<h2>Release notes:</h2>
<h3>Bug Fixes</h3>
<ul>
<li>fixed missed dispatchBeforeRedirect argument (<a href="https://github.com/axios/axios/issues/5778">#5778</a>) (<a href="https://github.com/axios/axios/commit/a1938ff073fcb0f89011f001dfbc1fa1dc995e39">a1938ff</a>)</li>
<li>wrap errors to improve async stack trace (<a href="https://github.com/axios/axios/issues/5987">#5987</a>) (<a href="https://github.com/axios/axios/commit/123f354b920f154a209ea99f76b7b2ef3d9ebbab">123f354</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li> <a href="https://github.com/ikonst" title="+91/-8 ([#5987](axios/axios#5987) )">Ilya Priven</a></li>
<li> <a href="https://github.com/zaosoula" title="+6/-6 ([#5778](axios/axios#5778) )">Zao Soula</a></li>
</ul>
<h2>Release v1.6.5</h2>
<h2>Release notes:</h2>
<h3>Bug Fixes</h3>
<ul>
<li><strong>ci:</strong> refactor notify action as a job of publish action; (<a href="https://github.com/axios/axios/issues/6176">#6176</a>) (<a href="https://github.com/axios/axios/commit/0736f95ce8776366dc9ca569f49ba505feb6373c">0736f95</a>)</li>
<li><strong>dns:</strong> fixed lookup error handling; (<a href="https://github.com/axios/axios/issues/6175">#6175</a>) (<a href="https://github.com/axios/axios/commit/f4f2b039dd38eb4829e8583caede4ed6d2dd59be">f4f2b03</a>)</li>
</ul>
<h3>Contributors to this release</h3>

</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/axios/axios/blob/v1.x/CHANGELOG.md">axios's changelog</a>.</em></p>
<blockquote>
<h2><a href="https://github.com/axios/axios/compare/v1.6.7...v1.6.8">1.6.8</a> (2024-03-15)</h2>
<h3>Bug Fixes</h3>
<ul>
<li><strong>AxiosHeaders:</strong> fix AxiosHeaders conversion to an object during config merging (<a href="https://github.com/axios/axios/issues/6243">#6243</a>) (<a href="https://github.com/axios/axios/commit/2656612bc10fe2757e9832b708ed773ab340b5cb">2656612</a>)</li>
<li><strong>import:</strong> use named export for EventEmitter; (<a href="https://github.com/axios/axios/commit/7320430aef2e1ba2b89488a0eaf42681165498b1">7320430</a>)</li>
<li><strong>vulnerability:</strong> update follow-redirects to 1.15.6 (<a href="https://github.com/axios/axios/issues/6300">#6300</a>) (<a href="https://github.com/axios/axios/commit/8786e0ff55a8c68d4ca989801ad26df924042e27">8786e0f</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li> <a href="https://github.com/jasonsaayman" title="+4572/-3446 ([#6238](axios/axios#6238) )">Jay</a></li>
<li> <a href="https://github.com/DigitalBrainJS" title="+30/-0 ([#6231](axios/axios#6231) )">Dmitriy Mozgovoy</a></li>
<li> <a href="https://github.com/Creaous" title="+9/-9 ([#6300](axios/axios#6300) )">Mitchell</a></li>
<li> <a href="https://github.com/mannoeu" title="+2/-2 ([#6196](axios/axios#6196) )">Emmanuel</a></li>
<li> <a href="https://github.com/ljkeller" title="+3/-0 ([#6194](axios/axios#6194) )">Lucas Keller</a></li>
<li> <a href="https://github.com/ADITYA-176" title="+1/-1 ()">Aditya Mogili</a></li>
<li> <a href="https://github.com/petrovmiroslav" title="+1/-1 ([#6243](axios/axios#6243) )">Miroslav Petrov</a></li>
</ul>
<h2><a href="https://github.com/axios/axios/compare/v1.6.6...v1.6.7">1.6.7</a> (2024-01-25)</h2>
<h3>Bug Fixes</h3>
<ul>
<li>capture async stack only for rejections with native error objects; (<a href="https://github.com/axios/axios/issues/6203">#6203</a>) (<a href="https://github.com/axios/axios/commit/1a08f90f402336e4d00e9ee82f211c6adb1640b0">1a08f90</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li> <a href="https://github.com/DigitalBrainJS" title="+30/-26 ([#6203](axios/axios#6203) )">Dmitriy Mozgovoy</a></li>
<li> <a href="https://github.com/zh-lx" title="+0/-3 ([#6186](axios/axios#6186) )">zhoulixiang</a></li>
</ul>
<h2><a href="https://github.com/axios/axios/compare/v1.6.5...v1.6.6">1.6.6</a> (2024-01-24)</h2>
<h3>Bug Fixes</h3>
<ul>
<li>fixed missed dispatchBeforeRedirect argument (<a href="https://github.com/axios/axios/issues/5778">#5778</a>) (<a href="https://github.com/axios/axios/commit/a1938ff073fcb0f89011f001dfbc1fa1dc995e39">a1938ff</a>)</li>
<li>wrap errors to improve async stack trace (<a href="https://github.com/axios/axios/issues/5987">#5987</a>) (<a href="https://github.com/axios/axios/commit/123f354b920f154a209ea99f76b7b2ef3d9ebbab">123f354</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li> <a href="https://github.com/ikonst" title="+91/-8 ([#5987](axios/axios#5987) )">Ilya Priven</a></li>
<li> <a href="https://github.com/zaosoula" title="+6/-6 ([#5778](axios/axios#5778) )">Zao Soula</a></li>
</ul>
<h2><a href="https://github.com/axios/axios/compare/v1.6.4...v1.6.5">1.6.5</a> (2024-01-05)</h2>
<h3>Bug Fixes</h3>
<ul>
<li><strong>ci:</strong> refactor notify action as a job of publish action; (<a href="https://github.com/axios/axios/issues/6176">#6176</a>) (<a href="https://github.com/axios/axios/commit/0736f95ce8776366dc9ca569f49ba505feb6373c">0736f95</a>)</li>
</ul>

</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/axios/axios/commit/ab3f0f9a94853c821cb00f1112788ecdd3ae7ed1"><code>ab3f0f9</code></a> chore(release): v1.6.8 (<a href="https://github.com/axios/axios/issues/6303">#6303</a>)</li>
<li><a href="https://github.com/axios/axios/commit/2656612bc10fe2757e9832b708ed773ab340b5cb"><code>2656612</code></a> fix(AxiosHeaders): fix AxiosHeaders conversion to an object during config mer...</li>
<li><a href="https://github.com/axios/axios/commit/7320430aef2e1ba2b89488a0eaf42681165498b1"><code>7320430</code></a> fix(import): use named export for EventEmitter;</li>
<li><a href="https://github.com/axios/axios/commit/8786e0ff55a8c68d4ca989801ad26df924042e27"><code>8786e0f</code></a> fix(vulnerability): update follow-redirects to 1.15.6 (<a href="https://github.com/axios/axios/issues/6300">#6300</a>)</li>
<li><a href="https://github.com/axios/axios/commit/d844227411263fab39d447442879112f8b0c8de5"><code>d844227</code></a> chore: update and bump deps (<a href="https://github.com/axios/axios/issues/6238">#6238</a>)</li>
<li><a href="https://github.com/axios/axios/commit/caa06252015003990958d7db96f63aa646bc58e8"><code>caa0625</code></a> docs: update README responseEncoding types (<a href="https://github.com/axios/axios/issues/6194">#6194</a>)</li>
<li><a href="https://github.com/axios/axios/commit/41c4584a41fad1d94cf86331667deff5a0b75044"><code>41c4584</code></a> docs: Update README.md to point to current axios version in CDN links (<a href="https://github.com/axios/axios/issues/6196">#6196</a>)</li>
<li><a href="https://github.com/axios/axios/commit/bf6974f16af6c72985f094a98d874c5a6adcdc83"><code>bf6974f</code></a> chore(ci): add npm tag action; (<a href="https://github.com/axios/axios/issues/6231">#6231</a>)</li>
<li><a href="https://github.com/axios/axios/commit/a52e4d9af51205959ef924f87bcf90c605e08a1e"><code>a52e4d9</code></a> chore(release): v1.6.7 (<a href="https://github.com/axios/axios/issues/6204">#6204</a>)</li>
<li><a href="https://github.com/axios/axios/commit/2b69888dd5601bbc872452ccd24010219fb6e41a"><code>2b69888</code></a> chore: remove unnecessary check (<a href="https://github.com/axios/axios/issues/6186">#6186</a>)</li>
<li>Additional commits viewable in <a href="https://github.com/axios/axios/compare/v0.27.2...v1.6.8">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=axios&package-manager=npm_and_yarn&previous-version=0.27.2&new-version=1.6.8)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/aws/aws-cdk/network/alerts).

</details>
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 this pull request may close these issues.

4 participants