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

Add command line flags to turn on routes #2442

Open
wants to merge 25 commits into
base: master
from

Conversation

@Ryan-Koch
Copy link
Contributor

commented Jul 24, 2019

Description

Creates command line flags so that we can separate services like Orders out from the rest of the app when running containers.

Setup

You'll want to make sure your dev environment kicks up

make server_run

You can also change your envrc file to make it think its staging to see if the flag check function for services.go is working properly. This will also require temporarily setting auth local to false and setting the mutual tls flag to true.

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using scripts/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Ryan-Koch added some commits Jul 24, 2019

Create command line flags to allow the separation of some app service…
…s from others. Add in check for the relationship between orders service and mutualtls listener in the check function for services.go.
Start edits to container json definitions. Still need to prune unnece…
…ssary things from app-client-tls container
Change the way the flag is being enabled in container json definition…
…s so that it's done in the environment object instead of trying to do so in the entryPoint
@codecov

This comment has been minimized.

Copy link

commented Jul 30, 2019

Codecov Report

Merging #2442 into master will decrease coverage by 0.6%.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           master   #2442     +/-   ##
========================================
- Coverage    60.1%   59.5%   -0.5%     
========================================
  Files         284     275      -9     
  Lines       15890   15563    -327     
========================================
- Hits         9535    9252    -283     
+ Misses       5235    5230      -5     
+ Partials     1120    1081     -39
Impacted Files Coverage Δ
pkg/cli/services.go 0% <0%> (ø)
pkg/auth/authentication/auth.go 27.8% <0%> (-13.9%) ⬇️
pkg/handlers/internalapi/move_documents.go 58.1% <0%> (-12%) ⬇️
pkg/services/accesscode/validate_access_code.go 75% <0%> (-9.6%) ⬇️
pkg/models/moving_expense_document.go 69% <0%> (-4.2%) ⬇️
pkg/uploader/uploader.go 48.7% <0%> (-1.3%) ⬇️
pkg/rateengine/rateengine.go 71% <0%> (-1.3%) ⬇️
pkg/services/fuelprice/diesel_fuel_price_storer.go 73.1% <0%> (-0.6%) ⬇️
pkg/handlers/adminapi/office_users.go 95.6% <0%> (ø) ⬇️
...g/handlers/internalapi/personally_procured_move.go 81.4% <0%> (ø) ⬇️
... and 36 more

@Ryan-Koch Ryan-Koch marked this pull request as ready for review Jul 31, 2019

SERVE_API_INTERNAL=true
SERVE_API_EXTERNAL=true
MUTUAL_TLS_ENABLED=true

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 31, 2019

Contributor

Go ahead and drop MUTUAL_TLS_ENABLED from all of these *.env files.

This comment has been minimized.

Copy link
@Ryan-Koch

Ryan-Koch Aug 9, 2019

Author Contributor

I did notice with this that removing it causes acceptance tests to fail, with orders still in these files. Should I keep mutual_tls_enabled here or remove orders or something else in your view?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 14, 2019

Contributor

That's probably because of a templating issue. Instead leave it in but set it like this:

Suggested change
MUTUAL_TLS_ENABLED=true
MUTUAL_TLS_ENABLED=false
Show resolved Hide resolved pkg/cli/services.go Outdated
Show resolved Hide resolved .envrc Outdated

@chrisgilmerproj chrisgilmerproj requested review from rdhariwal and removed request for pjdufour-truss Jul 31, 2019

@chrisgilmerproj chrisgilmerproj requested a review from mr337 Jul 31, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

Overall this looks great. I think you really nailed it. There are a few details to fix and we probably should deploy to experimental and test this before merging.

I've also added @rdhariwal and @mr337 to this since @pjdufour-truss has left the project.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@Ryan-Koch - I was thinking last night that when you deploy to experimental we need to verify that the Orders stuff still works on app-client-tls. In order to do that you'll need to coordinate with @jamesatheyDDS because he can run a script to upload fake Navy orders to that environment. I'd help but I'm still getting my setup working for uploading orders and I'm not ready. Once we verify that Orders works I think we can merge this in.

@jamesatheyDDS - the big impact for Orders here is that we won't be running any of the rest of the app on the same container. Only the orders API will be available. I'm pretty sure you don't need DPS for Orders but you should tell us. The larger picture is that we're keeping Login.gov auth on the app-client containers behind the ALB and mutual TLS auth on the app-client-tls containers behind the NLB. It's just a nice way for us to secure both auth systems separately.

@jim
Copy link
Contributor

left a comment

In some places we refer to the public API as public, and in others, we use external. I'd like to see this naming unified but am not partial to either name.

@Ryan-Koch

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

In some places we refer to the public API as public, and in others, we use external. I'd like to see this naming unified but am not partial to either name.

I hear you on this. Consistency would be ideal here. I'll go about seeing if I can safely rename the things on this end to match the public wording we use in the api structure itself. It seems a lighter lift than making that structure conform to this naming convention. I'll follow back up once that commit is pushed up.

Change externalapi naming convention here to match public api naming …
…convention used in the API related portions of the app.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.