diff --git a/pkg/adapter/adapter.go b/pkg/adapter/adapter.go index 25dd188fb1..3708d63d88 100644 --- a/pkg/adapter/adapter.go +++ b/pkg/adapter/adapter.go @@ -271,10 +271,9 @@ func (l listener) handleEvent(ctx context.Context) http.HandlerFunc { go func() { defer span.End() - err := s.processEvent(tracedCtx, localRequest) + err := s.handleEvent(tracedCtx, localRequest) if err != nil { span.RecordError(err) - logger.Errorf("an error occurred: %v", err) } }() diff --git a/pkg/adapter/sinker.go b/pkg/adapter/sinker.go index b029d163c0..19cb4a3eae 100644 --- a/pkg/adapter/sinker.go +++ b/pkg/adapter/sinker.go @@ -3,10 +3,12 @@ package adapter import ( "bytes" "context" + "errors" "fmt" "net/http" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/events" "github.com/openshift-pipelines/pipelines-as-code/pkg/gitclient" "github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction" "github.com/openshift-pipelines/pipelines-as-code/pkg/matcher" @@ -15,10 +17,12 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" "github.com/openshift-pipelines/pipelines-as-code/pkg/tracing" semconv "go.opentelemetry.io/otel/semconv/v1.41.0" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" + "go.uber.org/zap/zapcore" ) type sinker struct { @@ -70,6 +74,14 @@ func (s *sinker) processEventPayload(ctx context.Context, request *http.Request) return nil } +func (s *sinker) handleEvent(ctx context.Context, request *http.Request) error { + err := s.processEvent(ctx, request) + if err != nil { + s.logger.Errorf("error handling event: %v", err) + } + return err +} + func (s *sinker) processEvent(ctx context.Context, request *http.Request) error { if s.event.EventType != "incoming" { if err := s.processEventPayload(ctx, request); err != nil { @@ -89,6 +101,14 @@ func (s *sinker) processEvent(ctx context.Context, request *http.Request) error // We found the repository, now setup client with token scoping // If setup fails here, it's a configuration error and we should fail fast if err := s.setupClient(ctx, repo); err != nil { + if errors.Is(err, secrets.ErrSecretNotFound) { + events.NewEventEmitter(s.run.Clients.Kube, s.logger).EmitMessage( + repo, + zapcore.ErrorLevel, + "RepositorySecretMissing", + fmt.Sprintf("cannot process event, cannot setup vcs client: %v", err), + ) + } return fmt.Errorf("client setup failed: %w", err) } s.logger.Debugf("Client setup completed for event type: %s", s.event.EventType) diff --git a/pkg/secrets/secret.go b/pkg/secrets/secret.go index 25178ae0f3..bf8041c11c 100644 --- a/pkg/secrets/secret.go +++ b/pkg/secrets/secret.go @@ -2,6 +2,7 @@ package secrets import ( "context" + "errors" "fmt" "strings" @@ -19,6 +20,11 @@ const ( defaultPipelinesAscodeSecretWebhookSecretKey = "webhook.secret" ) +// ErrSecretNotFound indicates that a Repository's secret is misconfigured. +// For example if a required secret is not specified or the required secret +// does not exist. +var ErrSecretNotFound = errors.New("secret not found") + type SecretFromRepository struct { K8int kubeinteraction.Interface Config *info.ProviderConfig @@ -57,7 +63,7 @@ func (s *SecretFromRepository) Get(ctx context.Context) error { Name: s.Repo.Spec.GitProvider.Secret.Name, Key: gitProviderSecretKey, }); err != nil { - return err + return fmt.Errorf("%w: error getting provider secret: %w", ErrSecretNotFound, err) } s.Event.Provider.GitProviderSecretNamespace = s.Namespace s.Event.Provider.GitProviderSecretFromGlobalRepo = s.InheritedGlobalSecret @@ -91,7 +97,7 @@ func (s *SecretFromRepository) Get(ctx context.Context) error { Name: s.Repo.Spec.GitProvider.WebhookSecret.Name, Key: gitProviderWebhookSecretKey, }); err != nil { - return err + return fmt.Errorf("%w: error getting webhook secret: %w", ErrSecretNotFound, err) } if s.Event.Provider.WebhookSecret != "" { s.Event.Provider.WebhookSecretFromRepo = true diff --git a/pkg/secrets/secret_test.go b/pkg/secrets/secret_test.go index 705d66a5af..cdfea6c175 100644 --- a/pkg/secrets/secret_test.go +++ b/pkg/secrets/secret_test.go @@ -8,6 +8,7 @@ import ( apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" kitesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint" + "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" "gotest.tools/v3/assert" @@ -16,34 +17,35 @@ import ( func TestSecretFromRepository(t *testing.T) { tests := []struct { - name string - repo *apipac.Repository - providerconfig *info.ProviderConfig - logmatch []*regexp.Regexp - expectedSecret string - expectedWebhookSecret string - providerType string + name string + repo *apipac.Repository + providerconfig *info.ProviderConfig + providerType string + secrets map[string]string + logmatch []*regexp.Regexp + expectedSecret string + wantErr string + wantErrIs error }{ { name: "config default", providerconfig: &info.ProviderConfig{ APIURL: "https://apiurl.default", }, - expectedSecret: "configdefault", - expectedWebhookSecret: "webhooksecret", repo: &apipac.Repository{ Spec: apipac.RepositorySpec{ GitProvider: &apipac.GitProvider{ - Secret: &apipac.Secret{ - Name: "repo-secret", - }, - WebhookSecret: &apipac.Secret{ - Name: "repo-webhook-secret", - }, + Secret: &apipac.Secret{Name: "repo-secret"}, + WebhookSecret: &apipac.Secret{Name: "repo-webhook-secret"}, }, }, }, providerType: "lalala", + secrets: map[string]string{ + "repo-secret": "configdefault", + "repo-webhook-secret": "webhooksecret", + }, + expectedSecret: "configdefault", logmatch: []*regexp.Regexp{ regexp.MustCompile(fmt.Sprintf( "^Using git provider lalala: apiurl=https://apiurl.default user= token-secret=repo-secret token-key=%s", @@ -59,11 +61,16 @@ func TestSecretFromRepository(t *testing.T) { repo: &apipac.Repository{ Spec: apipac.RepositorySpec{ GitProvider: &apipac.GitProvider{ - URL: "https://dowant", - Secret: &apipac.Secret{}, + URL: "https://dowant", + Secret: &apipac.Secret{Name: "provider-secret"}, + WebhookSecret: &apipac.Secret{Name: "webhook-secret"}, }, }, }, + secrets: map[string]string{ + "provider-secret": "setapiurl", + "webhook-secret": "", + }, expectedSecret: "setapiurl", logmatch: []*regexp.Regexp{ regexp.MustCompile(".*apiurl=https://dowant.*"), @@ -75,36 +82,118 @@ func TestSecretFromRepository(t *testing.T) { repo: &apipac.Repository{ Spec: apipac.RepositorySpec{ GitProvider: &apipac.GitProvider{ - User: "userfoo", - Secret: &apipac.Secret{}, + User: "userfoo", + Secret: &apipac.Secret{Name: "provider-secret"}, + WebhookSecret: &apipac.Secret{Name: "webhook-secret"}, }, }, }, + secrets: map[string]string{ + "provider-secret": "set user", + "webhook-secret": "", + }, expectedSecret: "set user", logmatch: []*regexp.Regexp{ regexp.MustCompile(".*user=userfoo*"), }, }, + { + name: "no git provider", + providerconfig: &info.ProviderConfig{APIURL: "https://fake"}, + providerType: "subversion", + repo: &apipac.Repository{ + Spec: apipac.RepositorySpec{GitProvider: nil}, + }, + wantErr: "failed to find git_provider details", + }, + { + name: "no git provider secret", + providerconfig: &info.ProviderConfig{APIURL: "https://fake"}, + providerType: "subversion", + repo: &apipac.Repository{ + Spec: apipac.RepositorySpec{GitProvider: &apipac.GitProvider{}}, + }, + wantErr: "failed to find secret in git_provider section in repository", + }, + { + name: "git provider secret doesn't exist", + providerconfig: &info.ProviderConfig{APIURL: "https://fake"}, + providerType: "subversion", + repo: &apipac.Repository{ + Spec: apipac.RepositorySpec{ + GitProvider: &apipac.GitProvider{ + Secret: &apipac.Secret{Name: "bad-name"}, + }, + }, + }, + wantErr: "error getting provider secret", + wantErrIs: ErrSecretNotFound, + }, + { + // a bad key on an existing secret is not an error, it just resolves to an empty value + name: "git provider secret bad key", + providerconfig: &info.ProviderConfig{APIURL: "https://fake"}, + providerType: "subversion", + repo: &apipac.Repository{ + Spec: apipac.RepositorySpec{ + GitProvider: &apipac.GitProvider{ + Secret: &apipac.Secret{Name: "good-name", Key: "bad-key"}, + }, + }, + }, + secrets: map[string]string{ + "good-name": "keep it secret, keep it safe", + }, + expectedSecret: "keep it secret, keep it safe", + }, + { + // webhook secret being unspecified is OK, but if it is specified it must exist + name: "webhook secret missing", + providerconfig: &info.ProviderConfig{APIURL: "https://fake"}, + providerType: "subversion", + repo: &apipac.Repository{ + Spec: apipac.RepositorySpec{ + GitProvider: &apipac.GitProvider{ + Secret: &apipac.Secret{Name: "good-name", Key: "good-key"}, + WebhookSecret: &apipac.Secret{Name: "bad-name"}, + }, + }, + }, + secrets: map[string]string{ + "good-name": "keep it secret, keep it safe", + }, + wantErr: "error getting webhook secret", + wantErrIs: ErrSecretNotFound, + }, + { + name: "webhook secret bad key", + providerconfig: &info.ProviderConfig{APIURL: "https://fake"}, + providerType: "subversion", + repo: &apipac.Repository{ + Spec: apipac.RepositorySpec{ + GitProvider: &apipac.GitProvider{ + Secret: &apipac.Secret{Name: "good-name", Key: "good-key"}, + WebhookSecret: &apipac.Secret{Name: "good-name", Key: "bad-key"}, + }, + }, + }, + secrets: map[string]string{ + "good-name": "keep it secret, keep it safe", + }, + expectedSecret: "keep it secret, keep it safe", + logmatch: []*regexp.Regexp{ + regexp.MustCompile("^Using git provider subversion:.*"), + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) observer, log := zapobserver.New(zap.InfoLevel) logger := zap.New(observer).Sugar() - retsecret := map[string]string{} - if tt.repo.Spec.GitProvider.Secret != nil { - retsecret[tt.repo.Spec.GitProvider.Secret.Name] = tt.expectedSecret - } else { - tt.repo.Spec.GitProvider.Secret = &apipac.Secret{} - } - if tt.repo.Spec.GitProvider.WebhookSecret != nil { - retsecret[tt.repo.Spec.GitProvider.WebhookSecret.Name] = tt.expectedWebhookSecret - } else { - tt.repo.Spec.GitProvider.WebhookSecret = &apipac.Secret{} - } k8int := &kitesthelper.KinterfaceTest{ - GetSecretResult: retsecret, + GetSecretResult: tt.secrets, } event := info.NewEvent() sfr := SecretFromRepository{ @@ -118,6 +207,14 @@ func TestSecretFromRepository(t *testing.T) { } err := sfr.Get(ctx) + if tt.wantErr != "" { + assert.Assert(t, err != nil, "expected error: "+tt.wantErr) + assert.ErrorContains(t, err, tt.wantErr) + if tt.wantErrIs != nil { + assert.ErrorIs(t, err, tt.wantErrIs) + } + return + } assert.NilError(t, err) logs := log.TakeAll() assert.Equal(t, len(tt.logmatch), len(logs), "we didn't get the number of logging message: %+v", logs)