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.

Sign up for GitHub

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

Jump to bottom

(aws_cdk.aws_elasticsearch): Set connections defaultPort when enforceHttps is true #16251

Closed
SamStephens opened this issue Aug 27, 2021 · 4 comments · Fixed by #20602
Closed

(aws_cdk.aws_elasticsearch): Set connections defaultPort when enforceHttps is true #16251

SamStephens opened this issue Aug 27, 2021 · 4 comments · Fixed by #20602
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@SamStephens
Copy link
Contributor

When a VPC is used with an elasticsearch Domain, the resulting connections object does not have a default port. However, when enforceHttps is set, we know communications will be over port 443.

Use Case

So I don't have to manually specify a port when setting up VPC connections to the Domain.

Proposed Solution

Include a defaultPort when constructing the connections object if enforceHttps is true.

This is a 🚀 Feature Request
@SamStephens SamStephens added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 27, 2021
@peterwoodworth
Copy link
Contributor

Is this still true for opensearch @SamStephens ?

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 28, 2022
@peterwoodworth
Copy link
Contributor

Looks like this is still valid. Would probably need to change here

aws-cdk/packages/@aws-cdk/aws-opensearchservice/lib/domain.ts

Line 1244 in ea69ad2

this._connections = new ec2.Connections({ securityGroups });

We accept contributions 🙂

@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels May 28, 2022
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 28, 2022
@SamStephens SamStephens mentioned this issue Jun 3, 2022
Merged
4 tasks
@SamStephens
Copy link
Contributor Author

@peterwoodworth I've had a go at a fix for this, hopefully it suits.

@mergify mergify bot closed this as completed in #20602 Jun 13, 2022
mergify bot pushed a commit that referenced this issue Jun 13, 2022
@SamStephens
feat(opensearchservice): When a Domain has enforceHttps true, set the…
a6fe2cb 
… connections defaultPort (#20602)

For an Opensearch domain, if enforceHttps is enabled, set the defaultPort for the connections object of the domain, as we know it communicates over 443 in this scenario.

I also took the liberty of correcting a test title while I was in the code, I hope it's okay coupling that fix with this change.

closes #16251

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

It doesn't seem that an integration test is a good fit here, as this simply changes/simplifies how you'll create a connection to a Domain. I'm happy to be corrected however and will add one if asked.

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] 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*
@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.

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
@SamStephens @daschaa
feat(opensearchservice): When a Domain has enforceHttps true, set the…
8e38b0e 
… connections defaultPort (aws#20602)

For an Opensearch domain, if enforceHttps is enabled, set the defaultPort for the connections object of the domain, as we know it communicates over 443 in this scenario.

I also took the liberty of correcting a test title while I was in the code, I hope it's okay coupling that fix with this change.

closes aws#16251

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

It doesn't seem that an integration test is a good fit here, as this simply changes/simplifies how you'll create a connection to a Domain. I'm happy to be corrected however and will add one if asked.

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(opensearchservice): When a Domain has enforceHttps true, set the connections defaultPort
2 participants
@SamStephens @peterwoodworth