LG-9449: Prevent Location Search slowdown and errors from ArcGIS auth by dawei-nava · Pull Request #8579 · 18F/identity-idp

dawei-nava · 2023-06-12T15:16:54Z

🎫 LG-9449: Prevent Location Search slowdown and errors from ArcGIS auth

🛠 Summary of changes

Create a scheduled job to refresh the auth token out of band. Application will assume the token is always available and valid by default.

On the web tier side, it depends on the job functioning correctly by default. On the other hand can be configured to fetch token as needed synchronously.

(Details need to be updated when it's finalized)

📜 Testing Plan

Provide a checklist of steps to confirm the changes.(Need to be updated)

Check job scheduled
Validate token refreshed as scheduled
Validate PO search returns successfully

changelog: Internal, ArcGis token management, job to manage/refresh token

app/jobs/arcgis_token_job.rb

app/services/arcgis_api/token_keeper.rb

zachmargolis · 2023-06-12T17:36:05Z

app/services/arcgis_api/connection_factory.rb

@@ -0,0 +1,22 @@
+module ArcgisApi
+  # Stateless factory object that create a new Farady::COnnection object
+  class ConnectionFactory


What is the purpose of this class? If there a reason we couldn't reuse geocoder.faraday?

To easily inject a mock one that stub request on connection for testing purpose, also customize faraday middleware behaviours.

FWIW there is test support that allows you to simulate HTTP interactions, making mocks of HTTP libraries like Faraday unnecessary (see the ArcgisApiHelper). That said I like the separation.

@allthesignals , good point, this was to mock on faraday level, webmock covers much more. It was adapted to the mock geocoder used in feature test. Let me c whether I can handle it better.

I'm ambivalent to the existence of this class. There are two effective usages of this (excluding suggest which is not currently used).

app/services/arcgis_api/token_keeper.rb

app/services/arcgis_api/connection_factory.rb

app/jobs/arcgis_token_job.rb

app/services/arcgis_api/token_keeper.rb

NavaTim · 2023-06-12T23:53:35Z

app/services/arcgis_api/token_keeper.rb

+    def handle_expired_token(cache_value)
+      cache_value = wrap_raw_token(cache_value) unless cache_value.is_a?(Hash)
+      expires_at = cache_value.fetch(:expires_at, nil)
+      sliding_expires_at = cache_value&.fetch(:sliding_expires_at, nil)
+      # start fetch new token before actual expires time
+      if IdentityConfig.store.arcgis_token_sliding_expiration_enabled
+        now = Time.zone.now.to_f
+        if sliding_expires_at && now >= sliding_expires_at
+          # if passed sliding expiration time(exp-3*prefetch_ttl)
+          # but not 2*prefetch_ttl from hard expiration time
+          if now > (sliding_expires_at + prefetch_ttl)
+            cache_value = retrieve_token
+          else
+            # extend sliding expiration time
+            cache_value[:sliding_expires_at] = sliding_expires_at + prefetch_ttl
+            save_token(cache_value, expires_at)
+          end
+        end
+      elsif expires_at && expires_at <= now
+        cache_value = retrieve_token
+      end
+      cache_value
+    end


The particular complexity and conditionals used in this method suggest that we should apply a strategy pattern. Also, it's likely that we will want to remove the previous caching strategy after the new one has proven effective - the way we implement this change should support safe removal of the previous strategy.

@NavaTim , this is part of dealing with expiring token on web application side, and disabled by default since we assume the job will refresh the token as needed.
Act as an escape route hopefully never needed?

At the same time let me c what refactor/abstraction can be done.

@NavaTim played with it a little bit, got kind of circular dependency with strategy pattern, trying subclassing now.

Act as an escape route hopefully never needed?

I think it's acceptable (though undesirable) to use this as backup to the job, but that does increase the risk of excessive authentication calls. It may be better to turn it off entirely, but for now we will need both.

@NavaTim played with it a little bit, got kind of circular dependency with strategy pattern, trying subclassing now.

I think the sliding window approach used here may be more complicated than necessary. We can use a specific amount of time before expiration instead, and maybe that will make this logic simpler.

app/services/arcgis_api/token_keeper.rb

NavaTim · 2023-06-13T00:28:40Z

app/services/arcgis_api/response_validation.rb

+      raise Faraday::ServerError.new(
+        RuntimeError.new(error_message),
+        {
+          status: error_code,
+          body: { details: response_body.dig('error', 'details')&.join(', ') },
+        },
+      )


Is this only going to be reached on 5xx errors?

This is to deal with 200 response but actually erred without token available. The faraday raise_error middleware does not handle this situation.

Since this isn't a 5xx, I don't think we should use Faraday::ServerError. We can use a custom error type instead and then if needed update the exceptions configured with faraday-retry.

app/services/arcgis_api/token_keeper.rb

app/services/arcgis_api/geocoder.rb

mitchellhenke · 2023-06-13T15:51:58Z

app/services/arcgis_api/response_validation.rb

@@ -0,0 +1,28 @@
+module ArcgisApi
+  # Faraday middleware to raise exception when response status is 200 but with error in body
+  class ResponseValidation < Faraday::Middleware


is there a way to keep the response logic closer to where the request is made?

@mitchellhenke when a new connection is created on demand, this middleware is registered with the connection for response processing.

right, sorry, I understand how it works, but if this is specific to one API, could we pull it into the class that's making those requests rather than separating it across files?

@mitchellhenke , we use farady-retry for automatic retrying, if we handle this after response returned to client, I think it's too late to trigger retry with farady-retry.

I think it's possible to have similar logic for retries when making the request, and I think it'd be better to move it there instead of having a separate class.

@mitchellhenke not exactly, for invalid response with 200 http status code, once the response is returned to caller, it's out of faraday middleware chain, and wont be able to utilize faraday-retry gem which works as a faraday middleware and we have to write our own retry mechanisms(bookkeeping, counting, making new requests the whole nine yard that's implemented by farady-retry).

