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

Move Redis to a nonstandard port #139

Merged
merged 5 commits into from
Apr 25, 2023

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Apr 24, 2023

🗣 Description

This pull request:

  1. Modifies the Docker composition so that the external port for Redis is no longer the standard value of 6379 (if the ports section of the redis service is uncommented).
  2. Alphabetizes the key-value pairs in the ports section by key.
  3. Adds a host_ip key to the ports dictionary to restrict Redis access to only the Docker host.

💭 Motivation and context

  1. The Admiral uses and exposes its own instance of Redis, so by changing to a different port we avoid a conflict where both compositions attempt to use the same port.
  2. Alphabetization is next to godliness.
  3. Even when the ports dictionary is uncommented, the exposed port is only used for debugging purposes from the Docker host; therefore, it only makes sense to listen for local traffic.

🧪 Testing

All automated tests pass. I also started up the redis service with the ports dictionary uncommented and verified that redis-cli functions as expected from the Docker host.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

✅ Post-merge checklist

  • Create a release.

This is done to avoid conflict with the Admiral, which also uses
Redis.
This port is only ever used for debugging from the Docker host, so
there is no need to listen on all network interfaces.
@jsf9k jsf9k added improvement This issue or pull request will add or improve functionality, maintainability, or ease of use version bump This issue or pull request increments the version number labels Apr 24, 2023
@jsf9k jsf9k self-assigned this Apr 24, 2023
@jsf9k jsf9k marked this pull request as ready for review April 24, 2023 15:02
@jsf9k jsf9k requested review from jasonodoom and a team April 24, 2023 15:03
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

👍 with one suggestion that you may take or leave.

docker-compose.yml Outdated Show resolved Hide resolved
So future readers know what we are talking about.

Co-authored-by: dav3r <david.redmin@trio.dhs.gov>
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Seems reasonable enough.

@jsf9k jsf9k merged commit 2f01801 into develop Apr 25, 2023
@jsf9k jsf9k deleted the improvement/move-redis-to-nonstandard-port branch April 25, 2023 18:25
cisagovbot pushed a commit that referenced this pull request Sep 13, 2023
Use the correct repo name for the ansible-lint pre-commit hook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use version bump This issue or pull request increments the version number
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants