refactor(core): redact credentials, fail Start after Stop, tidy docs and errors

Names the core logger data-server, validates the datastore name in one
place, drops the error alias and documents swallowed errors.

Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>

Author: steiler

pkg/core/core.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"sync"
 
+	"github.com/sdcio/data-server/pkg/api"
 	"github.com/sdcio/data-server/pkg/cache"
 	"github.com/sdcio/data-server/pkg/config"
 	"github.com/sdcio/data-server/pkg/datastore"
@@ -23,8 +24,8 @@ type Core struct {
 	cfg      *config.Config
 	dialOpts []grpc.DialOption
 
-	ctx context.Context
-	cfn context.CancelFunc
+	ctx    context.Context
+	cancel context.CancelFunc
 
 	startOnce sync.Once
 	stopOnce  sync.Once
@@ -56,7 +57,7 @@ func New(ctx context.Context, cfg *config.Config, opts ...Option) (*Core, error)
 	c := &Core{
 		cfg:        cfg,
 		ctx:        ctx,
-		cfn:        cancel,
+		cancel:     cancel,
 		ready:      make(chan struct{}),
 		datastores: map[string]*datastore.Datastore{},
 	}
@@ -89,21 +90,27 @@ func (c *Core) WaitReady(ctx context.Context) error {
 	}
 }
 
-// Stop stops all datastores and cancels the Core's context. It is idempotent.
+// Stop stops all datastores and cancels the Core's context. It is idempotent
+// and always returns nil: datastore stop errors are discarded. A Start after
+// Stop fails with context.Canceled.
 func (c *Core) Stop(ctx context.Context) error {
 	c.stopOnce.Do(func() {
 		c.mu.Lock()
 		defer c.mu.Unlock()
 		for _, ds := range c.datastores {
 			_ = ds.Stop(ctx)
 		}
-		c.cfn()
+		c.cancel()
 	})
 	return nil
 }
 
 func (c *Core) start(ctx context.Context) error {
-	log := logf.FromContext(ctx)
+	if err := c.ctx.Err(); err != nil {
+		return err
+	}
+	log := logf.FromContext(ctx).WithName("data-server")
+	ctx = logf.IntoContext(ctx, log)
 
 	var wg sync.WaitGroup
 	var schemaErr, cacheErr error
@@ -158,12 +165,17 @@ func (c *Core) addDatastore(ds *datastore.Datastore) error {
 	return nil
 }
 
+// getDatastore fails with api.ErrInvalidArgument for an empty name and
+// api.ErrUnknownDatastore when the name is not registered.
 func (c *Core) getDatastore(name string) (*datastore.Datastore, error) {
+	if name == "" {
+		return nil, fmt.Errorf("%w: missing datastore name", api.ErrInvalidArgument)
+	}
 	c.mu.RLock()
 	defer c.mu.RUnlock()
 	ds, ok := c.datastores[name]
 	if !ok {
-		return nil, fmt.Errorf("%w %s", errUnknownDatastore, name)
+		return nil, fmt.Errorf("%w %s", api.ErrUnknownDatastore, name)
 	}
 	return ds, nil
 }

pkg/core/core_test.go

@@ -130,3 +130,14 @@ func TestWaitReadyHonoursContext(t *testing.T) {
 		t.Fatalf("got %v", err)
 	}
 }
+
+func TestStartAfterStopFails(t *testing.T) {
+	c, err := New(context.Background(), newTestConfig(t))
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = c.Stop(context.Background())
+	if err := c.Start(context.Background()); !errors.Is(err, context.Canceled) {
+		t.Fatalf("got %v", err)
+	}
+}

pkg/core/datastore.go

@@ -2,25 +2,20 @@ package core
 
 import (
 	"context"
-	"fmt"
 	"strings"
-	"time"
 
 	"github.com/sdcio/data-server/pkg/api"
 	"github.com/sdcio/data-server/pkg/config"
 	"github.com/sdcio/data-server/pkg/datastore"
+	"github.com/sdcio/data-server/pkg/utils"
 	sdcpb "github.com/sdcio/sdc-protos/sdcpb"
 )
 
-var errUnknownDatastore = api.ErrUnknownDatastore
-
-// GetDataStore returns the observed state of the named datastore. It fails
-// with api.ErrInvalidArgument for an empty name and api.ErrUnknownDatastore
-// when no such datastore exists.
+// GetDataStore returns the observed state of the named datastore, with
+// credentials in the effective spec redacted. It fails with
+// api.ErrInvalidArgument for an empty name and api.ErrUnknownDatastore when no
+// such datastore exists.
 func (c *Core) GetDataStore(ctx context.Context, name string) (*api.DatastoreInfo, error) {
-	if name == "" {
-		return nil, fmt.Errorf("%w: missing datastore name", api.ErrInvalidArgument)
-	}
 	ds, err := c.getDatastore(name)
 	if err != nil {
 		return nil, err
@@ -29,12 +24,9 @@ func (c *Core) GetDataStore(ctx context.Context, name string) (*api.DatastoreInf
 }
 
 // WatchDeviations returns a channel of deviation events for the named
-// datastore. The channel closes when ctx is done or the Core stops. Events are
-// not produced yet; the datastore deviation feed is wired in a later step.
+// datastore. The channel closes when ctx is done or the Core stops. It never
+// emits events: no deviation producer feeds it.
 func (c *Core) WatchDeviations(ctx context.Context, name string) (<-chan api.DeviationEvent, error) {
-	if name == "" {
-		return nil, fmt.Errorf("%w: missing datastore name", api.ErrInvalidArgument)
-	}
 	if _, err := c.getDatastore(name); err != nil {
 		return nil, err
 	}
@@ -98,7 +90,7 @@ func specFromConfig(dc *config.DatastoreConfig) api.DatastoreSpec {
 				Target:   &api.Target{Type: api.TargetType(strings.ToLower(sp.Protocol)), ProtocolOptions: api.ProtocolOptions{GNMI: gnmiSyncOptions(sp)}},
 				Paths:    sp.Paths,
 				Mode:     syncMode(sp.Mode),
-				Interval: uint64(sp.Interval / time.Nanosecond),
+				Interval: uint64(sp.Interval),
 			})
 		}
 	}
@@ -132,7 +124,7 @@ func targetFromSBI(s *config.SBI) api.Target {
 		Port:    s.Port,
 	}
 	if s.Credentials != nil {
-		t.Credentials = &api.Credentials{Username: s.Credentials.Username, Password: s.Credentials.Password, Token: s.Credentials.Token}
+		t.Credentials = &api.Credentials{Username: s.Credentials.Username, Password: redact(s.Credentials.Password), Token: redact(s.Credentials.Token)}
 	}
 	if s.TLS != nil {
 		t.TLS = &api.TLS{CA: s.TLS.CA, Cert: s.TLS.Cert, Key: s.TLS.Key, SkipVerify: s.TLS.SkipVerify}
@@ -141,16 +133,23 @@ func targetFromSBI(s *config.SBI) api.Target {
 		t.ProtocolOptions.GNMI = &api.GNMIOptions{Encoding: s.GnmiOptions.Encoding, TargetName: s.GnmiOptions.TargetName}
 	}
 	if o := s.NetconfOptions; o != nil {
-		cc := api.CommitCandidateCandidate
+		commit := api.CommitCandidateCandidate
 		if o.CommitDatastore == "running" {
-			cc = api.CommitCandidateRunning
+			commit = api.CommitCandidateRunning
 		}
 		t.ProtocolOptions.Netconf = &api.NetconfOptions{
 			IncludeNs:          o.IncludeNS,
 			OperationWithNs:    o.OperationWithNamespace,
 			UseOperationRemove: o.UseOperationRemove,
-			CommitCandidate:    cc,
+			CommitCandidate:    commit,
 		}
 	}
 	return t
 }
+
+func redact(v string) string {
+	if v == "" {
+		return ""
+	}
+	return utils.Redacted
+}

pkg/core/init.go

@@ -45,6 +45,8 @@ func (c *Core) createSchemaClient(ctx context.Context) error {
 	return c.createRemoteSchemaClient(ctx)
 }
 
+// createLocalSchemaStore fails only when the store cannot be opened; a schema
+// that fails to parse or add is logged and skipped.
 func (c *Core) createLocalSchemaStore(ctx context.Context) error {
 	log := logf.FromContext(ctx)
 	var store schemaStore.Store
@@ -67,9 +69,9 @@ func (c *Core) createLocalSchemaStore(ctx context.Context) error {
 		wg.Add(1)
 		go func(sCfg *schemaConfig.SchemaConfig) {
 			defer wg.Done()
-			sck := schemaStore.SchemaKey{Name: sCfg.Name, Vendor: sCfg.Vendor, Version: sCfg.Version}
-			log := log.WithValues("schema", sck)
-			if store.HasSchema(sck) {
+			key := schemaStore.SchemaKey{Name: sCfg.Name, Vendor: sCfg.Vendor, Version: sCfg.Version}
+			log := log.WithValues("schema", key)
+			if store.HasSchema(key) {
 				log.Info("schema already exists in the store, not reloading it")
 				return
 			}