@mitchellhenke I think that using this as a separate middleware class is acceptable given the way ESRI itself handles this in its arcgis-rest-js library. That said, I think it could have somewhat better documentation.

spec/services/arcgis_api/token_keeper_spec.rb

svalexander · 2023-06-15T14:30:57Z

app/jobs/arcgis_token_job.rb

@@ -0,0 +1,35 @@
+class ArcgisTokenJob < ApplicationJob


would it be more accurate to call this ArcgisRefreshTokenJob?

@svalexander, will in effect it's refresh the token, but it does not check whether the token need to be refreshed and replace with a new one anyway.

svalexander · 2023-06-15T14:34:39Z

app/services/arcgis_api/token_keeper.rb

+  # do not support entry expiration.
+  # @member token the token string
+  # @member expires_at hard expiration timestamp in epoch seconds
+  # @member sliding_expires_at optional the token keeper to maintain for


what is sliding expiration?

oh i see there is an explanation below but I don't fully understand it, is there another way to explain this?

@svalexander some description here https://brokul.dev/authentication-cookie-lifetime-and-sliding-expiration, not exactly with dotnet, but similar concept, basically avoiding a lot of clients getting token at the time very close to it's expiration time.

The sliding expiration doesn't seem to fit the use case as well since we have a regularly scheduled job to refresh the token and don't have to worry about user actions to trigger a refresh?

As I mentioned elsewhere, I think sliding expiration is a bit more complicated we require for this use case. Mostly we need to renew the token before it expires & before the IDP attempts to renew it. I'd be ok removing the frontend IDP renewal altogether if we can have a relatively high frequency & concurrency controls on the job.

app/services/arcgis_api/token_keeper.rb

app/jobs/arcgis_token_job.rb

app/services/arcgis_api/token_keeper.rb

config/application.yml.default

app/services/arcgis_api/token_keeper.rb

NavaTim · 2023-06-15T21:36:32Z

app/services/arcgis_api/token_keeper.rb

+      "#{IdentityConfig.store.arcgis_api_token_cache_key_prefix}:#{API_TOKEN_HOST}"
+    API_PREFETCH_TTL_SECONDS = IdentityConfig.store.arcgis_api_token_prefetch_ttl
+
+    RETRY_EXCEPTION = [404, 408, 409, 421, 429, 500, 502, 503, 504, 509]


This is a little complicated to dig up, but based on my prior research we'll be dealing with errors here that are similar to those mentioned in the ArcGIS REST JS documentation. You can view the underlying logic that ESRI uses to handle the errors in esri/arcgis-rest-js.

Mentioning this because I noticed a few additional errors here beyond what we're likely to see while using the ArcGIS API.

sheldon-b

I didn't have time to finish reviewing so left some questions/suggestions for now

config/application.yml.default

sheldon-b · 2023-06-15T21:39:31Z

config/application.yml.default

+arcgis_get_token_retry_interval: 1
+arcgis_get_token_retry_backoff_factor: 2
+arcgis_token_sliding_expiration_enabled: true
+arcgis_token_sync_request_enabled: false


