From d36bb71e613ad3a599a72bf78d53ab4beed5dc83 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 16 Sep 2021 09:33:20 +0200 Subject: [PATCH 1/4] add managed by pipelines-as-code label to pr --- pkg/pipelineascode/pipelineascode.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index 94e2402da..f985c8834 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -164,6 +164,7 @@ func Run(ctx context.Context, cs *cli.Clients, k8int cli.KubeInteractionIntf, ru // be aware of it when querying. refTomakeK8Happy := strings.ReplaceAll(runinfo.BaseBranch, "/", "-") pipelineRun.Labels = map[string]string{ + "app.kubernetes.io/managed-by": "pipelines-as-code", "pipelinesascode.tekton.dev/url-org": runinfo.Owner, "pipelinesascode.tekton.dev/url-repository": runinfo.Repository, "pipelinesascode.tekton.dev/sha": runinfo.SHA, From d7006118a2c76c52062794eba2ce41733109f7c6 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 16 Sep 2021 11:19:07 +0200 Subject: [PATCH 2/4] Fix secret auto creation We were not adding the SecretAutoCreation in runinfo after parsing. This part of code really need to get better tested --- pkg/cmd/pipelineascode/pipelineascode.go | 3 +- pkg/webvcs/github.go | 14 ++++--- pkg/webvcs/github_test.go | 49 +++++++++++++++++++----- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/pipelineascode/pipelineascode.go b/pkg/cmd/pipelineascode/pipelineascode.go index a7c80625a..8c6ce3589 100644 --- a/pkg/cmd/pipelineascode/pipelineascode.go +++ b/pkg/cmd/pipelineascode/pipelineascode.go @@ -91,8 +91,7 @@ func parsePayload(ctx context.Context, cs *cli.Clients, opts *pacpkg.Options) (* return nil, err } - payloadinfo, err := cs.GithubClient.ParsePayload(ctx, cs.Log, opts.RunInfo.EventType, - opts.RunInfo.TriggerTarget, string(payloadB)) + payloadinfo, err := cs.GithubClient.ParsePayload(ctx, cs.Log, opts.RunInfo, string(payloadB)) if err != nil { return &webvcs.RunInfo{}, err } diff --git a/pkg/webvcs/github.go b/pkg/webvcs/github.go index c270c3afe..ecaf51f0a 100644 --- a/pkg/webvcs/github.go +++ b/pkg/webvcs/github.go @@ -188,10 +188,11 @@ func (v GithubVCS) populateCommitInfo(ctx context.Context, runinfo *RunInfo) err } // ParsePayload parse payload event -func (v GithubVCS) ParsePayload(ctx context.Context, log *zap.SugaredLogger, eventType, triggerTarget, payload string) (*RunInfo, error) { +// TODO: this piece of code is just plain silly +func (v GithubVCS) ParsePayload(ctx context.Context, log *zap.SugaredLogger, optRunInfo RunInfo, payload string) (*RunInfo, error) { var runinfo RunInfo payload = payloadFix(payload) - event, err := github.ParseWebHook(eventType, []byte(payloadFix(payload))) + event, err := github.ParseWebHook(optRunInfo.EventType, []byte(payloadFix(payload))) if err != nil { return &runinfo, err } @@ -202,7 +203,7 @@ func (v GithubVCS) ParsePayload(ctx context.Context, log *zap.SugaredLogger, eve switch event := event.(type) { case *github.CheckRunEvent: - if triggerTarget == "issue-recheck" { + if optRunInfo.TriggerTarget == "issue-recheck" { runinfo, err = v.handleReRequestEvent(ctx, log, event) if err != nil { return &runinfo, err @@ -224,7 +225,7 @@ func (v GithubVCS) ParsePayload(ctx context.Context, log *zap.SugaredLogger, eve SHATitle: event.GetHeadCommit().GetMessage(), Sender: event.GetSender().GetLogin(), BaseBranch: event.GetRef(), - EventType: eventType, + EventType: optRunInfo.TriggerTarget, } runinfo.HeadBranch = runinfo.BaseBranch // in push events Head Branch is the same as Basebranch @@ -238,7 +239,7 @@ func (v GithubVCS) ParsePayload(ctx context.Context, log *zap.SugaredLogger, eve BaseBranch: event.GetPullRequest().Base.GetRef(), HeadBranch: event.GetPullRequest().Head.GetRef(), Sender: event.GetPullRequest().GetUser().GetLogin(), - EventType: eventType, + EventType: optRunInfo.EventType, } default: return &runinfo, errors.New("this event is not supported") @@ -250,7 +251,8 @@ func (v GithubVCS) ParsePayload(ctx context.Context, log *zap.SugaredLogger, eve } runinfo.Event = event - runinfo.TriggerTarget = triggerTarget + runinfo.TriggerTarget = optRunInfo.TriggerTarget + runinfo.SecretAutoCreation = optRunInfo.SecretAutoCreation return &runinfo, nil } diff --git a/pkg/webvcs/github_test.go b/pkg/webvcs/github_test.go index 0d2091659..988fe9020 100644 --- a/pkg/webvcs/github_test.go +++ b/pkg/webvcs/github_test.go @@ -131,7 +131,11 @@ func TestPayLoadFix(t *testing.T) { } logger, _ := getLogger() - _, err = gvcs.ParsePayload(ctx, logger, "pull_request", "pull_request", string(b)) + r := RunInfo{ + EventType: "pull_request", + TriggerTarget: "pull_request", + } + _, err = gvcs.ParsePayload(ctx, logger, r, string(b)) // would bomb out on "assertion failed: error is not nil: invalid character // '\n' in string literal" if we don't fix the payload assert.NilError(t, err) @@ -164,8 +168,11 @@ func TestParsePayloadRerequestFromPullRequest(t *testing.T) { Client: fakeclient, } logger, observer := getLogger() - // TODO - runinfo, err := gvcs.ParsePayload(ctx, logger, "check_run", "issue-recheck", checkrunEvent) + r := RunInfo{ + EventType: "check_run", + TriggerTarget: "issue-recheck", + } + runinfo, err := gvcs.ParsePayload(ctx, logger, r, checkrunEvent) assert.NilError(t, err) assert.Equal(t, prOwner, runinfo.Owner) @@ -217,8 +224,12 @@ func TestParsePayloadRerequestFromPush(t *testing.T) { _, _ = fmt.Fprint(w, `{"commit": {"message": "HELLO"}}`) }) + r := RunInfo{ + EventType: "check_run", + TriggerTarget: "issue-recheck", + } logger, _ := getLogger() - runinfo, err := gvcs.ParsePayload(ctx, logger, "check_run", "issue-recheck", checkrunEvent) + runinfo, err := gvcs.ParsePayload(ctx, logger, r, checkrunEvent) assert.NilError(t, err) assert.Equal(t, runinfo.EventType, "push") @@ -272,7 +283,11 @@ func TestParsePayLoadRetest(t *testing.T) { } // TODO - runinfo, err := gvcs.ParsePayload(ctx, logger, "issue_comment", "issue_comment", issueEvent) + r := RunInfo{ + EventType: "issue_comment", + TriggerTarget: "issue_comment", + } + runinfo, err := gvcs.ParsePayload(ctx, logger, r, issueEvent) assert.NilError(t, err) assert.Equal(t, prOwner, runinfo.Owner) // Make sure the PR owner is the runinfo.Owner and not the issueSender @@ -299,7 +314,11 @@ func TestParsePayload(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) logger, _ := getLogger() - runinfo, err := gvcs.ParsePayload(ctx, logger, "pull_request", "pull_request", string(b)) + r := RunInfo{ + EventType: "pull_request", + TriggerTarget: "pull_request", + } + runinfo, err := gvcs.ParsePayload(ctx, logger, r, string(b)) assert.NilError(t, err) assert.Assert(t, runinfo.BaseBranch == "master") assert.Assert(t, runinfo.Owner == "chmouel") @@ -312,7 +331,11 @@ func TestParsePayloadInvalid(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) gvcs := NewGithubVCS("none", "") logger, _ := getLogger() - _, err := gvcs.ParsePayload(ctx, logger, "pull_request", "pull_request", "hello moto") + r := RunInfo{ + EventType: "pull_request", + TriggerTarget: "pull_request", + } + _, err := gvcs.ParsePayload(ctx, logger, r, "hello moto") assert.ErrorContains(t, err, "invalid character") } @@ -320,7 +343,11 @@ func TestParsePayloadUnkownEvent(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) gvcs := NewGithubVCS("none", "") logger, _ := getLogger() - _, err := gvcs.ParsePayload(ctx, logger, "foo", "foo", "{\"hello\": \"moto\"}") + r := RunInfo{ + EventType: "foo", + TriggerTarget: "foo", + } + _, err := gvcs.ParsePayload(ctx, logger, r, "{\"hello\": \"moto\"}") assert.ErrorContains(t, err, "unknown X-Github-Event") } @@ -328,7 +355,11 @@ func TestParsePayCannotParse(t *testing.T) { gvcs := NewGithubVCS("none", "") ctx, _ := rtesting.SetupFakeContext(t) logger, _ := getLogger() - _, err := gvcs.ParsePayload(ctx, logger, "gollum", "gollum", "{}") + r := RunInfo{ + EventType: "gollum", + TriggerTarget: "gollum", + } + _, err := gvcs.ParsePayload(ctx, logger, r, "{}") assert.Error(t, err, "this event is not supported") } From 4031e66b9a47b28a5414b61723fb03a7a399ad72 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 16 Sep 2021 11:20:31 +0200 Subject: [PATCH 3/4] Drop the get and update right on the secret auto creation We are only give the rights to create and delete but not read every secrets out there. We need to find a way to disable that right if we want install by operator and user chose to disable that feature. --- config/200-role.yaml | 4 ++-- pkg/kubeinteraction/secrets.go | 24 +++++++++++++++++------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/config/200-role.yaml b/config/200-role.yaml index 9c92f95fe..99a548b9f 100644 --- a/config/200-role.yaml +++ b/config/200-role.yaml @@ -82,10 +82,10 @@ rules: - apiGroups: [""] resources: ["namespaces", "pods", "pods/log"] verbs: ["get", "list", "watch"] - # Allow creating secrets in namespace + # Allow creating and deleting secrets in namespace - apiGroups: [""] resources: ["secrets"] - verbs: ["get", "create", "update"] + verbs: ["create", "delete"] # Permissions to list repositories on cluster - apiGroups: ["pipelinesascode.tekton.dev"] resources: ["repositories"] diff --git a/pkg/kubeinteraction/secrets.go b/pkg/kubeinteraction/secrets.go index de7b4e18b..56ae49681 100644 --- a/pkg/kubeinteraction/secrets.go +++ b/pkg/kubeinteraction/secrets.go @@ -19,6 +19,16 @@ const ( ` ) +func (k Interaction) createSecret(ctx context.Context, secretData map[string]string, targetNamespace, secretName string) error { + secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Labels: map[string]string{"app.kubernetes.io/managed-by": "pipelines-as-code"}, + }} + secret.StringData = secretData + _, err := k.Clients.Kube.CoreV1().Secrets(targetNamespace).Create(ctx, secret, metav1.CreateOptions{}) + return err +} + // CreateBasicAuthSecret Create a secret for git-clone basic-auth workspace func (k Interaction) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.RunInfo, targetNamespace, token string) error { repoURL, err := url.Parse(runinfo.URL) @@ -32,15 +42,15 @@ func (k Interaction) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.R ".git-credentials": urlWithToken, } + // Try to create secrete if that fails then delete it first and then create + // This allows up not to give List and Get right clusterwide secretName := fmt.Sprintf(basicAuthSecretName, runinfo.Owner, runinfo.Repository) - secret, err := k.Clients.Kube.CoreV1().Secrets(targetNamespace).Get(ctx, secretName, metav1.GetOptions{}) + err = k.createSecret(ctx, secretData, targetNamespace, secretName) if err != nil { - secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: secretName}} - secret.StringData = secretData - _, err = k.Clients.Kube.CoreV1().Secrets(targetNamespace).Create(ctx, secret, metav1.CreateOptions{}) - } else { - secret.StringData = secretData - _, err = k.Clients.Kube.CoreV1().Secrets(targetNamespace).Update(ctx, secret, metav1.UpdateOptions{}) + err = k.Clients.Kube.CoreV1().Secrets(targetNamespace).Delete(ctx, secretName, metav1.DeleteOptions{}) + if err == nil { + err = k.createSecret(ctx, secretData, targetNamespace, secretName) + } } return err From 62935b5b4ea14aab42e34d33e5ff29d0aef791b7 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 16 Sep 2021 12:36:50 +0200 Subject: [PATCH 4/4] Add a log message when creating secret Fixes #222 --- pkg/cli/interface.go | 2 +- pkg/kubeinteraction/secrets.go | 7 +++---- pkg/kubeinteraction/secrets_test.go | 14 +++++++++++--- pkg/pipelineascode/pipelineascode.go | 2 +- pkg/test/kubernetestint/kubernetesint.go | 2 +- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/pkg/cli/interface.go b/pkg/cli/interface.go index 10e5323f9..e4bcf1e3b 100644 --- a/pkg/cli/interface.go +++ b/pkg/cli/interface.go @@ -49,5 +49,5 @@ type KubeInteractionIntf interface { // TODO: we don't need tektonv1beta1client stuff here WaitForPipelineRunSucceed(context.Context, tektonv1beta1client.TektonV1beta1Interface, *v1beta1.PipelineRun, time.Duration) error CleanupPipelines(context.Context, string, string, int) error - CreateBasicAuthSecret(context.Context, webvcs.RunInfo, string, string) error + CreateBasicAuthSecret(context.Context, webvcs.RunInfo, string) error } diff --git a/pkg/kubeinteraction/secrets.go b/pkg/kubeinteraction/secrets.go index 56ae49681..c1647cfd8 100644 --- a/pkg/kubeinteraction/secrets.go +++ b/pkg/kubeinteraction/secrets.go @@ -30,13 +30,12 @@ func (k Interaction) createSecret(ctx context.Context, secretData map[string]str } // CreateBasicAuthSecret Create a secret for git-clone basic-auth workspace -func (k Interaction) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.RunInfo, targetNamespace, token string) error { +func (k Interaction) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.RunInfo, targetNamespace string) error { repoURL, err := url.Parse(runinfo.URL) if err != nil { return err } - urlWithToken := fmt.Sprintf("%s://git:%s@%s%s", repoURL.Scheme, token, repoURL.Host, repoURL.Path) - + urlWithToken := fmt.Sprintf("%s://git:%s@%s%s", repoURL.Scheme, k.Clients.GithubClient.Token, repoURL.Host, repoURL.Path) secretData := map[string]string{ ".gitconfig": fmt.Sprintf(basicAuthGitConfigData, runinfo.URL), ".git-credentials": urlWithToken, @@ -52,6 +51,6 @@ func (k Interaction) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.R err = k.createSecret(ctx, secretData, targetNamespace, secretName) } } - + k.Clients.Log.Infof("Secret %s has been generated in namespace %s", secretName, targetNamespace) return err } diff --git a/pkg/kubeinteraction/secrets_test.go b/pkg/kubeinteraction/secrets_test.go index 39f0f064c..899278080 100644 --- a/pkg/kubeinteraction/secrets_test.go +++ b/pkg/kubeinteraction/secrets_test.go @@ -6,6 +6,8 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/cli" testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/webvcs" + "go.uber.org/zap" + zapobserver "go.uber.org/zap/zaptest/observer" "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -50,9 +52,14 @@ func TestCreateBasicAuthSecret(t *testing.T) { }, } stdata, _ := testclient.SeedTestData(t, ctx, tdata) - + observer, _ := zapobserver.New(zap.InfoLevel) + fakelogger := zap.New(observer).Sugar() kint := Interaction{ - Clients: &cli.Clients{Kube: stdata.Kube}, + Clients: &cli.Clients{ + Kube: stdata.Kube, + GithubClient: webvcs.GithubVCS{Token: token}, + Log: fakelogger, + }, } runinfo := webvcs.RunInfo{ Owner: "owner", @@ -75,10 +82,11 @@ func TestCreateBasicAuthSecret(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := kint.CreateBasicAuthSecret(ctx, runinfo, tt.targetNS, token) + err := kint.CreateBasicAuthSecret(ctx, runinfo, tt.targetNS) assert.NilError(t, err) slist, err := kint.Clients.Kube.CoreV1().Secrets(tt.targetNS).List(ctx, metav1.ListOptions{}) assert.NilError(t, err) + assert.Assert(t, len(slist.Items) > 0, "Secret has not been created") assert.Equal(t, slist.Items[0].Name, "pac-git-basic-auth-owner-repo") assert.Equal(t, slist.Items[0].StringData[".git-credentials"], "https://git:verysecrete@forge/owner/repo") }) diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index f985c8834..4299c4479 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -152,7 +152,7 @@ func Run(ctx context.Context, cs *cli.Clients, k8int cli.KubeInteractionIntf, ru // Automatically create a secret with the token to be reused by git-clone task if runinfo.SecretAutoCreation { - err = k8int.CreateBasicAuthSecret(ctx, *runinfo, repo.Spec.Namespace, cs.GithubClient.Token) + err = k8int.CreateBasicAuthSecret(ctx, *runinfo, repo.Spec.Namespace) if err != nil { return err } diff --git a/pkg/test/kubernetestint/kubernetesint.go b/pkg/test/kubernetestint/kubernetesint.go index 90398ba05..dc3cd9f0b 100644 --- a/pkg/test/kubernetestint/kubernetesint.go +++ b/pkg/test/kubernetestint/kubernetesint.go @@ -21,7 +21,7 @@ func (k *KinterfaceTest) GetConsoleUI(ctx context.Context, ns string, pr string) return k.ConsoleURL, nil } -func (k *KinterfaceTest) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.RunInfo, namespace, token string) error { +func (k *KinterfaceTest) CreateBasicAuthSecret(ctx context.Context, runinfo webvcs.RunInfo, ns string) error { return nil }