Disable failing patron authentication (DB Migration Required) by RishiDiwanTT · Pull Request #980 · ThePalaceProject/circulation

RishiDiwanTT · 2023-04-04T11:35:34Z

NOTE: This PR includes a database migration

Description

The LibraryAuthenticator skips authentication mechanisms based on their status

There is also a random sampling built into the disable logic
so the disabled integrations get sampled during the timeout window
Now instead of raising an error the authenticated_patron method will return a problem detail
This is so the error is written to the DB and not rolled back at the end of the request

Motivation and Context

Over the last several weeks, we have seen a number of patron authentication mechanisms fail (the remote ILS goes offline). Because these connections take some time to fail, they cause a backup of requests on the webserver, causing other library on the same CM not to be able to authenticate.

Notion

How Has This Been Tested?

Manually Set and unset the status while hitting an authenticated endpoint
New unit tests were written

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

Now instead of raising an error the authenticated_patron method will return a problem detail This is so the error is written to the DB and not rolled back at the end of the request

Moved the externalintegrationerrors to the configuration model file Added the migration for the externalintegrations model

There is also a random sampling built into the disable logic so the disabled integrations get sampled during the timeout window

codecov · 2023-04-04T11:50:33Z

Codecov Report

Patch coverage: 95.89% and project coverage change: +0.01 🎉

Comparison is base (ae80281) 89.30% compared to head (17385a6) 89.31%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
+ Coverage   89.30%   89.31%   +0.01%     
==========================================
  Files         181      182       +1     
  Lines       32188    32249      +61     
  Branches     7190     7196       +6     
==========================================
+ Hits        28744    28803      +59     
- Misses       2283     2284       +1     
- Partials     1161     1162       +1