What does this do? When would it be set to true vs. false? It's hard to tell from reading the code

@sheldon-b , it's to enable/disable arcgis token request from the web tier. We are creating new job to refresh the token, so we dont have to get it from web tier.

I don't understand what you mean, can you explain it more?

@sheldon-b The flag determines whether the user-facing servers will attempt to set the auth token if the auth token is missing. Disabling this flag will reduce the likelihood of a flood of USPS auth requests during high traffic to in-person proofing.

app/services/arcgis_api/mock/geocoder.rb

sheldon-b · 2023-06-15T21:58:29Z

app/services/arcgis_api/token_keeper.rb

+      expires_at = cache_value&.expires_at
+      sliding_expires_at = cache_value&.sliding_expires_at
+      # start fetch new token before actual expires time
+      if IdentityConfig.store.arcgis_token_sliding_expiration_enabled


When would we have this disabled vs. enabled? I don't see anything about this in the ticket or PR description so I don't entirely understand why it's added or how it's planned to be used

NavaTim · 2023-06-16T17:16:37Z

app/services/arcgis_api/connection_factory.rb

@@ -0,0 +1,22 @@
+module ArcgisApi
+  # Stateless factory object that create a new Farady::COnnection object
+  class ConnectionFactory


I'm ambivalent to the existence of this class. There are two effective usages of this (excluding suggest which is not currently used).

NavaTim · 2023-06-16T17:21:10Z

app/services/arcgis_api/response_validation.rb

@@ -0,0 +1,28 @@
+module ArcgisApi
+  # Faraday middleware to raise exception when response status is 200 but with error in body
+  class ResponseValidation < Faraday::Middleware


@mitchellhenke I think that using this as a separate middleware class is acceptable given the way ESRI itself handles this in its arcgis-rest-js library. That said, I think it could have somewhat better documentation.

NavaTim · 2023-06-16T17:24:11Z

app/services/arcgis_api/token_keeper.rb

+      current_sliding_expires_at = cache_value.sliding_expires_at
+      expiration_strategy.
+        extend_sliding_expires_at(token_info: cache_value, prefetch_ttl: prefetch_ttl)
+      if current_sliding_expires_at != cache_value.sliding_expires_at
+        save_token(
+          cache_value,
+          cache_value.expires_at,
+        )
+      end
+
+      # now retrieve new token
+      update_value = retrieve_token
+      expires_at = update_value&.expires_at
+      if sliding_expiration_enabled
+        update_value.sliding_expires_at = prefetch_ttl >= 0 ?
+                                            expires_at - TIMES_TTL * prefetch_ttl : expires_at
+      end
+      save_token(
+        update_value,
+        update_value.expires_at,
+      )
+      return update_value


@dawei-nava Per our discussion, let's refactor this logic so that the the sliding window approach is extremely clear and easy to read. This approach does look like it will reduce the likelihood of duplicate USPS auth calls and cache updates.

NavaTim · 2023-06-16T17:25:59Z

config/application.yml.default

+arcgis_get_token_retry_interval: 1
+arcgis_get_token_retry_backoff_factor: 2
+arcgis_token_sliding_expiration_enabled: true
+arcgis_token_sync_request_enabled: false


@sheldon-b The flag determines whether the user-facing servers will attempt to set the auth token if the auth token is missing. Disabling this flag will reduce the likelihood of a flood of USPS auth requests during high traffic to in-person proofing.

NavaTim · 2023-06-16T18:37:58Z

app/services/arcgis_api/token_keeper.rb

+      save_token(cache_entry, expires_at)
+      cache_entry
+    end
+
+    def remove_token
+      Rails.cache.delete(cache_key)
+    end
+
+    private
+
+    def handle_expired_token(cache_value)
+      cache_value = wrap_raw_token(cache_value) unless cache_value.is_a?(Hash)
+      expires_at = cache_value.fetch(:expires_at, nil)
+      sliding_expires_at = cache_value&.fetch(:sliding_expires_at, nil)
+      # start fetch new token before actual expires time
+      if IdentityConfig.store.arcgis_token_sliding_expiration_enabled
+        now = Time.zone.now.to_f
+        if sliding_expires_at && now >= sliding_expires_at
+          # if passed sliding expiration time(exp-3*prefetch_ttl)
+          # but not 2*prefetch_ttl from hard expiration time
+          if now > (sliding_expires_at + prefetch_ttl)
+            cache_value = retrieve_token
+          else
+            # extend sliding expiration time
+            cache_value[:sliding_expires_at] = sliding_expires_at + prefetch_ttl
+            save_token(cache_value, expires_at)


