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

Add deploy key for public repo #1301

Merged
merged 3 commits into from
Jan 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/gitops/add/clusters/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func getClusterCmdRunE(endpoint *string, client *resty.Client) func(*cobra.Comma
return cmderrors.ErrNoURL
}

url, err := gitproviders.NewRepoURL(flags.RepositoryURL)
url, err := gitproviders.NewRepoURL(flags.RepositoryURL, true)
if err != nil {
return fmt.Errorf("cannot parse url: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/gitops/delete/clusters/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func getClusterCmdRunE(endpoint *string, client *resty.Client) func(*cobra.Comma
return cmderrors.ErrNoURL
}

url, err := gitproviders.NewRepoURL(flags.RepositoryURL)
url, err := gitproviders.NewRepoURL(flags.RepositoryURL, true)
if err != nil {
return fmt.Errorf("cannot parse url: %w", err)
}
Expand Down
10 changes: 6 additions & 4 deletions cmd/gitops/install/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func installRunCmd(cmd *cobra.Command, args []string) error {
ctx := context.Background()
namespace, _ := cmd.Parent().Flags().GetString("namespace")

configURL, err := gitproviders.NewRepoURL(installParams.ConfigRepo)
configURL, err := gitproviders.NewRepoURL(installParams.ConfigRepo, true)
if err != nil {
return err
}
Expand Down Expand Up @@ -122,9 +122,11 @@ func installRunCmd(cmd *cobra.Command, args []string) error {
providerClient := internal.NewGitProviderClient(osysClient.Stdout(), osysClient.LookupEnv, auth.NewAuthCLIHandler, log)

gitClient, gitProvider, err = factory.GetGitClients(context.Background(), providerClient, services.GitConfigParams{
URL: installParams.ConfigRepo,
Namespace: namespace,
DryRun: installParams.DryRun,
// We need to set URL and ConfigRepo to the same value so a deploy key is created for public config repos
URL: installParams.ConfigRepo,
ConfigRepo: installParams.ConfigRepo,
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 dont love having to set these two values but this is the only solution short of rewriting the entire GetGitClients functionality and all the places that use it.

Namespace: namespace,
DryRun: installParams.DryRun,
})

if err != nil {
Expand Down
13 changes: 7 additions & 6 deletions cmd/internal/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ import (
"context"
"errors"
"fmt"
"io"
"os"

"github.com/fluxcd/go-git-providers/gitprovider"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/weaveworks/weave-gitops/pkg/gitproviders"
"github.com/weaveworks/weave-gitops/pkg/logger/loggerfakes"
"github.com/weaveworks/weave-gitops/pkg/services/auth"
"io"
"os"
)

const (
Expand Down Expand Up @@ -71,7 +72,7 @@ var _ = Describe("Get git provider", func() {
It("invalid token key returns an error", func() {
fakeLogger = &loggerfakes.FakeLogger{}
client = NewGitProviderClient(os.Stdout, fakeEnvLookupExists, fakeAuthHandlerFuncError, fakeLogger)
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@some-bucket.com/weaveworks/weave-gitops.git")
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@some-bucket.com/weaveworks/weave-gitops.git", false)

provider, err := client.GetProvider(repoUrl, fakeAccountGetterSuccess)
Expect(provider).To(BeNil())
Expand All @@ -86,7 +87,7 @@ var _ = Describe("Get git provider", func() {
BeforeEach(func() {
fakeLogger = &loggerfakes.FakeLogger{}
client = NewGitProviderClient(os.Stdout, fakeEnvLookupExists, fakeAuthHandlerFuncError, fakeLogger)
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@github.com/weaveworks/weave-gitops.git")
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@github.com/weaveworks/weave-gitops.git", false)
})

It("gitproviders.New returns an error", func() {
Expand All @@ -110,7 +111,7 @@ var _ = Describe("Get git provider", func() {
BeforeEach(func() {
fakeLogger = &loggerfakes.FakeLogger{}
client = NewGitProviderClient(os.Stdout, fakeEnvLookupExists, fakeAuthHandlerFuncError, fakeLogger)
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@gitlab.com/weaveworks/weave-gitops.git")
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@gitlab.com/weaveworks/weave-gitops.git", false)
})

It("gitproviders.New returns an error", func() {
Expand All @@ -134,7 +135,7 @@ var _ = Describe("Get git provider", func() {
fakeLogger = &loggerfakes.FakeLogger{
WarningfStub: func(fmtArg string, restArgs ...interface{}) {},
}
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@github.com/weaveworks/weave-gitops.git")
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@github.com/weaveworks/weave-gitops.git", false)
})

AfterEach(func() {
Expand Down
8 changes: 4 additions & 4 deletions pkg/flux/flux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ var _ = Describe("CreateSourceGit", func() {
return []byte("out"), nil
}

repoUrl, err := gitproviders.NewRepoURL("https://github.com/foo/my-name")
repoUrl, err := gitproviders.NewRepoURL("https://github.com/foo/my-name", false)
Expect(err).ShouldNot(HaveOccurred())
out, err := fluxClient.CreateSourceGit("my-name", repoUrl, "main", "my-secret", wego.DefaultNamespace)
Expect(err).ShouldNot(HaveOccurred())
Expand All @@ -100,7 +100,7 @@ var _ = Describe("CreateSourceGit", func() {
runner.RunStub = func(s1 string, s2 ...string) ([]byte, error) {
return []byte("out"), nil
}
repoUrl, err := gitproviders.NewRepoURL("ssh://git@github.com/foo/my-name")
repoUrl, err := gitproviders.NewRepoURL("ssh://git@github.com/foo/my-name", false)
Expect(err).ShouldNot(HaveOccurred())
out, err := fluxClient.CreateSourceGit("my-name", repoUrl, "main", "", wego.DefaultNamespace)
Expect(err).ShouldNot(HaveOccurred())
Expand All @@ -119,7 +119,7 @@ var _ = Describe("CreateSourceGit", func() {
return []byte("out"), nil
}

repoUrl, err := gitproviders.NewRepoURL("ssh://git@gitlab.com/foo/my-name")
repoUrl, err := gitproviders.NewRepoURL("ssh://git@gitlab.com/foo/my-name", false)
Expect(err).ShouldNot(HaveOccurred())
out, err := fluxClient.CreateSourceGit("my-name", repoUrl, "main", "", wego.DefaultNamespace)
Expect(err).ShouldNot(HaveOccurred())
Expand Down Expand Up @@ -244,7 +244,7 @@ var _ = Describe("CreateSecretGit", func() {
return []byte(`ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCh...`), nil
}

repoUrl, err := gitproviders.NewRepoURL("ssh://git@github.com/foo/bar.git")
repoUrl, err := gitproviders.NewRepoURL("ssh://git@github.com/foo/bar.git", false)
Expect(err).ShouldNot(HaveOccurred())
out, err := fluxClient.CreateSecretGit("my-secret", repoUrl, wego.DefaultNamespace)
Expect(err).ShouldNot(HaveOccurred())
Expand Down
2 changes: 1 addition & 1 deletion pkg/gitproviders/provider_org_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var _ = Describe("Org Provider", func() {
}

var err error
repoUrl, err = NewRepoURL("http://github.com/owner/repo-name")
repoUrl, err = NewRepoURL("http://github.com/owner/repo-name", false)
Expect(err).ToNot(HaveOccurred())
})

Expand Down
2 changes: 1 addition & 1 deletion pkg/gitproviders/provider_user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ var _ = Describe("User Provider", func() {
}

var err error
repoUrl, err = NewRepoURL("http://github.com/owner/repo-name")
repoUrl, err = NewRepoURL("http://github.com/owner/repo-name", false)
Expect(err).ToNot(HaveOccurred())
})

Expand Down
32 changes: 19 additions & 13 deletions pkg/gitproviders/repo_url.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,16 @@ const RepositoryURLProtocolHTTPS RepositoryURLProtocol = "https"
const RepositoryURLProtocolSSH RepositoryURLProtocol = "ssh"

type RepoURL struct {
repoName string
owner string
url *url.URL
normalized string
provider GitProviderName
protocol RepositoryURLProtocol
repoName string
owner string
url *url.URL
normalized string
provider GitProviderName
protocol RepositoryURLProtocol
isConfigRepo bool
}

func NewRepoURL(uri string) (RepoURL, error) {
func NewRepoURL(uri string, isConfigRepo bool) (RepoURL, error) {
providerName, err := detectGitProviderFromUrl(uri, ViperGetStringMapString("git-host-types"))
if err != nil {
return RepoURL{}, fmt.Errorf("could not get provider name from URL %s: %w", uri, err)
Expand All @@ -52,12 +53,13 @@ func NewRepoURL(uri string) (RepoURL, error) {
}

return RepoURL{
repoName: utils.UrlToRepoName(uri),
owner: owner,
url: u,
normalized: normalized,
provider: providerName,
protocol: protocol,
repoName: utils.UrlToRepoName(uri),
owner: owner,
url: u,
normalized: normalized,
provider: providerName,
protocol: protocol,
isConfigRepo: isConfigRepo,
}, nil
}

Expand Down Expand Up @@ -85,6 +87,10 @@ func (n RepoURL) Protocol() RepositoryURLProtocol {
return n.protocol
}

func (n RepoURL) IsConfigRepo() bool {
return n.isConfigRepo
}

func getOwnerFromUrl(url url.URL, providerName GitProviderName) (string, error) {
url.Path = strings.TrimPrefix(url.Path, "/")

Expand Down
2 changes: 1 addition & 1 deletion pkg/gitproviders/repo_url_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ var _ = DescribeTable("NewRepoURL", func(input, gitProviderEnv string, expected
if gitProviderEnv != "" {
viper.Set("git-host-types", gitProviderEnv)
}
result, err := NewRepoURL(input)
result, err := NewRepoURL(input, false)
Expect(err).NotTo(HaveOccurred())

Expect(result.String()).To(Equal(expected.s))
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/internal/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ var _ = Describe("Get git provider", func() {
var repoUrl gitproviders.RepoURL
BeforeEach(func() {
client = NewGitProviderClient(fakeToken)
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@github.com/weaveworks/weave-gitops.git")
repoUrl, _ = gitproviders.NewRepoURL("ssh://git@github.com/weaveworks/weave-gitops.git", false)
})

It("gitproviders.New throws an error", func() {
Expand Down
6 changes: 3 additions & 3 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,14 +299,14 @@ func (s *applicationServer) AddApplication(ctx context.Context, msg *pb.AddAppli
return nil, grpcStatus.Errorf(codes.Unauthenticated, "token error: %s", err.Error())
}

appUrl, err := gitproviders.NewRepoURL(msg.Url)
appUrl, err := gitproviders.NewRepoURL(msg.Url, false)
if err != nil {
return nil, grpcStatus.Errorf(codes.InvalidArgument, "unable to parse app url %q: %s", msg.Url, err)
}

var configRepo gitproviders.RepoURL
if msg.ConfigRepo != "" {
configRepo, err = gitproviders.NewRepoURL(msg.ConfigRepo)
configRepo, err = gitproviders.NewRepoURL(msg.ConfigRepo, true)
if err != nil {
return nil, grpcStatus.Errorf(codes.InvalidArgument, "unable to parse config url %q: %s", msg.ConfigRepo, err)
}
Expand Down Expand Up @@ -650,7 +650,7 @@ func (s *applicationServer) Authenticate(_ context.Context, msg *pb.Authenticate
}

func (s *applicationServer) ParseRepoURL(ctx context.Context, msg *pb.ParseRepoURLRequest) (*pb.ParseRepoURLResponse, error) {
u, err := gitproviders.NewRepoURL(msg.Url)
u, err := gitproviders.NewRepoURL(msg.Url, false)
if err != nil {
return nil, grpcStatus.Errorf(codes.InvalidArgument, "could not parse url: %s", err.Error())
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/services/app/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (a *AppSvc) updateParametersIfNecessary(ctx context.Context, gitProvider gi
default:
var err error

appRepoUrl, err = gitproviders.NewRepoURL(params.Url)
appRepoUrl, err = gitproviders.NewRepoURL(params.Url, false)
if err != nil {
return params, fmt.Errorf("error normalizing url: %w", err)
}
Expand All @@ -161,7 +161,7 @@ func (a *AppSvc) updateParametersIfNecessary(ctx context.Context, gitProvider gi

// making sure the config url is in good format
if models.IsExternalConfigRepo(params.ConfigRepo) {
configRepoUrl, err := gitproviders.NewRepoURL(params.ConfigRepo)
configRepoUrl, err := gitproviders.NewRepoURL(params.ConfigRepo, true)
if err != nil {
return params, fmt.Errorf("error normalizing url: %w", err)
}
Expand Down Expand Up @@ -236,7 +236,7 @@ func makeApplication(params AddParams) (models.Application, error) {
if models.SourceType(params.SourceType) == models.SourceTypeHelm {
helmSourceURL = params.Url
} else {
gitSourceURL, err = gitproviders.NewRepoURL(params.Url)
gitSourceURL, err = gitproviders.NewRepoURL(params.Url, false)
if err != nil {
return models.Application{}, err
}
Expand All @@ -245,7 +245,7 @@ func makeApplication(params AddParams) (models.Application, error) {
configRepo := gitSourceURL

if params.ConfigRepo != "" {
curl, err := gitproviders.NewRepoURL(params.ConfigRepo)
curl, err := gitproviders.NewRepoURL(params.ConfigRepo, true)
if err != nil {
return models.Application{}, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/app/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (a *AppSvc) GetCommits(gitProvider gitproviders.GitProvider, params CommitP
return nil, fmt.Errorf("unable to get commits for a helm chart")
}

repoUrl, err := gitproviders.NewRepoURL(application.Spec.URL)
repoUrl, err := gitproviders.NewRepoURL(application.Spec.URL, false)
if err != nil {
return nil, fmt.Errorf("error creating normalized url: %w", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/services/applicationv2/fetcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func translateApp(app wego.Application) (models.Application, error) {
)

if wego.DeploymentType(app.Spec.SourceType) == wego.DeploymentType(wego.SourceTypeGit) {
appRepoUrl, err = gitproviders.NewRepoURL(app.Spec.URL)
appRepoUrl, err = gitproviders.NewRepoURL(app.Spec.URL, false)
if err != nil {
return models.Application{}, err
}
Expand All @@ -92,7 +92,7 @@ func translateApp(app wego.Application) (models.Application, error) {
}

if models.IsExternalConfigRepo(app.Spec.ConfigRepo) {
configRepoUrl, err = gitproviders.NewRepoURL(app.Spec.ConfigRepo)
configRepoUrl, err = gitproviders.NewRepoURL(app.Spec.ConfigRepo, true)
if err != nil {
return models.Application{}, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (a *authSvc) provisionDeployKey(ctx context.Context, targetName string, nam
return nil, fmt.Errorf("error getting repo visibility: %w", err)
}

if *visibility == gitprovider.RepositoryVisibilityPublic {
if *visibility == gitprovider.RepositoryVisibilityPublic && !repo.IsConfigRepo() {
return nil, nil
}

Expand Down
17 changes: 10 additions & 7 deletions pkg/services/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,11 @@ var _ = Describe("auth", func() {
var namespace *corev1.Namespace
testClustername := "test-cluster"
repoUrlString := "ssh://git@github.com/my-org/my-repo.git"
repoUrl, err := gitproviders.NewRepoURL(repoUrlString)
configRepoUrl, err := gitproviders.NewRepoURL(repoUrlString, true)
Expect(err).NotTo(HaveOccurred())
repoUrl, err := gitproviders.NewRepoURL(repoUrlString, false)
Expect(err).NotTo(HaveOccurred())

BeforeEach(func() {
namespace = &corev1.Namespace{}
namespace.Name = "kube-test-" + rand.String(5)
Expand All @@ -50,7 +53,7 @@ var _ = Describe("auth", func() {
)
BeforeEach(func() {
ctx = context.Background()
secretName = automation.CreateRepoSecretName(repoUrl)
secretName = automation.CreateRepoSecretName(configRepoUrl)
Expect(err).NotTo(HaveOccurred())
osysClient = osys.New()
gp = gitprovidersfakes.FakeGitProvider{}
Expand All @@ -65,7 +68,7 @@ var _ = Describe("auth", func() {
}
})
It("create and stores a deploy key if none exists", func() {
_, err := as.CreateGitClient(ctx, repoUrl, testClustername, namespace.Name, false)
_, err := as.CreateGitClient(ctx, configRepoUrl, testClustername, namespace.Name, false)
Expect(err).NotTo(HaveOccurred())
sn := SecretName{Name: secretName, Namespace: namespace.Name}
secret := &corev1.Secret{}
Expand All @@ -75,7 +78,7 @@ var _ = Describe("auth", func() {
Expect(secret.StringData["identity.pub"]).NotTo(BeNil())
})
It("doesn't create a deploy key when dry-run is true", func() {
_, err := as.CreateGitClient(ctx, repoUrl, testClustername, namespace.Name, true)
_, err := as.CreateGitClient(ctx, configRepoUrl, testClustername, namespace.Name, true)
Expect(err).NotTo(HaveOccurred())
sn := SecretName{Name: secretName, Namespace: namespace.Name}
secret := &corev1.Secret{}
Expand All @@ -89,7 +92,7 @@ var _ = Describe("auth", func() {
Expect(err).NotTo(HaveOccurred())
Expect(k8sClient.Create(ctx, secret)).To(Succeed())

_, err = as.CreateGitClient(ctx, repoUrl, testClustername, namespace.Name, false)
_, err = as.CreateGitClient(ctx, configRepoUrl, testClustername, namespace.Name, false)
Expect(err).NotTo(HaveOccurred())
// We should NOT have uploaded anything since the key already exists
Expect(gp.UploadDeployKeyCallCount()).To(Equal(0))
Expand All @@ -98,15 +101,15 @@ var _ = Describe("auth", func() {
gp.DeployKeyExistsReturns(true, nil)
sn := SecretName{Name: secretName, Namespace: namespace.Name}

_, err = as.CreateGitClient(ctx, repoUrl, testClustername, namespace.Name, false)
_, err = as.CreateGitClient(ctx, configRepoUrl, testClustername, namespace.Name, false)
Expect(err).NotTo(HaveOccurred())

newSecret := &corev1.Secret{}
Expect(k8sClient.Get(ctx, sn.NamespacedName(), newSecret)).To(Succeed())
Expect(gp.UploadDeployKeyCallCount()).To(Equal(1))
})

It("avoids deploying key for public repos", func() {
It("avoids deploying key for non config repos", func() {
gp.GetRepoVisibilityReturns(gitprovider.RepositoryVisibilityVar(gitprovider.RepositoryVisibilityPublic), nil)

_, err = as.CreateGitClient(ctx, repoUrl, testClustername, namespace.Name, false)
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/automation/automation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var (
)

func createRepoURL(url string) gitproviders.RepoURL {
repoURL, err := gitproviders.NewRepoURL(url)
repoURL, err := gitproviders.NewRepoURL(url, false)
Expect(err).NotTo(HaveOccurred())

return repoURL
Expand Down
Loading