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.

Sign up for GitHub

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

Jump to bottom

Change where the keychain dockercreds are read #1436

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Change where the keychain dockercreds are read #1436

wants to merge 2 commits into from

Conversation

xtreme-shane-lattanzio
Copy link
Contributor

Resolves #1353

@xtreme-shane-lattanzio xtreme-shane-lattanzio requested a review from a team as a code owner November 29, 2023 21:31
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023 •
edited

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (ae9fb34) 67.19% compared to head (f0a5528) 67.33%.

Files Patch % Lines
pkg/dockercreds/docker_creds.go 57.69% 9 Missing and 2 partials ⚠️
pkg/dockercreds/parse_secrets.go 77.77% 10 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1436      +/-   ##
==========================================
+ Coverage   67.19%   67.33%   +0.14%     
==========================================
  Files         140      140              
  Lines        8827     8887      +60     
==========================================
+ Hits         5931     5984      +53     
- Misses       2389     2395       +6     
- Partials      507      508       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chenbh
chenbh requested changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@chenbh chenbh left a comment

Choose a reason for hiding this comment

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

I don't think this is a good idea, opening and reading the file every time a secret is requested (aka every single reconcile) is going to be a lot of wasted resources.

What do you think of calling os.Stat() instead? We could cache the last modified time of the file and re-parse the file if the new modified time is newer. stat()should be a lot more performant than open() since it's only doing a lookup and not allocating anything that needs to be cleaned up (i.e. every open() must have a close())

@matthewmcnew
Copy link
Collaborator

Is this an opportunity a single reconcile loop could continually re-read them?

@xtreme-shane-lattanzio
Copy link
Contributor Author

@chenbh I can add a condition in the NewVolumeSecretKeychain method to os.stat the secret file but where are you thinking would we cache the time to compare against?

@chenbh
Copy link
Contributor

chenbh commented Jan 25, 2024

@xtreme-shane-lattanzio I would have the NewVolumeSecretKeychain return a struct that implements the authn.Keychain interface, and so every time Resolve is called, it can compare last modified time against its internal state. I'm not 100% sure, but you might need to secure it with a mutex cause the KeychainFactor is shared among all the reconcilers which will run in separate goroutines.

@matthewmcnew I'm not too convinced about the benefits of continuously reading the file especially since we don't expect the underlying Secret to be rotated often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenbh chenbh chenbh requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Secrets mounted as volumes are not watched for changes
4 participants
@xtreme-shane-lattanzio @codecov-commenter @matthewmcnew @chenbh