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

Return existing records on update failure #1563

Merged
merged 10 commits into from
Jul 7, 2021
Merged

Return existing records on update failure #1563

merged 10 commits into from
Jul 7, 2021

Conversation

spgreenberg
Copy link
Contributor

This PR addresses a failure condition reported here: #1531

When an IDP with remote metadata fails to update, UAA enters a loop and eventually crashes. This update changes the cache implementation to use refreshAfterWrite to return the existing record on failure rather than failing: https://guava.dev/releases/19.0/api/docs/com/google/common/cache/CacheBuilder.html#refreshAfterWrite(long,%20java.util.concurrent.TimeUnit)

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/177913289

The labels on this github issue will be updated when the story is started.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 26, 2021

CLA Signed

The committers are authorized under a signed CLA.

@ChrisMcGowan
Copy link

Morning - any updates on when we can expect some movement on this PR? Thanks

Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

gernel quest about new class, why was the fix / extension of ExpiringUrlCache not an option ?

@@ -95,6 +95,7 @@ dependencies {
testImplementation(libraries.tomcatJdbc)

testImplementation(libraries.jsonPathAssert)
testImplementation('com.google.guava:guava-testlib:30.1.1-jre')
Copy link
Member

Choose a reason for hiding this comment

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

uaa uses spring deps, e.g. 30.0.? so is this really needed

Choose a reason for hiding this comment

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

Hi @strehle - Steve did this work on the fix as part of a contract with cloud.gov. At this time that contract is done so Steve is unable to respond on behalf of cloud.gov to your questions so i'm going to do the best I can with what I know.

The most likely reason for the one test library being a higher rev then the spring deps was it is the latest version of that dependency and in general when the cloud.gov team works on an issue we try to update what we can to the latest and greatest to reduce the risk of having open issues/CVEs on code bases.

As far the fix path - again I can't speak for Steve as to why he choose the path for the fix he did vs your question on ExpiringUrlCache. At the end of the day it solved the issue in what he probably felt was the best method based on his exp and the issue at hand.

At this point is there any reason this can not be approved and merged into the code base for a future release? The issue at hand has caused some login outages on the cloud.gov platform when other IDP providers outside of cloud.gov's control have had outages to their IDP systems causing uaa to go into a crash loop.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

please pull uaa develop branch into yours and then you can omit this version but only use
testImplementation('com.google.guava:guava-testlib')

@strehle
Copy link
Member

strehle commented Jun 28, 2021

@peterhaochen47 dont know about current status about how decides for a merge, so let it up to you

strehle and others added 4 commits June 30, 2021 11:45
- remove orphan class
- use new class in all tests
- dependency cosmetics
@strehle
Copy link
Member

strehle commented Jun 30, 2021

cleaned the PR, see
strehle@b6859c0
or
https://github.com/strehle/uaa/commits/new
because

  • remove orphan class
  • use new class in all tests
  • dependency cosmetics

The community will appreciate your efforts, but we should keep the usage clean. You created a new cache class and changed bean definition from ExpiringUrlCache to StaleUrlCache, so far ok.
Some existing tests still used ExpiringUrlCache but in runtime only StaleUrlCache is used, therefore I deleted ExpiringUrlCache

If you agree I will merge https://github.com/strehle/uaa/tree/new into your develop.

@peterhaochen47 review from my side

  • test suite runs
  • manual tests with OIDC still working

@peterhaochen47
Copy link
Member

peterhaochen47 commented Jun 30, 2021

Agreed with @strehle above. By removing @Component for ExpiringUrlCache, ExpiringUrlCache will stop being a Spring-managed bean, so StaleUrlCache will be the implementation that takes effect. So why not clean up ExpiringUrlCache?

@strehle strehle linked an issue Jul 1, 2021 that may be closed by this pull request
* strehle/new:
  update
  cleanup PR - remove orphan class - use new class in all tests - dependency cosmetics

# Conflicts:
#	dependencies.gradle
* origin/develop:
  Bump jasmine-core from 3.7.1 to 3.8.0 in /uaa (#1598)
  Bump jasmine from 3.7.0 to 3.8.0 in /uaa (#1597)
  Bump dependency (#1596)
  Performance optimation: Prevent expensive duplicate key exception and use upsert instead (#1562)
  Refactor: query minimal user information everywhere authorities not needed (#1322)
  Allow to use Account Chooser without Idp Discovery (#1550)
  Bump k8s.io/client-go from 0.21.0 to 0.21.2 in /k8s (#1586)
  Bump commons-io from 2.7 to 2.10.0 (#1582)
@strehle
Copy link
Member

strehle commented Jul 2, 2021

updates on when we can expect some movement on this PR?

please check if my rebase and cleanup is ok for you.

@ChrisMcGowan
Copy link

updates on when we can expect some movement on this PR?

please check if my rebase and cleanup is ok for you.

Looks good to me @strehle - thanks

@peterhaochen47
Copy link
Member

LGTM

@strehle strehle merged commit f26ca61 into cloudfoundry:develop Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SAML IDP Provider metadata site down causes exceptions
5 participants