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

Adding local file cache for credential helper #3

Merged
merged 2 commits into from
Jul 5, 2016

Conversation

sentientmonkey
Copy link
Contributor

This provides a JSON file cache that's serialized/deserialized
for each access. The local file cache tokens are kept for the half-life
of the token lifetime (i.e. a 12 hour token will get expired after 6
hours). Users may also opt-out of local cache with an environment
variable set of AWS_ECR_DISABLE_CACHE.

// file access. There is not guarantee here for handling multiple writes at once since there is no out of process locking.
func (f *fileCredentialCache) save(registryCache *RegistryCache) error {
defer log.Flush()
file, err := ioutil.TempFile("", "ecr-credential-cache")
Copy link
Contributor

@samuelkarp samuelkarp Jun 28, 2016

Choose a reason for hiding this comment

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

Can you change this to use a temp file in the ~/.ecr directory? os.Rename will fail if the temp file and destination file are not on the same filesystem, so creating the temp file in the same directory would help that problem.

dir, _ := homedir.Expand("~/.ecr")
file, err := ioutil.TempFile(dir, "ecr-credential-cache")

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 did not know that. 😞 Seems reasonable to keep all within ~/.ecr. I'll update, will inject homedir path into the file cache.

@samuelkarp
Copy link
Contributor

Looks good other than the temp file bit.

This provides a JSON file cache that's serialized/deserialized
for each access. The local file cache tokens are kept for the half-life
of the token lifetime (i.e. a 12 hour token will get expired after 6
hours). Users may also opt-out of local cache with an environment
variable set of `AWS_ECR_DISABLE_CACHE`.
@sentientmonkey
Copy link
Contributor Author

Added temp file & rebased against master.

@samuelkarp
Copy link
Contributor

@sentientmonkey Looks like some unit test failures.

@sentientmonkey sentientmonkey force-pushed the master branch 2 times, most recently from 1452bc1 to 1515736 Compare July 5, 2016 17:43
@sentientmonkey
Copy link
Contributor Author

Looks like it was a deep comparison issue with times. 😭 Comparing each of the values seemed to do the trick. Not exactly sure why this was failing in travis vs. locally.

@samuelkarp
Copy link
Contributor

👍

I think my one remaining concern here is garbage collection of expired cache entries when the access key changes (like how it will rotate on an EC2 instance profile), but we can follow up with that separately.

@sentientmonkey
Copy link
Contributor Author

Will create issue for cleaning up expired entries - seems reasonable to purge expired entries on load or save.

@sentientmonkey
Copy link
Contributor Author

Added: #6

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.

2 participants