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

Chore/add authz caching #432

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Chore/add authz caching #432

wants to merge 6 commits into from

Conversation

nss10
Copy link
Contributor

@nss10 nss10 commented Mar 13, 2025

Link to JIRA ticket if there is one: MIDRC-946

Improvements

  • Cache arborist requests for 1 second to reduce multiple frequent requets.
  • Add "linux/amd64" as the build platform to increase CI speed

Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@coveralls
Copy link

coveralls commented Mar 13, 2025

Pull Request Test Coverage Report for Build 13844442420

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 62.5%

Files with Coverage Reduction New Missed Lines %
auth/init.py 7 90.41%
Totals Coverage Status
Change from base Build 13812991300: 0.07%
Covered Lines: 2380
Relevant Lines: 3808

💛 - Coveralls

Copy link

Please find the ci env pod logs here

Copy link

filepath $$\textcolor{#23d18b}{\tt{passed}}$$ $$\textcolor{#ffa500}{\tt{skipped}}$$ SUBTOTAL
$$\textcolor{#ffa500}{\tt{tests/test\_graph\_submit\_and\_query.py}}$$ $$\textcolor{#23d18b}{\tt{13}}$$ $$\textcolor{#ffa500}{\tt{1}}$$ $$\textcolor{#ffa500}{\tt{14}}$$
$$\textcolor{#ffa500}{\tt{tests/test\_data\_upload.py}}$$ $$\textcolor{#23d18b}{\tt{8}}$$ $$\textcolor{#ffa500}{\tt{1}}$$ $$\textcolor{#ffa500}{\tt{9}}$$
$$\textcolor{#ffa500}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{21}}$$ $$\textcolor{#ffa500}{\tt{2}}$$ $$\textcolor{#ffa500}{\tt{23}}$$

Please find the detailed integration test report here

Please find the ci env pod logs here

@@ -29,6 +29,8 @@ jobs:
ImageBuildAndPush:
name: Build Image and Push
uses: uc-cdis/.github/.github/workflows/image_build_push.yaml@master
with:
BUILD_PLATFORMS: "linux/amd64"
Copy link
Contributor

@paulineribeyre paulineribeyre Mar 13, 2025

Choose a reason for hiding this comment

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

not building linux/arm64 isn't great for people with m1 macs trying to run images locally (like myself) - is this a recommendation from the SDET or PE teams?

Copy link
Contributor Author

@nss10 nss10 Mar 13, 2025

Choose a reason for hiding this comment

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

“It wasn’t—I mixed it up with the thread about _native.yaml. I’ll update it accordingly.”

authz = flask.current_app.auth.auth_request(
jwt=jwt, service="sheepdog", methods=roles, resources=[resource]
)
cache_key = str(hash((jwt, "sheepdog", tuple(roles), (resource))))
Copy link
Contributor

@paulineribeyre paulineribeyre Mar 13, 2025

Choose a reason for hiding this comment

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

i'm a bit unsure about using the hash function here, we shouldn't take any risks with authz checks and from a quick google search there's a "high" risk of collision. A long key that includes the whole JWT worked fine for orthanc, could we do that here? If not, maybe the username or user ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! I didn’t realize hash was so prone to collisions. I’ll replace it with an underscore-separated string, similar to how it’s handled in Orthanc.

Comment on lines 131 to 137
# The caching library raises an UnboundLocalError during unit tests due to a known bug.
# This workaround prevents the error from occurring in test environments.
except UnboundLocalError as e:
logger.error("Catching error caused by caching library: {}".format(e))
authz = flask.current_app.auth.auth_request(
jwt=jwt, service="sheepdog", methods=roles, resources=[resource]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

any way we could move this to the unit tests, then?
and please add a link to the known bug - github issue or such

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’ve removed the try-except block entirely and instead fixed mock_auth_request to ensure it returns True or raises an AuthzError as expected.

Comment on lines 127 to 129
logger.info(
f"Retrieveing response from arborist: {authz} with {type(authz)=}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Retrieving typo - and i don't think this log is necessary, it would clutter the sheepdog logs. I think you're using it to debug unit tests? let's remove it when you're done

Comment on lines 3 to 10
workers = 2
preload_app = False
user = "gen3"
group = "gen3"
timeout = 300
keepalive = 2
keepalive_timeout = 5
graceful_timeout = 45
keepalive = 10
pidfile = "/sheepdog/gunicorn.pid"
Copy link
Contributor

Choose a reason for hiding this comment

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

could/should these changes be moved to a different PR?

Copy link
Contributor Author

@nss10 nss10 Mar 13, 2025

Choose a reason for hiding this comment

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

I think in terms of bigger picture all of them are solving a single problem of solving sheepdog crashes. But I get it, i can put that as a different PR to avoid confusion in the future.

Here we go -- Update gunicorn config to improve sheepdog's responsiveness #433

@nss10 nss10 requested a review from paulineribeyre March 13, 2025 20:56
Copy link

filepath $$\textcolor{#23d18b}{\tt{passed}}$$ $$\textcolor{#ffa500}{\tt{skipped}}$$ SUBTOTAL
$$\textcolor{#ffa500}{\tt{tests/test\_graph\_submit\_and\_query.py}}$$ $$\textcolor{#23d18b}{\tt{13}}$$ $$\textcolor{#ffa500}{\tt{1}}$$ $$\textcolor{#ffa500}{\tt{14}}$$
$$\textcolor{#ffa500}{\tt{tests/test\_data\_upload.py}}$$ $$\textcolor{#23d18b}{\tt{8}}$$ $$\textcolor{#ffa500}{\tt{1}}$$ $$\textcolor{#ffa500}{\tt{9}}$$
$$\textcolor{#ffa500}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{21}}$$ $$\textcolor{#ffa500}{\tt{2}}$$ $$\textcolor{#ffa500}{\tt{23}}$$

Please find the detailed integration test report here

Please find the ci env pod logs here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants