Skip to content

Conversation

@roidelapluie
Copy link
Member

In order to improve the response time with basic authentication, cache
the results of bcrypt in memory.

This is a draft to see if that approach would be acceptable.

The code is coming from Caddy, and I have fullfiled the attribution
required by Apache-2 by adding the copyright to our headers:
caddyserver/caddy@9a7756c

Signed-off-by: Julien Pivotto [email protected]

@roidelapluie roidelapluie requested a review from SuperQ January 13, 2021 23:54
@roidelapluie
Copy link
Member Author

roidelapluie commented Jan 13, 2021

If the general approach is fine I will add specific tests.

In order to improve the response time with basic authentication, cache
the results of bcrypt in memory.

This is a draft to see if that approach would be acceptable.

The code is coming from Caddy, and I have fullfiled the attribution
required by Apache-2 by adding the copyright to our headers:
caddyserver/caddy@9a7756c

Signed-off-by: Julien Pivotto <[email protected]>
@roidelapluie
Copy link
Member Author

is this good to go?

@roidelapluie
Copy link
Member Author

Hold on.

Julien Pivotto added 2 commits January 16, 2021 15:57
Add getters and setters
Add a lock around bcrypt
Fix the unhappy scenario

Signed-off-by: Julien Pivotto <[email protected]>
Signed-off-by: Julien Pivotto <[email protected]>
@roidelapluie
Copy link
Member Author

OK now it works as expected. I have added getters and setters to release locks early.

I have also added a Lock around bcrypt so that we only run one at the same time.

@roidelapluie roidelapluie requested a review from SuperQ January 16, 2021 15:05
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@roidelapluie roidelapluie merged commit fa3ecab into prometheus:master Jan 16, 2021
logger log.Logger
cache *cache
// bcryptMtx is there to ensure that bcrypt.CompareHashAndPassword is run
// only once in parallel as this is CPU expansive.
Copy link
Member

Choose a reason for hiding this comment

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

expansive → intensive?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦🏻 I will fix it

@roidelapluie roidelapluie mentioned this pull request Jan 16, 2021
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