From 2b2ca0f355d3117ab1786f04ad23d5fd12354f38 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Mon, 4 Dec 2017 14:31:47 +0000 Subject: [PATCH 1/2] Check that the upstream repo can be written to Our "gitsync" mechanism relies on being able to push a tag back to the upstream repository. But in the daemon, we consider it ready to go once we've managed to clone the repository. This results in confusing errors: if the repo is read-only (either because the deploy key is, or because it's an `https` URL) then syncing will succeed in effect, but report problems. This commit adds a check that we can write to the upstream repo, before considering the daemon ready to go. The check is that we can push an otherwise-meaningless tag (then delete it) -- this being a fairly innocuous write operation. --- cmd/fluxd/main.go | 5 +++++ git/operations.go | 14 ++++++++++++++ git/repo.go | 10 ++++++++++ 3 files changed, 29 insertions(+) diff --git a/cmd/fluxd/main.go b/cmd/fluxd/main.go index b5b066f53..747bb6a60 100644 --- a/cmd/fluxd/main.go +++ b/cmd/fluxd/main.go @@ -370,6 +370,11 @@ func main() { ctx, cancel := context.WithTimeout(context.Background(), git.DefaultCloneTimeout) working, err := repo.Clone(ctx, gitConfig) cancel() + if err == nil { + ctx, cancel = context.WithTimeout(context.Background(), git.DefaultCloneTimeout) + err = working.CheckOriginWritable(ctx) + cancel() + } if err != nil { if checker == nil { checker = checkForUpdates(clusterVersion, "false", updateCheckLogger) diff --git a/git/operations.go b/git/operations.go index a0fe94343..39b3351c4 100644 --- a/git/operations.go +++ b/git/operations.go @@ -42,6 +42,20 @@ func clone(ctx context.Context, workingDir string, keyRing ssh.KeyRing, repoURL, return repoPath, nil } +// checkPush sanity-checks that we can write to the upstream repo with +// the given keyring (being able to `clone` is an adequate check that +// we can read the upstream). +func checkPush(ctx context.Context, keyRing ssh.KeyRing, workingDir, upstream string) error { + // --force just in case we fetched the tag from upstream when cloning + if err := execGitCmd(ctx, workingDir, nil, nil, "tag", "--force", CheckPushTag); err != nil { + return errors.Wrap(err, "tag for write check") + } + if err := execGitCmd(ctx, workingDir, keyRing, nil, "push", "--force", upstream, "tag", CheckPushTag); err != nil { + return errors.Wrap(err, "attempt to push tag") + } + return execGitCmd(ctx, workingDir, keyRing, nil, "push", "-d", upstream, "tag", CheckPushTag) +} + func commit(ctx context.Context, workingDir string, commitAction *CommitAction) error { commitAuthor := commitAction.Author if commitAuthor != "" { diff --git a/git/repo.go b/git/repo.go index 09edb020e..7c43b89cf 100644 --- a/git/repo.go +++ b/git/repo.go @@ -16,6 +16,7 @@ import ( const ( DefaultCloneTimeout = 2 * time.Minute + CheckPushTag = "flux-write-check" ) var ( @@ -141,6 +142,15 @@ func (c *Checkout) ManifestDir() string { return filepath.Join(c.Dir, c.repo.Path) } +// CheckOriginWritable tests that we can write to the origin +// repository; we need to be able to do this to push the sync tag, for +// example. +func (c *Checkout) CheckOriginWritable(ctx context.Context) error { + c.Lock() + defer c.Unlock() + return checkPush(ctx, c.repo.KeyRing, c.Dir, c.repo.URL) +} + // CommitAndPush commits changes made in this checkout, along with any // extra data as a note, and pushes the commit and note to the remote repo. func (c *Checkout) CommitAndPush(ctx context.Context, commitAction *CommitAction, note *Note) error { From c02e0725fb9a580a60af3253664d4d84870c9c24 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Mon, 4 Dec 2017 15:55:27 +0000 Subject: [PATCH 2/2] Generate helpful error on failed git write check While starting up, the daemon continually checks whether it can clone the upstream repo, and if so, whether it can write to it. If this results in an error, it stores this away and keeps trying. Until it succeeds, when asked to do anything related to the git repo (i.e., almost anything), it responds with the stored error. In that interim, it's possible that a user will ask for something that needs a cloned repo; and if so, a helpful error is .. helpful. --- git/errors.go | 47 +++++++++++++++++++++++++++++++++++++++++++++-- git/repo.go | 5 ++++- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/git/errors.go b/git/errors.go index 39d63d9df..d22df4e83 100644 --- a/git/errors.go +++ b/git/errors.go @@ -2,6 +2,7 @@ package git import ( "errors" + "strings" fluxerr "github.com/weaveworks/flux/errors" ) @@ -24,7 +25,7 @@ func CloningError(url string, actual error) error { return &fluxerr.Error{ Type: fluxerr.User, Err: actual, - Help: `Problem cloning your git repository + Help: `Could not clone the upstream git repository There was a problem cloning your git repository, @@ -44,6 +45,47 @@ cross-check with the fingerprint given by } } +func ErrUpstreamNotWritable(url string, actual error) error { + help := `Could not write to upstream repository + +To keep track of synchronisation, the flux daemon must be able to +write to the upstream git repository. +` + if strings.HasPrefix(url, "http://") || + strings.HasPrefix(url, "https://") { + help = help + ` +Usually, git URLs starting with "http://" or "https://" will not work +well with flux, because they require the user to supply credentials +interactively. If possible, use an SSH URL (starting with "ssh://", or +of the form "user@host:path/to/repo"). +` + } else { + help = help + ` +This failure may be due to the SSH (deploy) key used by the daemon not +having write permission. You can see the key used, with + + fluxctl identity + +In GitHub, please check via the repository settings that the deploy +key is "Read/write". You can cross-check the fingerprint with that +given by + + fluxctl identity --fingerprint + +If the key is present but read-only, you will need to delete it and +create a new deploy key. To create a new one, use + + fluxctl identity --regenerate +` + } + + return &fluxerr.Error{ + Type: fluxerr.User, + Err: actual, + Help: help, + } +} + func PushError(url string, actual error) error { return &fluxerr.Error{ Type: fluxerr.User, @@ -57,7 +99,8 @@ If this has worked before, it most likely means a fast-forward push was not possible. It is safe to try again. If it has not worked before, this probably means that the repository -exists but the deploy key provided doesn't have write permission. +exists but the SSH (deploy) key provided doesn't have write +permission. In GitHub, please check via the repository settings that the deploy key is "Read/write". You can cross-check the fingerprint with that diff --git a/git/repo.go b/git/repo.go index 7c43b89cf..60b579016 100644 --- a/git/repo.go +++ b/git/repo.go @@ -148,7 +148,10 @@ func (c *Checkout) ManifestDir() string { func (c *Checkout) CheckOriginWritable(ctx context.Context) error { c.Lock() defer c.Unlock() - return checkPush(ctx, c.repo.KeyRing, c.Dir, c.repo.URL) + if err := checkPush(ctx, c.repo.KeyRing, c.Dir, c.repo.URL); err != nil { + return ErrUpstreamNotWritable(c.repo.URL, err) + } + return nil } // CommitAndPush commits changes made in this checkout, along with any