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

OIDC: Using caching to retrieve issuer configs. #1141

Merged
merged 2 commits into from
Dec 20, 2019

Conversation

eloycoto
Copy link
Contributor

@eloycoto eloycoto commented Nov 19, 2019

This commit fix issues with the flipflop connections to the issuer.
Multiple users have hundred of services with the same issuer, and
multiple requests happen at the same time.

This commit has two pourposes:

  • Make the issuer config retrieve a bit faster in the APIcast and do
    not make multiple requests to retrieve the same info.
  • In case that the issuer has internment issues, avoid that one service
    loads correctly, and others do not load at all.

Fix THREESCALE-3809

  • Should this caching be enabled with an Env variable?

@eloycoto eloycoto requested a review from a team as a code owner November 19, 2019 14:52
@eloycoto eloycoto requested a review from a team as a code owner December 17, 2019 11:06
This commit fix issues with the flipflop connections to the issuer.
Multiple users have hundred of services with the same issuer, and
multiple requests happen at the same time.

This commit has two pourposes:
  - Make the issuer config retrieve a bit faster in the APIcast and do
  not make multiple requests to retrieve the same info.
  - In case that the issuer has internment issues, avoid that one service
  loads correctly, and others does not load at all.

Fix THREESCALE-3809

Signed-off-by: Eloy Coto <[email protected]>
Copy link
Contributor

@porueesq porueesq left a comment

Choose a reason for hiding this comment

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

A couple of suggestions.

location /api/ {
echo 'yay, api backend';
}
--- request
Copy link
Contributor

Choose a reason for hiding this comment

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

Question. Maybe I'm missing something. The configuration reload mode is set to "lazy". Shouldn't there be one request to each of the services? And then verify somehow that in one of the request there's the "retrieving OIDC" message and in the other it's not there because it should be cached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no host, so retrieve the configuration for both.


function _M.new_with_http_client(http_client)
local self = setmetatable({ http_client = http_client }, mt)
self:init_cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I think that it's a bit unexpected that the method assigns self.something.
I'd change this to self.cache = init_cache() or just inline if it's simple enough.

function _M:save_issuer_in_cache(issuer, config, ttl)
local expires = tonumber(ttl) or 0

if not self.cache then
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick : I think it's better to move this if to be the first line of the function.
I think it clearly says that if there's no cache there's nothing else to do.

Copy link
Contributor

@porueesq porueesq left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Eloy Coto <[email protected]>
@eloycoto eloycoto merged commit e30d476 into 3scale:master Dec 20, 2019
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