fix(logging): log sinker errors , emit event on repository config error

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)
 			}
 		}()
 

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)

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

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)