Skip to content

LG-7918: Use Rails.cache for USPS IPPaaS auth tokens#7294

Closed
sheldon-b wants to merge 5 commits intomainfrom
sbachstein/lg-7918-cache-usps-auth-token-using-rails-cache
Closed

LG-7918: Use Rails.cache for USPS IPPaaS auth tokens#7294
sheldon-b wants to merge 5 commits intomainfrom
sbachstein/lg-7918-cache-usps-auth-token-using-rails-cache

Conversation

@sheldon-b
Copy link
Contributor

@sheldon-b sheldon-b commented Nov 4, 2022

⚠️ Do not merge

Matt Gardner says that the caching of auth tokens wasn't working as expected for the ArcGIS Geocoder class so I want to dig into that before this PR gets merged.

🎫 Ticket

LG-7918

🛠 Summary of changes

  • Updates the UspsInPersonProofing::Proofer class to cache authorization tokens to the Rails cache so that they can be re-used across different instances of our IDP web servers.

📜 Testing Plan

  • Test IPP flow in dev environment
  • Use a Rails console in the dev environment to ensure that tokens are being requested and cache appropriately, and are being refreshed when the cache expires

@sheldon-b sheldon-b marked this pull request as ready for review November 4, 2022 15:35
Comment on lines +114 to +119
authz = "#{token_type} #{token}"
Rails.cache.write(
API_TOKEN_CACHE_KEY, authz,
expires_at: Time.zone.now + expires_in
)
authz
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an average/expected expiration for these tokens? Wondering if we could pick a fixed duration and use the block Rails.cache.fetch(..., expires_in) { } syntax and simplify a bunch of this code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expected is 15 minutes but it feels weird to hardcode it when we get the actual value in the API response. Agreed it'd be more concise. Mixed feelings about it

Copy link
Contributor

Choose a reason for hiding this comment

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

I would make our version of the token expire slightly before the value provided in the response, maybe by a few seconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. to reduce the probability that the token will expire between fetching it from cache & attempting to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had that idea but Andrew convinced me that it's a half-measure and we should just fix it properly by adding in a retry if the token has expired. I created this ticket for it. Original PR thread

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not both?

Copy link
Contributor Author

@sheldon-b sheldon-b Nov 4, 2022

Choose a reason for hiding this comment

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

Because it removes the urgency of fixing it better. I think we'd want to do both, just not the one right now

@sheldon-b sheldon-b requested review from a team, NavaTim and allthesignals November 4, 2022 17:16
Comment on lines +125 to 127
def token
Rails.cache.read(API_TOKEN_CACHE_KEY) || retrieve_token!
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Rails cache include an in-memory cache or does it go straight to Redis? If it's the latter, then I would suggest keeping the class/module field.

e.g. in the use case of a large volume in the the USPS proofing job, this could mean hundreds of unnecessary trips to Redis.

Copy link
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

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

You would still need to be mindful of the TTL though, as well as the possibility of another server writing to the Redis cache.

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 think it'd be interesting to see how load testing goes with all of the requests to redis. I agree it could end up being an issue under very high load and doing two layers of caching (eg class variable and Rails.cache) may make a lot of sense here

Copy link
Contributor

@aduth aduth Nov 4, 2022

Choose a reason for hiding this comment

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

My understanding is that it was a primary goal of this work to have it be stored in a Redis cache, so that the same USPS token would be reused across IdP instances.

The Redis cost is definitely an interesting factor I hadn't given much thought to previously, but then we'd still want to weigh it against the additional cost of roundtrips to USPS for IdP-instance-specific tokens.

Copy link
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

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

The intention would be to use the same token across instances, but that token could be cached in-memory - i.e. in a manner that respects the original TTL of the token.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think I see what you're saying now -- still use Redis, but also having a local copy to avoid repeatedly calling to Redis while the token is assumed to be valid? (what @sheldon-b was referring to in "two layers of caching")

Sounds like it could be a good idea, but also likely to be tricky to implement 😅

Copy link
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

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

That's what I'm talking about.

Pseudocode would look like:

If local cache exists and TTL has not passed:
  Use local version
Else:
  Get Redis value

  If Redis value is missing:
    Authenticate w/ USPS
    SET Redis value w/ NX and PX options
    Get Redis value  

  Locally cache and use Redis value

The TTL should be identical between the local cache and Redis, but both should be very slightly before the real expiry of the token.

@sheldon-b sheldon-b marked this pull request as draft November 7, 2022 18:42
@aduth
Copy link
Contributor

aduth commented Dec 20, 2022

Is this pull request still relevant?

@sheldon-b
Copy link
Contributor Author

@aduth the next ticket I'm planning to pick up is to resolve this. Not sure if I'll end up resurrecting this PR or closing it but I'll resolve it one way or the other in the next few work days

@sheldon-b sheldon-b closed this Dec 22, 2022
@sheldon-b sheldon-b deleted the sbachstein/lg-7918-cache-usps-auth-token-using-rails-cache branch August 29, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants