diff --git a/config/config-tracing.yaml b/config/config-tracing.yaml index f65862304b4..e4d6ebc91ee 100644 --- a/config/config-tracing.yaml +++ b/config/config-tracing.yaml @@ -42,3 +42,5 @@ data: # API endpoint to send the traces to # (optional): The default value is given below endpoint: "http://jaeger-collector.jaeger.svc.cluster.local:14268/api/traces" + # (optional) Name of the k8s secret which contains basic auth credentils + credentialsSecret: "jaeger-creds" diff --git a/config/controller.yaml b/config/controller.yaml index d4a18b134f3..4dbfcd2c1a1 100644 --- a/config/controller.yaml +++ b/config/controller.yaml @@ -110,13 +110,6 @@ spec: value: /etc/ssl/certs - name: METRICS_DOMAIN value: tekton.dev/pipeline - # The following variables can be uncommented with correct values to enable Jaeger tracing - #- name: OTEL_EXPORTER_JAEGER_ENDPOINT - # value: http://jaeger-collector.jaeger:14268/api/traces - #- name: OTEL_EXPORTER_JAEGER_USER - # value: username - #- name: OTEL_EXPORTER_JAEGER_PASSWORD - # value: password securityContext: allowPrivilegeEscalation: false capabilities: diff --git a/docs/developers/tracing.md b/docs/developers/tracing.md index 5a4b90a576f..45d61520f4d 100644 --- a/docs/developers/tracing.md +++ b/docs/developers/tracing.md @@ -30,8 +30,4 @@ The configmap `config/config-tracing.yaml` contains the configuration for tracin * enabled: Set this to true to enable tracing * endpoint: API endpoint for jaeger collector to send the traces. By default the endpoint is configured to be `http://jaeger-collector.jaeger.svc.cluster.local:14268/api/traces`. - -Tekton pipelines controller also supports the following additional environment variables to be able to connect to jaeger: - -* `OTEL_EXPORTER_JAEGER_USER` is the username to be sent as authentication to the collector endpoint. -* `OTEL_EXPORTER_JAEGER_PASSWORD` is the password to be sent as authentication to the collector endpoint. +* credentialsSecret: Name of the secret which contains `username` and `password` to authenticate against the endpoint diff --git a/pkg/apis/config/tracing.go b/pkg/apis/config/tracing.go index d8685b758b2..715a5b0d8e7 100644 --- a/pkg/apis/config/tracing.go +++ b/pkg/apis/config/tracing.go @@ -30,6 +30,9 @@ const ( // tracingEndpintKey is the configmap key for tracing api endpoint tracingEndpointKey = "endpoint" + // tracingCredentialsSecretKey is the name of the secret which contains credentials for tracing endpoint + tracingCredentialsSecretKey = "credentialsSecret" + // DefaultEndpoint is the default destination for sending traces DefaultEndpoint = "http://jaeger-collector.jaeger.svc.cluster.local:14268/api/traces" ) @@ -40,8 +43,9 @@ var DefaultTracing, _ = newTracingFromMap(map[string]string{}) // Tracing holds the configurations for tracing // +k8s:deepcopy-gen=true type Tracing struct { - Enabled bool - Endpoint string + Enabled bool + Endpoint string + CredentialsSecret string } // Equals returns true if two Configs are identical @@ -55,7 +59,8 @@ func (cfg *Tracing) Equals(other *Tracing) bool { } return other.Enabled == cfg.Enabled && - other.Endpoint == cfg.Endpoint + other.Endpoint == cfg.Endpoint && + other.CredentialsSecret == cfg.CredentialsSecret } // GetTracingConfigName returns the name of the configmap containing all @@ -78,6 +83,10 @@ func newTracingFromMap(config map[string]string) (*Tracing, error) { t.Endpoint = endpoint } + if secret, ok := config[tracingCredentialsSecretKey]; ok { + t.CredentialsSecret = secret + } + if enabled, ok := config[tracingEnabledKey]; ok { e, err := strconv.ParseBool(enabled) if err != nil { diff --git a/pkg/reconciler/pipelinerun/controller.go b/pkg/reconciler/pipelinerun/controller.go index 38d4ed19e2d..f7b4d61b51a 100644 --- a/pkg/reconciler/pipelinerun/controller.go +++ b/pkg/reconciler/pipelinerun/controller.go @@ -35,11 +35,13 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" resolution "github.com/tektoncd/pipeline/pkg/resolution/resource" "github.com/tektoncd/pipeline/pkg/tracing" + corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/utils/clock" kubeclient "knative.dev/pkg/client/injection/kube/client" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" + secretinformer "knative.dev/pkg/injection/clients/namespacedkube/informers/core/v1/secret" "knative.dev/pkg/logging" ) @@ -59,9 +61,10 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock) func(contex pipelineRunInformer := pipelineruninformer.Get(ctx) resolutionInformer := resolutioninformer.Get(ctx) verificationpolicyInformer := verificationpolicyinformer.Get(ctx) - tracerProvider := tracing.New(TracerProviderName) + secretinformer := secretinformer.Get(ctx) + tracerProvider := tracing.New(TracerProviderName, logger.Named("tracing")) //nolint:contextcheck // OnStore methods does not support context as a parameter - configStore := config.NewStore(logger.Named("config-store"), pipelinerunmetrics.MetricsOnStore(logger), tracerProvider.OnStore(logger)) + configStore := config.NewStore(logger.Named("config-store"), pipelinerunmetrics.MetricsOnStore(logger), tracerProvider.OnStore(secretinformer.Lister())) configStore.WatchConfigs(cmw) c := &Reconciler{ @@ -86,6 +89,17 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock) func(contex } }) + if _, err := secretinformer.Informer().AddEventHandler(controller.HandleAll(func(obj interface{}) { + secret, ok := obj.(*corev1.Secret) + if !ok { + logger.Error("Failed to do type assertion for Secret") + return + } + tracerProvider.OnSecret(secret) + })); err != nil { + logging.FromContext(ctx).Panicf("Couldn't register Secret informer event handler: %w", err) + } + if _, err := pipelineRunInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)); err != nil { logging.FromContext(ctx).Panicf("Couldn't register PipelineRun informer event handler: %w", err) } diff --git a/pkg/reconciler/taskrun/controller.go b/pkg/reconciler/taskrun/controller.go index 81609775320..b6c6f843c13 100644 --- a/pkg/reconciler/taskrun/controller.go +++ b/pkg/reconciler/taskrun/controller.go @@ -35,6 +35,7 @@ import ( "github.com/tektoncd/pipeline/pkg/spire" "github.com/tektoncd/pipeline/pkg/taskrunmetrics" "github.com/tektoncd/pipeline/pkg/tracing" + corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/utils/clock" kubeclient "knative.dev/pkg/client/injection/kube/client" @@ -42,6 +43,7 @@ import ( filteredpodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" + secretinformer "knative.dev/pkg/injection/clients/namespacedkube/informers/core/v1/secret" "knative.dev/pkg/logging" ) @@ -61,10 +63,11 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock) func(contex limitrangeInformer := limitrangeinformer.Get(ctx) verificationpolicyInformer := verificationpolicyinformer.Get(ctx) resolutionInformer := resolutioninformer.Get(ctx) + secretinformer := secretinformer.Get(ctx) spireClient := spire.GetControllerAPIClient(ctx) - tracerProvider := tracing.New(TracerProviderName) + tracerProvider := tracing.New(TracerProviderName, logger.Named("tracing")) //nolint:contextcheck // OnStore methods does not support context as a parameter - configStore := config.NewStore(logger.Named("config-store"), taskrunmetrics.MetricsOnStore(logger), spire.OnStore(ctx, logger), tracerProvider.OnStore(logger)) + configStore := config.NewStore(logger.Named("config-store"), taskrunmetrics.MetricsOnStore(logger), spire.OnStore(ctx, logger), tracerProvider.OnStore(secretinformer.Lister())) configStore.WatchConfigs(cmw) entrypointCache, err := pod.NewEntrypointCache(kubeclientset) @@ -96,6 +99,17 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock) func(contex } }) + if _, err := secretinformer.Informer().AddEventHandler(controller.HandleAll(func(obj interface{}) { + secret, ok := obj.(*corev1.Secret) + if !ok { + logger.Error("Failed to do type assertion for Secret") + return + } + tracerProvider.OnSecret(secret) + })); err != nil { + logging.FromContext(ctx).Panicf("Couldn't register Secret informer event handler: %w", err) + } + if _, err := taskRunInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)); err != nil { logging.FromContext(ctx).Panicf("Couldn't register TaskRun informer event handler: %w", err) } diff --git a/pkg/tracing/tracing.go b/pkg/tracing/tracing.go index 9985ca11b91..aaed99d4b47 100644 --- a/pkg/tracing/tracing.go +++ b/pkg/tracing/tracing.go @@ -28,12 +28,18 @@ import ( semconv "go.opentelemetry.io/otel/semconv/v1.12.0" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + listerv1 "k8s.io/client-go/listers/core/v1" + "knative.dev/pkg/system" ) type tracerProvider struct { service string provider trace.TracerProvider cfg *config.Tracing + username string + password string + logger *zap.SugaredLogger } func init() { @@ -41,42 +47,48 @@ func init() { } // New returns a new instance of tracerProvider for the given service -func New(service string) *tracerProvider { +func New(service string, logger *zap.SugaredLogger) *tracerProvider { return &tracerProvider{ service: service, provider: trace.NewNoopTracerProvider(), + logger: logger, } } // OnStore configures tracerProvider dynamically -func (t *tracerProvider) OnStore(logger *zap.SugaredLogger) func(name string, value interface{}) { +func (t *tracerProvider) OnStore(lister listerv1.SecretLister) func(name string, value interface{}) { return func(name string, value interface{}) { - if name == config.GetTracingConfigName() { - cfg, ok := value.(*config.Tracing) - if !ok { - logger.Error("Failed to do type assertion for extracting TRACING config") - return - } + if name != config.GetTracingConfigName() { + return + } - if cfg.Equals(t.cfg) { - logger.Info("Tracing config unchanged", cfg, t.cfg) - return - } - t.cfg = cfg + cfg, ok := value.(*config.Tracing) + if !ok { + t.logger.Error("tracing configmap is in invalid format. value: %v", value) + return + } - tp, err := createTracerProvider(t.service, cfg) + if cfg.Equals(t.cfg) { + t.logger.Info("tracing config unchanged", cfg, t.cfg) + return + } + t.cfg = cfg + + if lister != nil && cfg.CredentialsSecret != "" { + sec, err := lister.Secrets(system.Namespace()).Get(cfg.CredentialsSecret) if err != nil { - logger.Errorf("Unable to initialize tracing with error : %v", err.Error()) + t.logger.Errorf("unable to initialize tracing with error : %v", err.Error()) return } - logger.Info("Initialized Tracer Provider") - if p, ok := t.provider.(*tracesdk.TracerProvider); ok { - if err := p.Shutdown(context.Background()); err != nil { - logger.Errorf("Unable to shutdown tracingprovider with error : %v", err.Error()) - } - } - t.provider = tp + creds := sec.Data + t.username = string(creds["username"]) + t.password = string(creds["password"]) + } else { + t.username = "" + t.password = "" } + + t.reinitialize() } } @@ -84,13 +96,51 @@ func (t *tracerProvider) Tracer(name string, options ...trace.TracerOption) trac return t.provider.Tracer(name, options...) } -func createTracerProvider(service string, cfg *config.Tracing) (trace.TracerProvider, error) { +func (t *tracerProvider) OnSecret(secret *corev1.Secret) { + if secret.Name != t.cfg.CredentialsSecret { + return + } + + creds := secret.Data + username := string(creds["username"]) + password := string(creds["password"]) + + if t.username == username && t.password == password { + // No change in credentials, no need to reinitialize + return + } + t.username = username + t.password = password + + t.logger.Debugf("tracing credentials updated, reinitializing tracingprovider with secret: %v", secret.Name) + + t.reinitialize() +} + +func (t *tracerProvider) reinitialize() { + tp, err := createTracerProvider(t.service, t.cfg, t.username, t.password) + if err != nil { + t.logger.Errorf("unable to initialize tracing with error : %v", err.Error()) + return + } + t.logger.Info("initialized Tracer Provider") + if p, ok := t.provider.(*tracesdk.TracerProvider); ok { + if err := p.Shutdown(context.Background()); err != nil { + t.logger.Errorf("unable to shutdown tracingprovider with error : %v", err.Error()) + } + } + t.provider = tp +} + +func createTracerProvider(service string, cfg *config.Tracing, user, pass string) (trace.TracerProvider, error) { if !cfg.Enabled { return trace.NewNoopTracerProvider(), nil } exp, err := jaeger.New(jaeger.WithCollectorEndpoint( jaeger.WithEndpoint(cfg.Endpoint), + jaeger.WithUsername(user), + jaeger.WithPassword(pass), )) if err != nil { return nil, err diff --git a/pkg/tracing/tracing_test.go b/pkg/tracing/tracing_test.go index 1f2cdfde0fe..c9b75618dd2 100644 --- a/pkg/tracing/tracing_test.go +++ b/pkg/tracing/tracing_test.go @@ -14,19 +14,20 @@ See the License for the specific language governing permissions and limitations under the License. */ -package tracing_test +package tracing import ( "context" "testing" "github.com/tektoncd/pipeline/pkg/apis/config" - "github.com/tektoncd/pipeline/pkg/tracing" "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestNewTracerProvider(t *testing.T) { - tp := tracing.New("test-serice") + tp := New("test-service", zap.NewNop().Sugar()) tracer := tp.Tracer("tracer") _, span := tracer.Start(context.TODO(), "example") @@ -39,13 +40,13 @@ func TestNewTracerProvider(t *testing.T) { } func TestOnStore(t *testing.T) { - tp := tracing.New("test-service") + tp := New("test-service", zap.NewNop().Sugar()) cfg := &config.Tracing{ Enabled: false, } - tp.OnStore(zap.NewExample().Sugar())("config-tracing", cfg) + tp.OnStore(nil)("config-tracing", cfg) tracer := tp.Tracer("tracer") _, span := tracer.Start(context.TODO(), "example") @@ -58,14 +59,14 @@ func TestOnStore(t *testing.T) { } func TestOnStoreWithEnabled(t *testing.T) { - tp := tracing.New("test-service") + tp := New("test-service", zap.NewNop().Sugar()) cfg := &config.Tracing{ Enabled: true, Endpoint: "test-endpoint", } - tp.OnStore(zap.NewExample().Sugar())("config-tracing", cfg) + tp.OnStore(nil)("config-tracing", cfg) tracer := tp.Tracer("tracer") _, span := tracer.Start(context.TODO(), "example") @@ -74,3 +75,89 @@ func TestOnStoreWithEnabled(t *testing.T) { t.Fatalf("Span is not recording with tracing enabled") } } + +func TestOnSecretWithSecretName(t *testing.T) { + tp := New("test-service", zap.NewNop().Sugar()) + + cfg := &config.Tracing{ + Enabled: true, + CredentialsSecret: "jaeger", + } + + tp.OnStore(nil)("config-tracing", cfg) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jaeger", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + } + + tp.OnSecret(secret) + + if tp.username != "user" || tp.password != "pass" { + t.Errorf("Tracing provider is not updated with correct credentials") + } +} + +// If OnSecret was called without changing the credentials, do not initialize again +func TestOnSecretWithSameCreds(t *testing.T) { + tp := New("test-service", zap.NewNop().Sugar()) + + cfg := &config.Tracing{ + Enabled: true, + CredentialsSecret: "jaeger", + } + + tp.OnStore(nil)("config-tracing", cfg) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jaeger", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + } + + tp.OnSecret(secret) + + p := tp.provider + + tp.OnSecret(secret) + + if p != tp.provider { + t.Errorf("Tracerprovider was reinitialized when the credentials were not changed") + } +} + +func TestOnSecretWithWrongName(t *testing.T) { + tp := New("test-service", zap.NewNop().Sugar()) + + cfg := &config.Tracing{ + Enabled: true, + CredentialsSecret: "jaeger", + } + + tp.OnStore(nil)("config-tracing", cfg) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "somethingelse", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + } + + tp.OnSecret(secret) + + if tp.username == "user" || tp.password == "pass" { + t.Errorf("Tracing provider is updated with incorrect credentials") + } +}