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

feat(redshift): adds elasticIp parameter to redshift cluster #21085

Merged
merged 7 commits into from
Jul 18, 2022

Conversation

daschaa
Copy link
Contributor

@daschaa daschaa commented Jul 10, 2022

Fixes #19191.

Adds the property elasticIp to the ClusterProps.


All Submissions:

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
Copy link

gitpod-io bot commented Jul 10, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team July 10, 2022 16:55
@github-actions github-actions bot added p2 feature-request A feature should be added or improved. p1 and removed p2 labels Jul 10, 2022
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@daschaa it looks like there are some additional considerations when creating a public redshift cluster and using an elasticIP https://docs.aws.amazon.com/redshift/latest/mgmt/managing-clusters-vpc.html.

Can you update the README to have a section that discusses this?

@daschaa
Copy link
Contributor Author

daschaa commented Jul 11, 2022

@daschaa it looks like there are some additional considerations when creating a public redshift cluster and using an elasticIP https://docs.aws.amazon.com/redshift/latest/mgmt/managing-clusters-vpc.html.

Can you update the README to have a section that discusses this?

Of course! Thanks for the feedback.
In the link you provided is a really good section about the elastic ip configuration. Quote:

If you configure your cluster to be publicly accessible, you can optionally select an elastic IP address to use for the external IP address. An elastic IP address is a static IP address that is associated with your AWS account. You can use an elastic IP address to connect to your cluster from outside the VPC. An elastic IP address gives you the ability to change your underlying configuration without affecting the IP address that clients use to connect to your cluster. This approach can be helpful for situations such as recovery after a failure.

Is it ok to copy this to the readme? Imho it explains it very good and simple. :)

@corymhall
Copy link
Contributor

Is it ok to copy this to the readme? Imho it explains it very good and simple. :)

yeah I think that is fine. Some of the other items I think it is important to include:

  1. The piece around connecting from within the VPC (with a code example showing the VPC configuration)
  2. The note around giving the cluster an elastic IP before making the VPC configuration (only active after a resize)
  3. When to use the elasticIP vs the clusterEndpointAddress
  4. How to attach or change the ip after the cluster has been completed.

@mergify mergify bot dismissed corymhall’s stale review July 12, 2022 18:51

Pull request has been modified.

@daschaa
Copy link
Contributor Author

daschaa commented Jul 12, 2022

@corymhall I updated the Readme and added the section about the elastic ip. I hope everything is ok - it would be great if you could double check.

In the topic of when to use elastic ip vs. cluster endpoint I'm not quite sure how to formulate it. As far as I understand it, the elastic ip should only be used when you need to have a static ip which you use to connect to the Redshift Cluster and the cluster endpoint is more dynamic, because it is a DNS address which is generated. Is it correct?

And the last thing about changing the ip after the cluster has been completed I do not quite understand. Could you give me a hint where to look regarding this topic?

Thanks in advance! ❤️

@corymhall
Copy link
Contributor

And the last thing about changing the ip after the cluster has been completed I do not quite understand. Could you give me a hint where to look regarding this topic?

It's near the bottom of that page:

The option to associate a cluster with an elastic IP address is available when you create the cluster or restore the cluster from a snapshot. In some cases, you might want to associate the cluster with an elastic IP address or change an elastic IP address that is associated with the cluster. To attach an elastic IP address after the cluster is created, first update the cluster so that it is not publicly accessible, then make it both publicly accessible and add an Elastic IP address in the same operation.


In the topic of when to use elastic ip vs. cluster endpoint I'm not quite sure how to formulate it.

I would just take from this portion. I think the important thing to not is just that they are not the same thing. There might be some confusion if there is a public elasticIP and a public node ip address.

The elastic IP address is an external IP address for accessing the cluster outside of a VPC. It's not related to the cluster node public IP addresses and private IP addresses that are displayed in the Amazon Redshift console under Connection details. The public and private cluster node IP addresses appear regardless of whether the cluster is publicly accessible or not. They are used only in certain circumstances to configure ingress rules on the remote host. These circumstances occur when you load data from an Amazon EC2 instance or other remote host using a Secure Shell (SSH) connection.

@daschaa
Copy link
Contributor Author

daschaa commented Jul 14, 2022

@corymhall Thank you very much for the hints! I added the sections you mentioned in your comment.

Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@daschaa awesome! love the readme updates ❤️

@mergify
Copy link
Contributor

mergify bot commented Jul 15, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@daschaa
Copy link
Contributor Author

daschaa commented Jul 15, 2022

@corymhall Are the tests flaky? Because I did not change anything I think..

@mrgrain
Copy link
Contributor

mrgrain commented Jul 18, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2022

update

✅ Branch has been successfully updated

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 638abec
  • 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 Jul 18, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit c88030f into aws:main Jul 18, 2022
@daschaa daschaa deleted the redshift_elastic_ip branch July 18, 2022 20:34
comcalvi pushed a commit to comcalvi/aws-cdk that referenced this pull request Jul 25, 2022
Fixes aws#19191.

Adds the property `elasticIp` to the `ClusterProps`.

----

### 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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
Labels
feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-redshift): Redshift construct is missing the elasticIp property
4 participants