@dawei-nava Per our discussion, this code still needs to be updated to permit both the previous and new servers to correctly read the token during the rollout. The rollout will not include planned downtime.

…/LG-9449-arcgis-token-refresh

NavaTim · 2023-06-26T23:21:54Z

Closed in favor of #8662.

dawei-nava added 14 commits June 9, 2023 13:45

test

df007b7

LG-9449: refresh token

c253ded

LG-9449: WIP

144f2c5

LG-9449: WIP

d96e22d

LG-9449: refactor

1dfabbc

LG-9449: format

0c4e05c

LG-9449: cleanup and test

ec69df9

LG-9449: clean up.

8535941

LG-9449: job config and test.

73a8ee9

LG-9449: remove test change

187bc92

LG-9449: lint and new event.

ef74c4b

LG-9449: cleanup geocoder

7857c2f

LG-9449: fix test mock

29196e7

LG-9449: minor refactor and cleanup.

ef8c07c

changelog: Internal, ArcGis token management, job to manage/refresh token

dawei-nava requested review from a team and tomas-nava June 12, 2023 16:39

LG-9449: test fixes.

4a1154a

zachmargolis reviewed Jun 12, 2023

View reviewed changes

allthesignals reviewed Jun 12, 2023

View reviewed changes

app/services/arcgis_api/connection_factory.rb Outdated Show resolved Hide resolved

LG-9449: address comment

6562861

NavaTim reviewed Jun 13, 2023

View reviewed changes

LG-9449: address comment

7d4f6ae

mitchellhenke reviewed Jun 13, 2023

View reviewed changes

tomas-nava changed the title ~~Dwang/lg 9449 arcgis token refresh~~ LG-9449: Prevent Location Search slowdown and errors from ArcGIS auth Jun 13, 2023

dawei-nava added 3 commits June 13, 2023 21:01

LG-9449: address comment

a6a8caa

LG-9449: code climate

2d7e545

LG-9449: minor change.

feb10fa

mitchellhenke reviewed Jun 14, 2023

View reviewed changes

spec/services/arcgis_api/token_keeper_spec.rb Outdated Show resolved Hide resolved

dawei-nava added 2 commits June 14, 2023 12:01

LG-9449: job refactor and comment work.

69b8f4f

LG-9449: address comment on test.

b980494

svalexander reviewed Jun 15, 2023

View reviewed changes

app/services/arcgis_api/token_keeper.rb Outdated Show resolved Hide resolved

mitchellhenke reviewed Jun 15, 2023

View reviewed changes

app/jobs/arcgis_token_job.rb Outdated Show resolved Hide resolved

svalexander reviewed Jun 15, 2023

View reviewed changes

app/services/arcgis_api/token_keeper.rb Outdated Show resolved Hide resolved

svalexander reviewed Jun 15, 2023

View reviewed changes

config/application.yml.default Show resolved Hide resolved

svalexander reviewed Jun 15, 2023

View reviewed changes

app/services/arcgis_api/token_keeper.rb Outdated Show resolved Hide resolved

NavaTim reviewed Jun 15, 2023

View reviewed changes

sheldon-b reviewed Jun 15, 2023

View reviewed changes

dawei-nava added 6 commits June 16, 2023 08:55

LG-9449: address comment, refactor and test.

18a931f

LG-9449: address comment

6379866

LG-9449: address comment

fe2428e

LG-9449: address comment

d7024d6

LG-9449: address comment

5fb9933

LG-9449: lint issue.

d9ad542

NavaTim reviewed Jun 16, 2023

View reviewed changes

dawei-nava and others added 6 commits June 16, 2023 15:54

LG-9449: exception type.

b903397

LG-9449: address comments.

df19d25

LG-9449: customized error.

70cd2b7

Merge branch 'main' of https://github.com/18F/identity-idp into dwang…

0f6d3a8

…/LG-9449-arcgis-token-refresh

LG-9449: Remove unused suggest method and tests

d932d18

LG-9449: Remove puts usage

d7a98f7

NavaTim mentioned this pull request Jun 26, 2023

LG-9449: Prevent ArcGIS Token Expiration #8662

Closed

9 tasks

NavaTim closed this Jun 26, 2023

NavaTim mentioned this pull request Jun 28, 2023

LG-9449: Implement and configure ArcGIS API token refresh job #8692

Merged

10 tasks

Conversation

dawei-nava commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!