Skip to content

blobserve: dynamically reload Docker auth config #15442

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
Merged
merged 1 commit into from
Jan 5, 2023
Merged

blobserve: dynamically reload Docker auth config #15442

merged 1 commit into from
Jan 5, 2023

Conversation

jenting
Copy link
Contributor

@jenting jenting commented Dec 20, 2022 •
edited
Loading

Description

AWS ECR requires regular secret rotation.
This PR adds hot-reloading authentication credentials to pull images from the Gitpod container registry.

Similar code change https://github.com/gitpod-io/gitpod/pull/14679/files#diff-9efd0bd7f5ebc628c240ccf20295bf47583097f0d6f426826a0dc5d18b96a096 for reference.

Related Issue(s)

Fixes #15426

How to test

  1. Open the workspace preview
  2. Update the secret gcp-sa-registry-auth with another docker credential
  3. Watch the blobserve log, and check the below log display 
    "message":"reloading file after change","path":"/mnt/pull-secret/pull-secret.json"

Release Notes

None

Documentation

None

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@jenting jenting requested a review from a team December 20, 2022 13:36
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-jenting-blobserve-watch-and-reload-15426.1 because the annotations in the pull request description changed
(with .werft/ from main)

@akosyakov
Copy link
Member

@jenting Would it be enough if we test for regressions? And rely for testing for external registries with you?

@jenting jenting marked this pull request as draft December 20, 2022 13:45
@jenting
Copy link
Contributor Author

jenting commented Dec 20, 2022

Would it be enough if we test for regressions?

I did not see any integration tests regards to blobserve. How do we test regression?

And rely for testing for external registries with you?

Sure

@jenting jenting force-pushed the jenting/blobserve-watch-and-reload-15426 branch from 5297968 to 8e266e1 Compare December 20, 2022 13:52
@jenting
Copy link
Contributor Author

jenting commented Dec 20, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-jenting-blobserve-watch-and-reload-15426.4
(with .werft/ from main)

@jenting
blobserve: dynamically reload Docker auth config
429de9e 
Signed-off-by: JenTing Hsiao <hsiaoairplane@gmail.com>
@jenting jenting force-pushed the jenting/blobserve-watch-and-reload-15426 branch from 8e266e1 to 429de9e Compare December 21, 2022 01:50
@jenting jenting changed the title blobserve: Dynamically reload Docker auth config blobserve: dynamically reload Docker auth config Dec 21, 2022
@jenting jenting marked this pull request as ready for review December 21, 2022 02:25
@kylos101
Copy link
Contributor

@aledbf for 👀

@stale
Copy link

stale bot commented Dec 31, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added meta: stale This issue/PR is stale and will be closed soon and removed meta: stale This issue/PR is stale and will be closed soon labels Dec 31, 2022
@iQQBot
Copy link
Contributor

iQQBot commented Jan 4, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-jenting-blobserve-watch-and-reload-15426.6
(with .werft/ from main)

akosyakov
akosyakov approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I did not try to change credentials, but verified for regressions that first read still works.

/hold

please unhold when are ready to merge

components/blobserve/cmd/run.go
}

reg := prometheus.NewRegistry()

resolverProvider := func() remotes.Resolver {
var resolverOpts docker.ResolverOptions

dockerCfgMu.RLock()
defer dockerCfgMu.RUnlock()
Copy link
Member

Choose a reason for hiding this comment

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

(nit) wonder whether mutex really needed or capturing local reference can be enough

components/blobserve/cmd/run.go
}
fr, err := os.OpenFile(fn, os.O_RDONLY, 0)
if err != nil {
log.WithError(err).Fatal("cannot read docker auth config")
Copy link
Member

Choose a reason for hiding this comment

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

Should we really fail blobserve or keep using previous credentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on how earlier we want to catch the problem.
Fails blob server catches the problem immediately. Using the previous credentials catches the problem when the user report.

Copy link
Member

Choose a reason for hiding this comment

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

@jenting it is just a question, feel free to merge

@jenting
Copy link
Contributor Author

jenting commented Jan 5, 2023

/unhold

@roboquat roboquat merged commit 43a9422 into main Jan 5, 2023
@roboquat roboquat deleted the jenting/blobserve-watch-and-reload-15426 branch January 5, 2023 10:37
@Siddhant-K-code Siddhant-K-code mentioned this pull request Jan 5, 2023
Merged
4 tasks
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov approved these changes

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed Change is completely running in production release-note-none size/M team: IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[blobserve] watch and reload dockerAuth config when using ECR
5 participants
@jenting @akosyakov @kylos101 @iQQBot @roboquat