Impacted Files	Coverage Δ
core/model/__init__.py	`86.03% <ø> (ø)`
api/authenticator.py	`91.04% <90.62%> (-0.01%)`	⬇️
api/sip/client.py	`96.34% <100.00%> (+<0.01%)`	⬆️
core/jobs/update_integration_status.py	`100.00% <100.00%> (ø)`
core/model/configuration.py	`94.08% <100.00%> (+0.15%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

api/authenticator.py

api/sip/client.py

dbernstein

Looks good to me. Nice work, @RishiDiwanTT. I appreciated the comments and also the thorough test coverage.

RishiDiwanTT · 2023-04-05T06:05:07Z

core/jobs/update_integration_status.py

+
+    # 10 errors in the last 30 minutes
+    ERROR_WINDOW_DELTA = datetime.timedelta(minutes=30)
+    ERROR_WINDOW_COUNT = 10


I forgot to draw attention to these numbers, they were chosen because they are numbers, and for no other reason.
Should they be changed to something with more rationale behind them?
@dbernstein @jonathangreen

@RishiDiwanTT : I like numbers. Numbers are good. ;) It seems like as good a guess as any. I think 10 errors in 1/2 hour is a good place to start.

tdilauro

This is looking pretty good!

I have a couple concerns, which are discussed in the comments below, and one minor general general suggestion, which is to end sentences in comments and docstrings with a period/full stop.

alembic/versions/20230404_3d5db04dc99b_integration_status.py

api/authenticator.py

tdilauro · 2023-04-05T19:17:20Z

api/authenticator.py

+        )
+        return record
+
+    def _is_provider_available(self, _db, provider):


Can you provide type hints here?

api/authenticator.py

api/sip/client.py

tdilauro · 2023-04-05T20:11:04Z

core/model/configuration.py

+    class STATUS:
+        RED = 0
+        GREEN = 1


Any reason not to just make this an enum?

Laziness. This has been fixed. Not in me, in the code.

I was reading through my GitHub notifications this morning while I was eating breakfast, and this comment gave me a good laugh 🤣

This is more readable than an integer

RishiDiwanTT · 2023-04-13T05:27:34Z

@tdilauro I'll wait for your approval before merging.

tdilauro

I think this is ready to go! 🏁

We are seeing elevated errors due to the auth filtering happening in migration intact. We will do further work and reenable in a future PR.

RishiDiwanTT requested a review from a team April 4, 2023 11:35

RishiDiwanTT self-assigned this Apr 4, 2023

RishiDiwanTT added 3 commits April 4, 2023 17:08

External Integration error log model and logic

ec85500

Now instead of raising an error the authenticated_patron method will return a problem detail This is so the error is written to the DB and not rolled back at the end of the request

Update integration status script and crontab

5843169

Moved the externalintegrationerrors to the configuration model file Added the migration for the externalintegrations model

Authenticator skips authentication mechanisms based on their status

60a8b45

There is also a random sampling built into the disable logic so the disabled integrations get sampled during the timeout window

RishiDiwanTT force-pushed the feature/ils-deadman-switch branch from f5a70bb to 60a8b45 Compare April 4, 2023 11:42

RishiDiwanTT commented Apr 4, 2023

View reviewed changes

api/authenticator.py Outdated Show resolved Hide resolved

RishiDiwanTT commented Apr 4, 2023

View reviewed changes

api/sip/client.py Show resolved Hide resolved

dbernstein changed the title ~~Disable failing patron authentication~~ Disable failing patron authentication (DB Migration Required) Apr 4, 2023

dbernstein approved these changes Apr 4, 2023

View reviewed changes

RishiDiwanTT commented Apr 5, 2023

View reviewed changes

tdilauro requested changes Apr 5, 2023

View reviewed changes

RishiDiwanTT added 2 commits April 12, 2023 12:09

Minor review changes

d456f19

Moved the external integration status column to an Enum

17385a6

This is more readable than an integer

RishiDiwanTT requested a review from tdilauro April 12, 2023 07:37

RishiDiwanTT added the DB migration This PR contains a DB migration label Apr 13, 2023

tdilauro approved these changes Apr 14, 2023

View reviewed changes

jonathangreen added the feature New feature label Apr 14, 2023

RishiDiwanTT merged commit d2eb840 into main Apr 17, 2023

RishiDiwanTT deleted the feature/ils-deadman-switch branch April 17, 2023 05:10

jonathangreen added a commit that referenced this pull request May 15, 2023

Revert #980 - Causing too many failures.

bd47d68

jonathangreen added a commit that referenced this pull request May 15, 2023

Revert #980 for now, since it is causing errors

cfc3e14

We are seeing elevated errors due to the auth filtering happening in migration intact. We will do further work and reenable in a future PR.

jonathangreen mentioned this pull request May 15, 2023

Revert PR #980 temporarily, while we do further work on it #1103

Merged

2 tasks

jonathangreen added a commit that referenced this pull request May 16, 2023

Revert #980 for now, since it is causing errors (#1103)

88f5554

We are seeing elevated errors due to the auth filtering happening in migration intact. We will do further work and reenable in a future PR.

jonathangreen mentioned this pull request May 25, 2023

Remove tables leftover after reverting #980 #1144

Merged

2 tasks

jonathangreen added a commit that referenced this pull request Jun 6, 2023

Remove tables leftover after reverting #980 (#1144)

e063f55

Comments

Conversation

RishiDiwanTT commented Apr 4, 2023 • edited by dbernstein Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTE: This PR includes a database migration

Description

Motivation and Context

How Has This Been Tested?

Checklist

Uh oh!

codecov bot commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

dbernstein left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RishiDiwanTT Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

dbernstein Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tdilauro Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tdilauro Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

RishiDiwanTT Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

jonathangreen Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

RishiDiwanTT commented Apr 13, 2023

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RishiDiwanTT commented Apr 4, 2023 •

edited by dbernstein

Loading

codecov bot commented Apr 4, 2023 •

edited

Loading

dbernstein left a comment •

edited

Loading