diff --git a/Makefile b/Makefile index 2b196f30..543561c9 100644 --- a/Makefile +++ b/Makefile @@ -65,7 +65,7 @@ mocks-gen: mocks-rm ## Generate mocks for all the defined interfaces. mockgen -package=mocktarget -source=pkg/datastore/target/target.go -destination=$(MOCKDIR)/mocktarget/target.go mockgen -package=mockTreeEntry -source=pkg/tree/api/entry.go -destination=$(MOCKDIR)/mocktreeentry/entry.go mockgen -package=mocksdcpbpath -source=pkg/tree/api/sdcpb_path.go -destination=$(MOCKDIR)/mocksdcpbpath/sdcpb_path.go - mockgen -package=mockconfigread -destination=$(MOCKDIR)/mockconfigread/client.go github.com/sdcio/sdc-protos/config_read ConfigReadServiceClient + mockgen -package=mockconfigread -destination=$(MOCKDIR)/mockconfigread/client.go github.com/sdcio/sdc-protos/config_read ConfigSnapshotServiceClient .PHONY: mocks-rm mocks-rm: ## remove generated mocks diff --git a/go.mod b/go.mod index 3765815c..924eff88 100644 --- a/go.mod +++ b/go.mod @@ -24,7 +24,7 @@ require ( github.com/sdcio/cache v0.0.38 github.com/sdcio/logger v0.0.3 github.com/sdcio/schema-server v0.0.34 - github.com/sdcio/sdc-protos v0.0.56-0.20260923101805-f894efbc2109 + github.com/sdcio/sdc-protos v0.0.56-0.20260923110351-2e109b71c1dd github.com/sdcio/yang-parser v0.0.12 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 diff --git a/go.sum b/go.sum index f7035da9..f857f68f 100644 --- a/go.sum +++ b/go.sum @@ -193,6 +193,8 @@ github.com/sdcio/schema-server v0.0.34 h1:NNDOkvtUMONtBA7cVvN96F+FWGD/Do6HNqfchy github.com/sdcio/schema-server v0.0.34/go.mod h1:6t8HLXpqUqEJmE5yNZh29u/KZw0jlOICdNWns7zE4GE= github.com/sdcio/sdc-protos v0.0.56-0.20260923101805-f894efbc2109 h1:LUWrCZAnPui2+vx2wdIYVEAHATqCv+/PVbK6QUmPhCY= github.com/sdcio/sdc-protos v0.0.56-0.20260923101805-f894efbc2109/go.mod h1:hER0riYU3G55NnMQ0p0R9kIL/fzqq1zab4pOgUT3Yfg= +github.com/sdcio/sdc-protos v0.0.56-0.20260923110351-2e109b71c1dd h1:JUgGBY1KnMWOP6eNCgE5jcyZ6ZYy75slAk7iRzdI83k= +github.com/sdcio/sdc-protos v0.0.56-0.20260923110351-2e109b71c1dd/go.mod h1:hER0riYU3G55NnMQ0p0R9kIL/fzqq1zab4pOgUT3Yfg= github.com/sdcio/yang-parser v0.0.12 h1:RSSeqfAOIsJx5Lno5u4/ezyOmQYUduQ22rBfU/mtpJ4= github.com/sdcio/yang-parser v0.0.12/go.mod h1:CBqn3Miq85qmFVGHxHXHLluXkaIOsTzV06IM4DW6+D4= github.com/sirikothe/gotextfsm v1.0.1-0.20200816110946-6aa2cfd355e4 h1:FHUL2HofYJuslFOQdy/JjjP36zxqIpd/dcoiwLMIs7k= diff --git a/mocks/mockconfigread/client.go b/mocks/mockconfigread/client.go index e2ddb909..a0dbd9b1 100644 --- a/mocks/mockconfigread/client.go +++ b/mocks/mockconfigread/client.go @@ -1,9 +1,9 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/sdcio/sdc-protos/config_read (interfaces: ConfigReadServiceClient) +// Source: github.com/sdcio/sdc-protos/config_read (interfaces: ConfigSnapshotServiceClient) // // Generated by this command: // -// mockgen -package=mockconfigread -destination=./mocks/mockconfigread/client.go github.com/sdcio/sdc-protos/config_read ConfigReadServiceClient +// mockgen -package=mockconfigread -destination=./mocks/mockconfigread/client.go github.com/sdcio/sdc-protos/config_read ConfigSnapshotServiceClient // // Package mockconfigread is a generated GoMock package. @@ -18,32 +18,52 @@ import ( grpc "google.golang.org/grpc" ) -// MockConfigReadServiceClient is a mock of ConfigReadServiceClient interface. -type MockConfigReadServiceClient struct { +// MockConfigSnapshotServiceClient is a mock of ConfigSnapshotServiceClient interface. +type MockConfigSnapshotServiceClient struct { ctrl *gomock.Controller - recorder *MockConfigReadServiceClientMockRecorder + recorder *MockConfigSnapshotServiceClientMockRecorder isgomock struct{} } -// MockConfigReadServiceClientMockRecorder is the mock recorder for MockConfigReadServiceClient. -type MockConfigReadServiceClientMockRecorder struct { - mock *MockConfigReadServiceClient +// MockConfigSnapshotServiceClientMockRecorder is the mock recorder for MockConfigSnapshotServiceClient. +type MockConfigSnapshotServiceClientMockRecorder struct { + mock *MockConfigSnapshotServiceClient } -// NewMockConfigReadServiceClient creates a new mock instance. -func NewMockConfigReadServiceClient(ctrl *gomock.Controller) *MockConfigReadServiceClient { - mock := &MockConfigReadServiceClient{ctrl: ctrl} - mock.recorder = &MockConfigReadServiceClientMockRecorder{mock} +// NewMockConfigSnapshotServiceClient creates a new mock instance. +func NewMockConfigSnapshotServiceClient(ctrl *gomock.Controller) *MockConfigSnapshotServiceClient { + mock := &MockConfigSnapshotServiceClient{ctrl: ctrl} + mock.recorder = &MockConfigSnapshotServiceClientMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockConfigReadServiceClient) EXPECT() *MockConfigReadServiceClientMockRecorder { +func (m *MockConfigSnapshotServiceClient) EXPECT() *MockConfigSnapshotServiceClientMockRecorder { return m.recorder } +// Delete mocks base method. +func (m *MockConfigSnapshotServiceClient) Delete(ctx context.Context, in *config_read.DeleteConfigRequest, opts ...grpc.CallOption) (*config_read.DeleteConfigResponse, error) { + m.ctrl.T.Helper() + varargs := []any{ctx, in} + for _, a := range opts { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Delete", varargs...) + ret0, _ := ret[0].(*config_read.DeleteConfigResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Delete indicates an expected call of Delete. +func (mr *MockConfigSnapshotServiceClientMockRecorder) Delete(ctx, in any, opts ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx, in}, opts...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockConfigSnapshotServiceClient)(nil).Delete), varargs...) +} + // Get mocks base method. -func (m *MockConfigReadServiceClient) Get(ctx context.Context, in *config_read.GetConfigRequest, opts ...grpc.CallOption) (*config_read.GetConfigResponse, error) { +func (m *MockConfigSnapshotServiceClient) Get(ctx context.Context, in *config_read.GetConfigRequest, opts ...grpc.CallOption) (*config_read.GetConfigResponse, error) { m.ctrl.T.Helper() varargs := []any{ctx, in} for _, a := range opts { @@ -56,14 +76,14 @@ func (m *MockConfigReadServiceClient) Get(ctx context.Context, in *config_read.G } // Get indicates an expected call of Get. -func (mr *MockConfigReadServiceClientMockRecorder) Get(ctx, in any, opts ...any) *gomock.Call { +func (mr *MockConfigSnapshotServiceClientMockRecorder) Get(ctx, in any, opts ...any) *gomock.Call { mr.mock.ctrl.T.Helper() varargs := append([]any{ctx, in}, opts...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockConfigReadServiceClient)(nil).Get), varargs...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockConfigSnapshotServiceClient)(nil).Get), varargs...) } // List mocks base method. -func (m *MockConfigReadServiceClient) List(ctx context.Context, in *config_read.ListConfigRequest, opts ...grpc.CallOption) (*config_read.ListConfigResponse, error) { +func (m *MockConfigSnapshotServiceClient) List(ctx context.Context, in *config_read.ListConfigRequest, opts ...grpc.CallOption) (*config_read.ListConfigResponse, error) { m.ctrl.T.Helper() varargs := []any{ctx, in} for _, a := range opts { @@ -76,8 +96,28 @@ func (m *MockConfigReadServiceClient) List(ctx context.Context, in *config_read. } // List indicates an expected call of List. -func (mr *MockConfigReadServiceClientMockRecorder) List(ctx, in any, opts ...any) *gomock.Call { +func (mr *MockConfigSnapshotServiceClientMockRecorder) List(ctx, in any, opts ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx, in}, opts...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockConfigSnapshotServiceClient)(nil).List), varargs...) +} + +// Modify mocks base method. +func (m *MockConfigSnapshotServiceClient) Modify(ctx context.Context, in *config_read.ModifyConfigRequest, opts ...grpc.CallOption) (*config_read.ModifyConfigResponse, error) { + m.ctrl.T.Helper() + varargs := []any{ctx, in} + for _, a := range opts { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Modify", varargs...) + ret0, _ := ret[0].(*config_read.ModifyConfigResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Modify indicates an expected call of Modify. +func (mr *MockConfigSnapshotServiceClientMockRecorder) Modify(ctx, in any, opts ...any) *gomock.Call { mr.mock.ctrl.T.Helper() varargs := append([]any{ctx, in}, opts...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockConfigReadServiceClient)(nil).List), varargs...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Modify", reflect.TypeOf((*MockConfigSnapshotServiceClient)(nil).Modify), varargs...) } diff --git a/pkg/cache/CONTEXT.md b/pkg/cache/CONTEXT.md index dc74f5c7..2d2c4604 100644 --- a/pkg/cache/CONTEXT.md +++ b/pkg/cache/CONTEXT.md @@ -7,10 +7,30 @@ actually stores it. ## Language **Intent**: -A named, prioritized piece of northbound-authored config, owned by whichever -system is the sole writer for the active `Cache.Type` (config-server, or -data-server itself under `local`). Read through the `Client.InstanceIntent*` -family. +A named, prioritized piece of northbound-authored config — the *desired* +value. Authorship (who decides what an Intent's content should be) belongs +to whichever system is the sole writer for the active `Cache.Type` +(config-server, or data-server itself under `local`). Read through the +`Client.InstanceIntent*` family. _Avoid_: using "Intent" when you mean +last-applied — under `Cache.Type: config-server` these are two different +values with two different owners (see **Last-applied** below). + +**Last-applied**: +The value of a named Intent that this datastore last successfully pushed +southbound — updated at the same moment `IntentModify`/`IntentDelete` run +inside `TransactionSet`'s apply loop, for every `Cache.Type`, not gated on +`TransactionConfirm`. Under `Cache.Type: local` this has always been true by +construction (disk write happens at that exact call). Under +`Cache.Type: config-server`, data-server writes this back into config-server +so `Client.InstanceIntent*` reads (backed by `TargetSnapshot.Spec.Configs`) +never lag behind what was actually applied — a deleted Intent must stop +being last-applied at delete-apply time, not at some later, best-effort +snapshot refresh, or the next `LoadAllButRunningIntents` can rehydrate config +that was meant to be gone. _Avoid_: "confirmed" or "acknowledged" (collide +with the separate `TransactionConfirm` RPC step); "expected state" (says +nothing about whether it's desired-or-applied); and "Document" as a domain +concept (a branch-local Go DTO name for the ConfigSnapshotService +interchange shape — not a term in this language). **Running**: The synced, on-device configuration state — produced and consumed entirely @@ -46,5 +66,19 @@ Tree Entry). **ConfigSnapshotService**: The canonical local gRPC seam name for config-server-backed intent reads and -writes (`Get`/`List`/`Modify`/`Delete`) against `TargetSnapshot` data. _Avoid_: -using the older `ConfigReadService` name for current behavior. +writes (`Get`/`List`/`Modify`/`Delete`) against `TargetSnapshot` data. +Data-server’s Go port over that wire is `ConfigSnapshotClient` (same four +operations, Document-shaped) in `pkg/cache/configserver`, implemented by +`GRPCConfigClient` and `FakeConfigSnapshotClient`. Distinct from the generated +gRPC stub `ConfigSnapshotServiceClient`. _Avoid_: the older +`ConfigReadService` name for current behavior; `LocalConfigClient` / +`LocalConfigReader` / `LocalConfigWriter` (retired half-split names for the +same port). + +**Namespaced name**: +Under `Cache.Type: config-server` only, the `namespace.name` encoding that +splits on the first `.` — Target datastore identity (`prod.srl1`) and Intent +owner / Config identity (`prod.intent1`). Split and strip live next to +`ConfigServerCache`; join lives on the Document DTO (`IntentName`). Not a +`Client`-level concept; local cache has no namespaces. _Avoid_: treating it +as a generic cache key format across backends. diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 3ccc854b..b827423d 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -29,8 +29,11 @@ type IntentReader interface { InstanceIntentGetAll(ctx context.Context, cacheName string, excludeIntentNames []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error) } -// IntentWriter persists real Intents. A backend that does not own those -// writes (config-server: kube-api writes them) composes noopIntentWriter. +// IntentWriter persists real Intents. Every current backend (LocalCache, +// ConfigServerCache) implements this directly. A future backend that +// genuinely cannot own real-Intent writes would need an explicit no-op +// composition at Server.createCacheClient so the type system makes that +// lack of ownership visible. type IntentWriter interface { InstanceIntentModify(ctx context.Context, cacheName string, intent *tree_persist.Intent) error InstanceIntentDelete(ctx context.Context, cacheName string, intentName string, IgnoreNonExisting bool) error diff --git a/pkg/cache/configserver.go b/pkg/cache/configserver.go index 592bd718..2733d8ee 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -18,55 +18,45 @@ import ( "context" "errors" "fmt" - "strings" "sync" "github.com/sdcio/data-server/pkg/cache/configserver" + "github.com/sdcio/data-server/pkg/cache/configsnapshot" "github.com/sdcio/data-server/pkg/tree/importer" treeproto "github.com/sdcio/data-server/pkg/tree/importer/proto" "github.com/sdcio/sdc-protos/tree_persist" ) // ConfigServerCache is the Cache.Type: config-server Client: real Intents -// are read-only, served through a configserver.LocalConfigReader seam over -// the colocated config-server controller's own watch-synced store — this -// backend never persists its own copy of them, and write calls on them are -// unconditional no-ops, since config-server/kube-api is the sole writer. +// are read through and written to a configserver.ConfigSnapshotClient port over +// the colocated config-server controller (ConfigSnapshotService) — this +// backend never persists its own copy of them, and Modify/Delete write +// through synchronously at apply time, the same moment Cache.Type: local +// persists last-applied, so last-applied never lags behind southbound apply +// (see pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md). // "running" is unaffected by backend choice (see the ADR): it is kept in its // own independent in-memory, per-instance store, entirely separate from the -// seam. +// port. type ConfigServerCache struct { - reader configserver.LocalConfigReader + client configserver.ConfigSnapshotClient mu sync.RWMutex running map[string]*tree_persist.Intent } -// NewConfigServerCache returns a Client backed by reader for real Intents. +// NewConfigServerCache returns a Client backed by client for real Intents. // Every InstanceIntent* call derives its target namespace/name from the // cacheInstanceName it's given (see target), so a single ConfigServerCache // correctly serves datastores across multiple Kubernetes namespaces. -func NewConfigServerCache(reader configserver.LocalConfigReader) *ConfigServerCache { +// ConfigServerCache satisfies IntentWriter directly (Modify/Delete write +// through the ConfigSnapshotClient port to config-server). +func NewConfigServerCache(client configserver.ConfigSnapshotClient) *ConfigServerCache { return &ConfigServerCache{ - reader: reader, + client: client, running: map[string]*tree_persist.Intent{}, } } -// NewConfigServerClient composes a reader-backed *ConfigServerCache with the -// generic noopIntentWriter into a full Client. ConfigServerCache alone never -// implements IntentWriter (config-server/kube-api is the sole writer of real -// Intents), so this is the one seam Server.createCacheClient's config-server -// case uses to assemble s.cacheClient. -func NewConfigServerClient(reader configserver.LocalConfigReader) Client { - return struct { - *ConfigServerCache - noopIntentWriter - }{ - ConfigServerCache: NewConfigServerCache(reader), - } -} - // target derives the target namespace/name from cacheInstanceName, mirroring // config-server's own . datastore-naming // convention. It returns ErrMalformedDatastoreName when cacheInstanceName @@ -75,19 +65,6 @@ func (c *ConfigServerCache) target(cacheInstanceName string) (configserver.Targe return splitDatastoreName(cacheInstanceName) } -// lookupConfigName maps an owner/intent name onto the bare Config resource -// name ConfigReadService keys TargetSnapshot.Spec.Configs by. GetGVKNSN -// names (".") are stripped when the namespace matches the -// target; a bare name is passed through unchanged so existing Get callers -// keep working. -func lookupConfigName(target configserver.Target, intentName string) string { - prefix := target.Namespace + "." - if rest, ok := strings.CutPrefix(intentName, prefix); ok && rest != "" { - return rest - } - return intentName -} - // InstanceCreate/InstanceDelete/InstanceClose/InstanceExists/InstancesList // only manage the independent "running" store: real intents live in // config-server, never locally, so there is nothing else to create/delete. @@ -159,7 +136,7 @@ func (c *ConfigServerCache) InstanceIntentsList(ctx context.Context, cacheInstan if err != nil { return nil, err } - docs, err := c.reader.List(ctx, target) + docs, err := c.client.List(ctx, target) if err != nil { return nil, err } @@ -177,11 +154,11 @@ func (c *ConfigServerCache) InstanceIntentGet(ctx context.Context, cacheName str if err != nil { return nil, err } - doc, err := c.reader.Get(ctx, target, lookupConfigName(target, intentName)) + doc, err := c.client.Get(ctx, target, lookupConfigName(target, intentName)) if err != nil { return nil, err } - return configserver.NewImportAdapter(doc) + return configsnapshot.NewImportAdapter(doc) } // InstanceIntentExists calls Get and maps "not found" to (false, nil), @@ -192,7 +169,7 @@ func (c *ConfigServerCache) InstanceIntentExists(ctx context.Context, cacheName if err != nil { return false, err } - _, err = c.reader.Get(ctx, target, lookupConfigName(target, intentName)) + _, err = c.client.Get(ctx, target, lookupConfigName(target, intentName)) if err != nil { if errors.Is(err, configserver.ErrNotFound) { return false, nil @@ -219,14 +196,14 @@ func (c *ConfigServerCache) InstanceIntentGetAll(ctx context.Context, cacheName return } - docs, err := c.reader.List(ctx, target) + docs, err := c.client.List(ctx, target) if err != nil { errChan <- err return } for _, d := range docs { - adapter, err := configserver.NewImportAdapter(d) + adapter, err := configsnapshot.NewImportAdapter(d) if err != nil { errChan <- err return @@ -241,7 +218,7 @@ func (c *ConfigServerCache) InstanceIntentGetAll(ctx context.Context, cacheName // InstanceRunningGet/InstanceRunningModify back "running" with its own // independent in-memory, per-instance store — entirely separate from the -// seam, since "running" never touches config-server under any backend. +// port, since "running" never touches config-server under any backend. func (c *ConfigServerCache) InstanceRunningGet(ctx context.Context, cacheName string) (importer.ImportConfigAdapter, error) { c.mu.RLock() @@ -262,3 +239,45 @@ func (c *ConfigServerCache) InstanceRunningModify(ctx context.Context, cacheName c.running[cacheName] = intent return nil } + +// InstanceIntentModify flattens intent into a Document (see +// configsnapshot.DocumentFromIntent) and writes it through the port +// synchronously, at the same moment TransactionSet's apply loop calls it — +// matching Cache.Type: local's write-at-apply timing so last-applied never +// lags behind southbound apply. +func (c *ConfigServerCache) InstanceIntentModify(ctx context.Context, cacheName string, intent *tree_persist.Intent) error { + target, err := c.target(cacheName) + if err != nil { + return err + } + name := lookupConfigName(target, intent.GetIntentName()) + doc, err := configsnapshot.DocumentFromIntent(target, name, intent) + if err != nil { + return err + } + return c.client.Modify(ctx, target, doc) +} + +// InstanceIntentDelete writes through the port's Delete synchronously. +// ignoreNonExisting is part of the IntentWriter signature but unused here: +// ConfigSnapshotClient.Delete's current implementations (GRPCConfigClient, +// FakeConfigSnapshotClient) are unconditionally idempotent on a missing name — +// TargetSnapshot's membership model has no tombstone to distinguish +// "already gone" from "never existed" — so there's nothing for this flag to +// gate against today. A future ConfigSnapshotClient that needed to distinguish +// the two would take ignoreNonExisting as a parameter on Delete itself. +func (c *ConfigServerCache) InstanceIntentDelete(ctx context.Context, cacheName string, intentName string, ignoreNonExisting bool) error { + target, err := c.target(cacheName) + if err != nil { + return err + } + return c.client.Delete(ctx, target, lookupConfigName(target, intentName)) +} + +var ( + _ IntentReader = (*ConfigServerCache)(nil) + _ IntentWriter = (*ConfigServerCache)(nil) + _ RunningStore = (*ConfigServerCache)(nil) + _ InstanceLifecycle = (*ConfigServerCache)(nil) + _ Client = (*ConfigServerCache)(nil) +) diff --git a/pkg/cache/configserver/client.go b/pkg/cache/configserver/client.go new file mode 100644 index 00000000..a1d15d0e --- /dev/null +++ b/pkg/cache/configserver/client.go @@ -0,0 +1,76 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package configserver defines the Go port between the config-server-backed +// cache.Client and the ConfigSnapshotService wire it reads and writes real +// Intents through. +// +// ConfigSnapshotClient is exactly what that backend needs: Get/List/Modify/ +// Delete against Document-shaped DTOs. Nothing more. This decouples the +// backend from the cross-repo transport (a unary gRPC ConfigSnapshotService +// on config-server's colocated controller, defined in sdc-protos and +// implemented here by GRPCConfigClient) — the backend can still be fully +// unit-tested against the fake in this package. +// +// Capability segregation (IntentReader / IntentWriter) lives one layer up on +// cache.Client — see ADR 0002 / 0003. The Document-shaped DTO and +// Intent↔DTO↔ImportConfigAdapter codec live in pkg/cache/configsnapshot; +// this package owns only the remote port. +package configserver + +import ( + "context" + "errors" + + "github.com/sdcio/data-server/pkg/cache/configsnapshot" +) + +// ErrNotFound is returned by ConfigSnapshotClient.Get when no Document exists +// for the given target/name pair. +var ErrNotFound = errors.New("configserver: document not found") + +// Target, ConfigBlob, and Document are the ConfigSnapshotService interchange +// DTOs owned by configsnapshot. Aliased here so the ConfigSnapshotClient +// contract keeps a stable import path for port callers. +type ( + Target = configsnapshot.Target + ConfigBlob = configsnapshot.ConfigBlob + Document = configsnapshot.Document +) + +// ConfigSnapshotClient is the Go port over ConfigSnapshotService: get/list/ +// upsert/delete Document entries scoped to a target. Distinct from the +// generated config_read.ConfigSnapshotServiceClient stub. Implemented by +// GRPCConfigClient and FakeConfigSnapshotClient. +// +// Modify/Delete write what data-server's TransactionSet apply loop actually +// pushed south, at the same moment Cache.Type: local persists it, so +// last-applied never lags behind southbound apply (see +// pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md). +type ConfigSnapshotClient interface { + // Get returns the Document named name for target. It returns ErrNotFound + // (wrapped or not) if no such Document exists. + Get(ctx context.Context, target Target, name string) (*Document, error) + // List returns every Document scoped to target. An empty/nil slice with a + // nil error means the target has no Documents. + List(ctx context.Context, target Target) ([]*Document, error) + // Modify upserts doc into TargetSnapshot.Spec.Configs[doc.Name] for + // target. + Modify(ctx context.Context, target Target, doc *Document) error + // Delete removes name from TargetSnapshot.Spec.Configs for target. + // Membership is a single map with no tombstone: deleting a name that + // isn't (or is no longer) present is a no-op success, not an error, so + // retries and out-of-order delivery stay idempotent. + Delete(ctx context.Context, target Target, name string) error +} diff --git a/pkg/cache/configserver/fake.go b/pkg/cache/configserver/fake.go index 354dda80..40c2138e 100644 --- a/pkg/cache/configserver/fake.go +++ b/pkg/cache/configserver/fake.go @@ -20,25 +20,26 @@ import ( "sync" ) -// FakeLocalConfigReader is an in-memory LocalConfigReader, seedable with test -// fixtures. It stands in for the real config-server local-read transport in -// unit tests, so the config-server-backed cache.Client can be built and fully -// tested well before that transport exists. -type FakeLocalConfigReader struct { +// FakeConfigSnapshotClient is an in-memory ConfigSnapshotClient, seedable +// with test fixtures. It stands in for the real config-server transport in +// unit tests, so the config-server-backed cache.Client can be built and +// fully tested against realistic read-after-write behavior (Modify/Delete +// mutate the same map Get/List read from) without a live config-server. +type FakeConfigSnapshotClient struct { mu sync.RWMutex docs map[Target]map[string]*Document } -// NewFakeLocalConfigReader returns an empty FakeLocalConfigReader. Use Seed -// to populate it with fixtures. -func NewFakeLocalConfigReader() *FakeLocalConfigReader { - return &FakeLocalConfigReader{ +// NewFakeConfigSnapshotClient returns an empty FakeConfigSnapshotClient. +// Use Seed to populate it with fixtures. +func NewFakeConfigSnapshotClient() *FakeConfigSnapshotClient { + return &FakeConfigSnapshotClient{ docs: map[Target]map[string]*Document{}, } } // Seed adds/replaces Documents for target, keyed by Document.Name. -func (f *FakeLocalConfigReader) Seed(target Target, docs ...*Document) { +func (f *FakeConfigSnapshotClient) Seed(target Target, docs ...*Document) { f.mu.Lock() defer f.mu.Unlock() @@ -53,13 +54,13 @@ func (f *FakeLocalConfigReader) Seed(target Target, docs ...*Document) { } // Reset removes every seeded Document for every target. -func (f *FakeLocalConfigReader) Reset() { +func (f *FakeConfigSnapshotClient) Reset() { f.mu.Lock() defer f.mu.Unlock() f.docs = map[Target]map[string]*Document{} } -func (f *FakeLocalConfigReader) Get(ctx context.Context, target Target, name string) (*Document, error) { +func (f *FakeConfigSnapshotClient) Get(ctx context.Context, target Target, name string) (*Document, error) { f.mu.RLock() defer f.mu.RUnlock() @@ -74,7 +75,7 @@ func (f *FakeLocalConfigReader) Get(ctx context.Context, target Target, name str return doc, nil } -func (f *FakeLocalConfigReader) List(ctx context.Context, target Target) ([]*Document, error) { +func (f *FakeConfigSnapshotClient) List(ctx context.Context, target Target) ([]*Document, error) { f.mu.RLock() defer f.mu.RUnlock() @@ -88,4 +89,22 @@ func (f *FakeLocalConfigReader) List(ctx context.Context, target Target) ([]*Doc return result, nil } -var _ LocalConfigReader = (*FakeLocalConfigReader)(nil) +// Modify is Seed for a single Document — same underlying map, so a Modify +// is immediately visible to Get/List, the real-write behavior this fake +// exists to let tests exercise. +func (f *FakeConfigSnapshotClient) Modify(ctx context.Context, target Target, doc *Document) error { + f.Seed(target, doc) + return nil +} + +// Delete removes name from target's Documents. Deleting a name that isn't +// (or is no longer) present is a no-op success, matching the real +// ConfigSnapshotService.Delete contract. +func (f *FakeConfigSnapshotClient) Delete(ctx context.Context, target Target, name string) error { + f.mu.Lock() + defer f.mu.Unlock() + delete(f.docs[target], name) + return nil +} + +var _ ConfigSnapshotClient = (*FakeConfigSnapshotClient)(nil) diff --git a/pkg/cache/configserver/fake_test.go b/pkg/cache/configserver/fake_test.go index 06a31d41..06718a11 100644 --- a/pkg/cache/configserver/fake_test.go +++ b/pkg/cache/configserver/fake_test.go @@ -24,8 +24,8 @@ import ( "google.golang.org/protobuf/testing/protocmp" ) -func TestFakeLocalConfigReader_GetNotFound(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_GetNotFound(t *testing.T) { + f := NewFakeConfigSnapshotClient() _, err := f.Get(context.Background(), Target{Namespace: "ns1", Name: "target1"}, "missing") if !errors.Is(err, ErrNotFound) { @@ -33,8 +33,8 @@ func TestFakeLocalConfigReader_GetNotFound(t *testing.T) { } } -func TestFakeLocalConfigReader_GetNotFound_UnknownTarget(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_GetNotFound_UnknownTarget(t *testing.T) { + f := NewFakeConfigSnapshotClient() f.Seed(Target{Namespace: "ns1", Name: "target1"}, &Document{Name: "intent1"}) _, err := f.Get(context.Background(), Target{Namespace: "ns1", Name: "other-target"}, "intent1") @@ -43,8 +43,8 @@ func TestFakeLocalConfigReader_GetNotFound_UnknownTarget(t *testing.T) { } } -func TestFakeLocalConfigReader_SeedAndGet(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_SeedAndGet(t *testing.T) { + f := NewFakeConfigSnapshotClient() target := Target{Namespace: "ns1", Name: "target1"} want := &Document{ @@ -70,8 +70,8 @@ func TestFakeLocalConfigReader_SeedAndGet(t *testing.T) { } } -func TestFakeLocalConfigReader_List(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_List(t *testing.T) { + f := NewFakeConfigSnapshotClient() target := Target{Namespace: "ns1", Name: "target1"} other := Target{Namespace: "ns1", Name: "other-target"} @@ -92,8 +92,8 @@ func TestFakeLocalConfigReader_List(t *testing.T) { } } -func TestFakeLocalConfigReader_ListEmptyForUnknownTarget(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_ListEmptyForUnknownTarget(t *testing.T) { + f := NewFakeConfigSnapshotClient() got, err := f.List(context.Background(), Target{Namespace: "ns1", Name: "target1"}) if err != nil { @@ -104,8 +104,8 @@ func TestFakeLocalConfigReader_ListEmptyForUnknownTarget(t *testing.T) { } } -func TestFakeLocalConfigReader_SeedReplacesByName(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_SeedReplacesByName(t *testing.T) { + f := NewFakeConfigSnapshotClient() target := Target{Namespace: "ns1", Name: "target1"} f.Seed(target, &Document{Name: "intent1", Priority: 1}) @@ -120,8 +120,8 @@ func TestFakeLocalConfigReader_SeedReplacesByName(t *testing.T) { } } -func TestFakeLocalConfigReader_Reset(t *testing.T) { - f := NewFakeLocalConfigReader() +func TestFakeConfigSnapshotClient_Reset(t *testing.T) { + f := NewFakeConfigSnapshotClient() target := Target{Namespace: "ns1", Name: "target1"} f.Seed(target, &Document{Name: "intent1"}) @@ -132,3 +132,51 @@ func TestFakeLocalConfigReader_Reset(t *testing.T) { t.Fatalf("Get() error = %v, want ErrNotFound after Reset", err) } } + +// TestFakeConfigSnapshotClient_ModifyThenGet locks read-after-write: a Modify +// must be immediately visible to Get, the real-write behavior this fake +// exists to let ConfigServerCache's IntentWriter path be tested against. +func TestFakeConfigSnapshotClient_ModifyThenGet(t *testing.T) { + f := NewFakeConfigSnapshotClient() + target := Target{Namespace: "ns1", Name: "target1"} + + if err := f.Modify(context.Background(), target, &Document{Name: "intent1", Priority: 5}); err != nil { + t.Fatalf("Modify() error = %v", err) + } + + got, err := f.Get(context.Background(), target, "intent1") + if err != nil { + t.Fatalf("Get() after Modify: %v", err) + } + if got.Priority != 5 { + t.Errorf("Get().Priority = %d, want 5", got.Priority) + } +} + +// TestFakeConfigSnapshotClient_DeleteRemovesEntry locks read-after-delete. +func TestFakeConfigSnapshotClient_DeleteRemovesEntry(t *testing.T) { + f := NewFakeConfigSnapshotClient() + target := Target{Namespace: "ns1", Name: "target1"} + f.Seed(target, &Document{Name: "intent1"}, &Document{Name: "intent2"}) + + if err := f.Delete(context.Background(), target, "intent1"); err != nil { + t.Fatalf("Delete() error = %v", err) + } + + if _, err := f.Get(context.Background(), target, "intent1"); !errors.Is(err, ErrNotFound) { + t.Errorf("Get() after Delete: err = %v, want ErrNotFound", err) + } + if _, err := f.Get(context.Background(), target, "intent2"); err != nil { + t.Errorf("Get() intent2 after deleting intent1: %v, want untouched entry", err) + } +} + +// TestFakeConfigSnapshotClient_DeleteMissingIsNoop locks idempotent-delete, +// matching the real ConfigSnapshotService.Delete contract. +func TestFakeConfigSnapshotClient_DeleteMissingIsNoop(t *testing.T) { + f := NewFakeConfigSnapshotClient() + + if err := f.Delete(context.Background(), Target{Namespace: "ns1", Name: "target1"}, "missing"); err != nil { + t.Errorf("Delete() of missing entry: %v, want no-op success", err) + } +} diff --git a/pkg/cache/configserver/grpc.go b/pkg/cache/configserver/grpc.go index b4c21421..60472924 100644 --- a/pkg/cache/configserver/grpc.go +++ b/pkg/cache/configserver/grpc.go @@ -24,23 +24,26 @@ import ( "google.golang.org/grpc/status" ) -// GRPCConfigReader implements LocalConfigReader over the real -// config_read.ConfigReadService, the localhost-bound gRPC surface ticket 07 -// serves from inside the colocated config-server controller. +// GRPCConfigClient implements ConfigSnapshotClient over the real +// config_read.ConfigSnapshotService, the localhost-bound gRPC surface +// served from inside the colocated config-server controller. +// Get/List/Modify/Delete are all one RPC service over one resource +// (TargetSnapshot), so one generated client structurally satisfies the +// ConfigSnapshotClient port (see ADR 0003's "Wire contract" section). // // It takes a grpc.ClientConnInterface rather than dialing one itself, // mirroring pkg/schema.NewRemoteClient: dial address/credentials are the // caller's concern (Dial below is one convenience constructor for the // localhost/insecure case this service is always deployed as), so nothing // here is hardcoded. -type GRPCConfigReader struct { - client config_read.ConfigReadServiceClient +type GRPCConfigClient struct { + client config_read.ConfigSnapshotServiceClient } -// NewGRPCConfigReader returns a LocalConfigReader that calls the -// ConfigReadService over cc. -func NewGRPCConfigReader(cc grpc.ClientConnInterface) *GRPCConfigReader { - return &GRPCConfigReader{client: config_read.NewConfigReadServiceClient(cc)} +// NewGRPCConfigClient returns a ConfigSnapshotClient that calls the +// ConfigSnapshotService over cc. +func NewGRPCConfigClient(cc grpc.ClientConnInterface) *GRPCConfigClient { + return &GRPCConfigClient{client: config_read.NewConfigSnapshotServiceClient(cc)} } // Dial establishes an insecure gRPC connection to address, the shape every @@ -56,10 +59,10 @@ func Dial(address string, opts ...grpc.DialOption) (*grpc.ClientConn, error) { return grpc.NewClient(address, dialOpts...) } -// Get calls ConfigReadService.Get, mapping a NotFound gRPC status to -// ErrNotFound per the LocalConfigReader contract; any other error +// Get calls ConfigSnapshotService.Get, mapping a NotFound gRPC status to +// ErrNotFound per the ConfigSnapshotClient contract; any other error // propagates unwrapped. -func (r *GRPCConfigReader) Get(ctx context.Context, target Target, name string) (*Document, error) { +func (r *GRPCConfigClient) Get(ctx context.Context, target Target, name string) (*Document, error) { rsp, err := r.client.Get(ctx, &config_read.GetConfigRequest{ TargetNamespace: target.Namespace, TargetName: target.Name, @@ -74,9 +77,9 @@ func (r *GRPCConfigReader) Get(ctx context.Context, target Target, name string) return documentFromEntry(rsp.GetConfig()), nil } -// List calls ConfigReadService.List, mapping every returned ConfigEntry into -// a Document. -func (r *GRPCConfigReader) List(ctx context.Context, target Target) ([]*Document, error) { +// List calls ConfigSnapshotService.List, mapping every returned ConfigEntry +// into a Document. +func (r *GRPCConfigClient) List(ctx context.Context, target Target) ([]*Document, error) { rsp, err := r.client.List(ctx, &config_read.ListConfigRequest{ TargetNamespace: target.Namespace, TargetName: target.Name, @@ -92,9 +95,9 @@ func (r *GRPCConfigReader) List(ctx context.Context, target Target) ([]*Document return docs, nil } -// documentFromEntry maps a config_read.ConfigEntry onto the seam's Document +// documentFromEntry maps a config_read.ConfigEntry onto the port's Document // shape, field for field — no further translation happens here, that's -// NewImportAdapter's job. +// configsnapshot.NewImportAdapter's job. func documentFromEntry(e *config_read.ConfigEntry) *Document { blobs := e.GetConfig() config := make([]*ConfigBlob, 0, len(blobs)) @@ -112,4 +115,49 @@ func documentFromEntry(e *config_read.ConfigEntry) *Document { } } -var _ LocalConfigReader = (*GRPCConfigReader)(nil) +// configEntryFromDocument maps a Document onto the wire config_read.ConfigEntry +// shape, field for field — the inverse of documentFromEntry. +func configEntryFromDocument(doc *Document) *config_read.ConfigEntry { + blobs := make([]*config_read.ConfigBlob, 0, len(doc.Config)) + for _, b := range doc.Config { + blobs = append(blobs, &config_read.ConfigBlob{Path: b.Path, Value: b.Value}) + } + return &config_read.ConfigEntry{ + Name: doc.Name, + Namespace: doc.Namespace, + NonRevertive: doc.NonRevertive, + Orphan: doc.Orphan, + Priority: doc.Priority, + SensitivePaths: doc.SensitivePaths, + Config: blobs, + } +} + +// Modify calls ConfigSnapshotService.Modify, mapping doc onto the wire +// ConfigEntry shape via configEntryFromDocument. The payload crosses the +// wire as plaintext, deliberately: ADR 0003's "encrypted payload" describes +// the persisted TargetSnapshot record, not this RPC's bytes — config-server +// encrypts on receipt (see config-server's fromConfigEntry), the same way +// it already encrypts SensitiveConfig content it receives northbound. +func (r *GRPCConfigClient) Modify(ctx context.Context, target Target, doc *Document) error { + _, err := r.client.Modify(ctx, &config_read.ModifyConfigRequest{ + TargetNamespace: target.Namespace, + TargetName: target.Name, + Config: configEntryFromDocument(doc), + }) + return err +} + +// Delete calls ConfigSnapshotService.Delete. The server side already treats +// a missing key/snapshot as a no-op success, so no NotFound mapping is needed +// here. +func (r *GRPCConfigClient) Delete(ctx context.Context, target Target, name string) error { + _, err := r.client.Delete(ctx, &config_read.DeleteConfigRequest{ + TargetNamespace: target.Namespace, + TargetName: target.Name, + Name: name, + }) + return err +} + +var _ ConfigSnapshotClient = (*GRPCConfigClient)(nil) diff --git a/pkg/cache/configserver/grpc_test.go b/pkg/cache/configserver/grpc_test.go index df023080..4a9bb476 100644 --- a/pkg/cache/configserver/grpc_test.go +++ b/pkg/cache/configserver/grpc_test.go @@ -30,9 +30,9 @@ import ( "github.com/sdcio/sdc-protos/config_read" ) -func TestGRPCConfigReader_Get(t *testing.T) { +func TestGRPCConfigClient_Get(t *testing.T) { ctrl := gomock.NewController(t) - client := mockconfigread.NewMockConfigReadServiceClient(ctrl) + client := mockconfigread.NewMockConfigSnapshotServiceClient(ctrl) target := Target{Namespace: "ns1", Name: "target1"} client.EXPECT().Get(gomock.Any(), &config_read.GetConfigRequest{ @@ -55,7 +55,7 @@ func TestGRPCConfigReader_Get(t *testing.T) { }, }, nil) - r := NewGRPCConfigReader(nil) + r := NewGRPCConfigClient(nil) r.client = client want := &Document{ @@ -81,15 +81,15 @@ func TestGRPCConfigReader_Get(t *testing.T) { } } -func TestGRPCConfigReader_Get_NotFound(t *testing.T) { +func TestGRPCConfigClient_Get_NotFound(t *testing.T) { ctrl := gomock.NewController(t) - client := mockconfigread.NewMockConfigReadServiceClient(ctrl) + client := mockconfigread.NewMockConfigSnapshotServiceClient(ctrl) target := Target{Namespace: "ns1", Name: "target1"} client.EXPECT().Get(gomock.Any(), gomock.Any()). Return(nil, status.Error(codes.NotFound, "no such Config")) - r := NewGRPCConfigReader(nil) + r := NewGRPCConfigClient(nil) r.client = client _, err := r.Get(context.Background(), target, "missing") @@ -98,15 +98,15 @@ func TestGRPCConfigReader_Get_NotFound(t *testing.T) { } } -func TestGRPCConfigReader_Get_OtherError(t *testing.T) { +func TestGRPCConfigClient_Get_OtherError(t *testing.T) { ctrl := gomock.NewController(t) - client := mockconfigread.NewMockConfigReadServiceClient(ctrl) + client := mockconfigread.NewMockConfigSnapshotServiceClient(ctrl) target := Target{Namespace: "ns1", Name: "target1"} wantErr := status.Error(codes.Unavailable, "controller unreachable") client.EXPECT().Get(gomock.Any(), gomock.Any()).Return(nil, wantErr) - r := NewGRPCConfigReader(nil) + r := NewGRPCConfigClient(nil) r.client = client _, err := r.Get(context.Background(), target, "intent1") @@ -118,9 +118,9 @@ func TestGRPCConfigReader_Get_OtherError(t *testing.T) { } } -func TestGRPCConfigReader_List(t *testing.T) { +func TestGRPCConfigClient_List(t *testing.T) { ctrl := gomock.NewController(t) - client := mockconfigread.NewMockConfigReadServiceClient(ctrl) + client := mockconfigread.NewMockConfigSnapshotServiceClient(ctrl) target := Target{Namespace: "ns1", Name: "target1"} client.EXPECT().List(gomock.Any(), &config_read.ListConfigRequest{ @@ -133,7 +133,7 @@ func TestGRPCConfigReader_List(t *testing.T) { }, }, nil) - r := NewGRPCConfigReader(nil) + r := NewGRPCConfigClient(nil) r.client = client got, err := r.List(context.Background(), target) @@ -149,15 +149,15 @@ func TestGRPCConfigReader_List(t *testing.T) { } } -func TestGRPCConfigReader_List_Empty(t *testing.T) { +func TestGRPCConfigClient_List_Empty(t *testing.T) { ctrl := gomock.NewController(t) - client := mockconfigread.NewMockConfigReadServiceClient(ctrl) + client := mockconfigread.NewMockConfigSnapshotServiceClient(ctrl) target := Target{Namespace: "ns1", Name: "target1"} client.EXPECT().List(gomock.Any(), gomock.Any()). Return(&config_read.ListConfigResponse{}, nil) - r := NewGRPCConfigReader(nil) + r := NewGRPCConfigClient(nil) r.client = client got, err := r.List(context.Background(), target) @@ -169,15 +169,15 @@ func TestGRPCConfigReader_List_Empty(t *testing.T) { } } -func TestGRPCConfigReader_List_Error(t *testing.T) { +func TestGRPCConfigClient_List_Error(t *testing.T) { ctrl := gomock.NewController(t) - client := mockconfigread.NewMockConfigReadServiceClient(ctrl) + client := mockconfigread.NewMockConfigSnapshotServiceClient(ctrl) target := Target{Namespace: "ns1", Name: "target1"} wantErr := status.Error(codes.Unavailable, "controller unreachable") client.EXPECT().List(gomock.Any(), gomock.Any()).Return(nil, wantErr) - r := NewGRPCConfigReader(nil) + r := NewGRPCConfigClient(nil) r.client = client _, err := r.List(context.Background(), target) diff --git a/pkg/cache/configserver/localread.go b/pkg/cache/configserver/localread.go deleted file mode 100644 index 7e90a01a..00000000 --- a/pkg/cache/configserver/localread.go +++ /dev/null @@ -1,94 +0,0 @@ -// Copyright 2024 Nokia -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package configserver defines the seam between the config-server-backed -// cache.Client and the outside world it reads real Intents from. -// -// LocalConfigReader is exactly what that backend needs: get one Document by -// name, list every Document for a target. Nothing more. This decouples the -// backend (built behind this interface) from the real cross-repo transport -// (a unary gRPC service on config-server's colocated controller, defined in -// sdc-protos and wired up in later tickets) — the backend can be built and -// fully unit-tested against the fake in this package right now. -package configserver - -import ( - "context" - "errors" - - sdcpb "github.com/sdcio/sdc-protos/sdcpb" -) - -// ErrNotFound is returned by LocalConfigReader.Get when no Document exists -// for the given target/name pair. -var ErrNotFound = errors.New("configserver: document not found") - -// Target scopes a Get/List call to a single southbound target, mirroring the -// TargetNamespaceKey/TargetNameKey labels config-server's controller already -// indexes Config resources by (see the ADR's "Scope note"). -type Target struct { - Namespace string - Name string -} - -// ConfigBlob is a single raw path + JSON value entry from a Document's -// config payload — shaped so it parses cleanly into the same form -// importer/json.JsonTreeImporter already consumes. -type ConfigBlob struct { - Path string - // Value is the raw JSON encoding of the value at Path. - Value []byte -} - -// Document is the seam's representation of one config-server Config (joined -// with its SensitiveConfig, if any). It carries exactly the fields the ADR's -// field-mapping table needs to build an importer.ImportConfigAdapter: -// name, namespace, priority, non-revertive flag, orphan flag, sensitive -// paths, and the raw config payload. -type Document struct { - // Name is the Config resource's metadata.name — the lookup key - // ConfigReadService uses against TargetSnapshot.Spec.Configs. - Name string - // Namespace is the Config's Kubernetes namespace, populated from - // ConfigEntry.Namespace. Together with Name it forms the owner string - // config-server uses on TransactionSet (config.GetGVKNSN). - Namespace string - Priority int32 - NonRevertive bool - Orphan bool - SensitivePaths []*sdcpb.Path - Config []*ConfigBlob -} - -// IntentName is the owner name data-server and config-server share for a -// Config: ".", matching config.GetGVKNSN. When Namespace is -// empty the bare Name is returned, so incomplete fixtures stay usable. -func (d *Document) IntentName() string { - if d.Namespace == "" { - return d.Name - } - return d.Namespace + "." + d.Name -} - -// LocalConfigReader is the local-read seam a config-server-backed -// cache.Client depends on: get one Document by name, list every Document -// scoped to a target. -type LocalConfigReader interface { - // Get returns the Document named name for target. It returns ErrNotFound - // (wrapped or not) if no such Document exists. - Get(ctx context.Context, target Target, name string) (*Document, error) - // List returns every Document scoped to target. An empty/nil slice with a - // nil error means the target has no Documents. - List(ctx context.Context, target Target) ([]*Document, error) -} diff --git a/pkg/cache/configserver_split.go b/pkg/cache/configserver_names.go similarity index 69% rename from pkg/cache/configserver_split.go rename to pkg/cache/configserver_names.go index 07236999..51ae73f8 100644 --- a/pkg/cache/configserver_split.go +++ b/pkg/cache/configserver_names.go @@ -26,7 +26,7 @@ import ( // name. var ErrMalformedDatastoreName = errors.New("configserver cache: malformed datastore name") -// splitDatastoreName decodes a datastore name (e.g. "prod.srl1") into a +// splitDatastoreName decodes a Namespaced name (e.g. "prod.srl1") into a // configserver.Target, mirroring config-server's own encoding // (storebackend.Key.String(), "."). It splits // on the first '.' only, since a Kubernetes namespace is always a DNS-1123 @@ -40,3 +40,17 @@ func splitDatastoreName(cacheInstanceName string) (configserver.Target, error) { } return configserver.Target{Namespace: namespace, Name: name}, nil } + +// lookupConfigName maps an owner/intent Namespaced name onto the bare Config +// resource name ConfigSnapshotService keys TargetSnapshot.Spec.Configs by. +// GetGVKNSN names (".") are stripped when the namespace +// matches the target; a bare name is passed through unchanged so existing +// Get callers keep working. An empty rest after a matching prefix +// ("prod.") is also passed through unchanged. +func lookupConfigName(target configserver.Target, intentName string) string { + prefix := target.Namespace + "." + if rest, ok := strings.CutPrefix(intentName, prefix); ok && rest != "" { + return rest + } + return intentName +} diff --git a/pkg/cache/configserver_split_test.go b/pkg/cache/configserver_names_test.go similarity index 65% rename from pkg/cache/configserver_split_test.go rename to pkg/cache/configserver_names_test.go index 394f038f..a1c2c1a9 100644 --- a/pkg/cache/configserver_split_test.go +++ b/pkg/cache/configserver_names_test.go @@ -73,3 +73,48 @@ func TestSplitDatastoreName(t *testing.T) { }) } } + +func TestLookupConfigName(t *testing.T) { + target := configserver.Target{Namespace: "prod", Name: "srl1"} + + tests := []struct { + name string + intentName string + want string + }{ + { + name: "prefix match", + intentName: "prod.intent1", + want: "intent1", + }, + { + name: "prefix match with dots in rest", + intentName: "prod.rack1.intent1", + want: "rack1.intent1", + }, + { + name: "bare pass-through", + intentName: "intent1", + want: "intent1", + }, + { + name: "other namespace pass-through", + intentName: "other.intent1", + want: "other.intent1", + }, + { + name: "empty rest", + intentName: "prod.", + want: "prod.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := lookupConfigName(target, tt.intentName) + if got != tt.want { + t.Errorf("lookupConfigName(%+v, %q) = %q, want %q", target, tt.intentName, got, tt.want) + } + }) + } +} diff --git a/pkg/cache/configserver_test.go b/pkg/cache/configserver_test.go index 0fdadd02..309af4d5 100644 --- a/pkg/cache/configserver_test.go +++ b/pkg/cache/configserver_test.go @@ -25,22 +25,34 @@ import ( "github.com/sdcio/data-server/pkg/tree/importer" sdcpb "github.com/sdcio/sdc-protos/sdcpb" "github.com/sdcio/sdc-protos/tree_persist" + "google.golang.org/protobuf/proto" ) +// leafVariant marshals tv the same way TreeExport does (le.ValueAsBytes()), +// so fixtures build TreeElements the way a real export would. +func leafVariant(t *testing.T, tv *sdcpb.TypedValue) []byte { + t.Helper() + b, err := proto.Marshal(tv) + if err != nil { + t.Fatalf("marshal TypedValue: %v", err) + } + return b +} + const ( testNamespace = "ns1" testTarget = "target1" testCacheName = testNamespace + "." + testTarget ) -func newTestConfigServerCache(t *testing.T) (*ConfigServerCache, *configserver.FakeLocalConfigReader) { +func newTestConfigServerCache(t *testing.T) (*ConfigServerCache, *configserver.FakeConfigSnapshotClient) { t.Helper() - reader := configserver.NewFakeLocalConfigReader() - return NewConfigServerCache(reader), reader + client := configserver.NewFakeConfigSnapshotClient() + return NewConfigServerCache(client), client } // TestConfigServerCache_InstanceIntentGet_FieldMapping verifies -// InstanceIntentGet calls the seam's Get and wraps the result per the ADR's +// InstanceIntentGet calls the port's Get and wraps the result per the ADR's // field-mapping table, covering it end to end against the fake. func TestConfigServerCache_InstanceIntentGet_FieldMapping(t *testing.T) { ctx := context.Background() @@ -227,31 +239,6 @@ func TestConfigServerCache_InstanceIntentExists(t *testing.T) { } } -// TestConfigServerCache_WritesAreNoOps verifies the full Client built by -// NewConfigServerClient never errors, never panics, and never reaches the -// seam on IntentWriter calls (the fake has no Modify/Delete methods at -// all — only Get/List — so any attempt to use it that way wouldn't compile -// in the first place). Real-Intent writes are composed in via -// noopIntentWriter, not implemented directly on ConfigServerCache. -func TestConfigServerCache_WritesAreNoOps(t *testing.T) { - ctx := context.Background() - reader := configserver.NewFakeLocalConfigReader() - c := NewConfigServerClient(reader) - - if err := c.InstanceIntentModify(ctx, testCacheName, &tree_persist.Intent{IntentName: "intent1"}); err != nil { - t.Errorf("InstanceIntentModify() error = %v, want nil", err) - } - if err := c.InstanceIntentDelete(ctx, testCacheName, "intent1", false); err != nil { - t.Errorf("InstanceIntentDelete() error = %v, want nil", err) - } - - // A write must not make the intent appear/disappear from the seam's - // perspective — this backend never touches it either way. - if _, err := c.InstanceIntentGet(ctx, testCacheName, "intent1"); !errors.Is(err, configserver.ErrNotFound) { - t.Errorf("InstanceIntentGet() error = %v, want ErrNotFound (write must be a no-op)", err) - } -} - // TestConfigServerCache_MalformedDatastoreName_ReadCallers verifies // InstanceIntentsList/InstanceIntentGet/InstanceIntentExists all surface // ErrMalformedDatastoreName as a normal returned error when the @@ -343,8 +330,11 @@ func TestConfigServerCache_InstanceLifecycle(t *testing.T) { // TestConfigServerCache_RunningIndependentOfSeam verifies InstanceRunningGet // / InstanceRunningModify work purely off the in-memory store, entirely -// independent of the LocalConfigReader seam (which never sees "running" at -// all under this backend). +// independent of the ConfigSnapshotClient port (which never sees "running" at +// all under this backend). InstanceRunningGet returns an +// importer.ImportConfigAdapter (Intent reads return IntentAdapter) rather +// than the raw *tree_persist.Intent, so the assertions go through its +// accessors. func TestConfigServerCache_RunningIndependentOfSeam(t *testing.T) { ctx := context.Background() c, reader := newTestConfigServerCache(t) @@ -369,7 +359,7 @@ func TestConfigServerCache_RunningIndependentOfSeam(t *testing.T) { t.Errorf("InstanceRunningGet().GetPriority() = %d, want %d", gotPriority, want.GetPriority()) } - // The seam was never seeded with anything and never asked for + // The port was never seeded with anything and never asked for // "running" — List/Get must still be untouched (fake has no docs at // all for this target). docs, err := reader.List(ctx, configserver.Target{Namespace: testNamespace, Name: testTarget}) @@ -377,7 +367,7 @@ func TestConfigServerCache_RunningIndependentOfSeam(t *testing.T) { t.Fatalf("reader.List() error = %v", err) } if len(docs) != 0 { - t.Errorf("reader.List() = %v, want empty (running never touches the seam)", docs) + t.Errorf("reader.List() = %v, want empty (running never touches the port)", docs) } } @@ -413,17 +403,113 @@ func TestConfigServerCache_InstanceRunningGet_UnknownInstance(t *testing.T) { } } -// TestConfigServerCache_ImplementsReaderCapabilities verifies -// *ConfigServerCache satisfies IntentReader, RunningStore, and -// InstanceLifecycle directly. It deliberately does not assert IntentWriter -// here — there is no method left on the type to test for no-op behavior; -// that behavior lives in noopIntentWriter, composed in at -// Server.createCacheClient instead. -func TestConfigServerCache_ImplementsReaderCapabilities(t *testing.T) { - c := NewConfigServerCache(configserver.NewFakeLocalConfigReader()) - var ( - _ IntentReader = c - _ RunningStore = c - _ InstanceLifecycle = c - ) +// TestConfigServerCache_ImplementsClient verifies *ConfigServerCache +// satisfies the full cache.Client directly — including IntentWriter, since +// Modify/Delete are real writes against the ConfigSnapshotClient port. +func TestConfigServerCache_ImplementsClient(t *testing.T) { + c := NewConfigServerCache(configserver.NewFakeConfigSnapshotClient()) + var _ Client = c +} + +// TestConfigServerCache_InstanceIntentModify_CreatesAndIsReadableBack +// covers the write-then-read round trip through the port: a modified +// Intent must be readable back via InstanceIntentGet with the content that +// was written, matching the ghost-intent regression's "last-applied +// updates at apply time" contract. +func TestConfigServerCache_InstanceIntentModify_CreatesAndIsReadableBack(t *testing.T) { + ctx := context.Background() + c, _ := newTestConfigServerCache(t) + + intent := &tree_persist.Intent{ + IntentName: testNamespace + ".intent1", + Priority: 5, + Root: &tree_persist.TreeElement{ + Childs: []*tree_persist.TreeElement{ + {Name: "hostname", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "router1"}})}, + }, + }, + } + + if err := c.InstanceIntentModify(ctx, testCacheName, intent); err != nil { + t.Fatalf("InstanceIntentModify() error = %v", err) + } + + adapter, err := c.InstanceIntentGet(ctx, testCacheName, testNamespace+".intent1") + if err != nil { + t.Fatalf("InstanceIntentGet() after Modify: %v", err) + } + if adapter.GetPriority() != 5 { + t.Errorf("GetPriority() = %d, want 5", adapter.GetPriority()) + } + if got := adapter.GetName(); got != testNamespace+".intent1" { + t.Errorf("GetName() = %q, want %q", got, testNamespace+".intent1") + } + hostname := adapter.GetElement("hostname") + if hostname == nil { + t.Fatal(`GetElement("hostname") = nil after Modify→Get, want tree content preserved`) + } + got, err := hostname.GetKeyValue(ctx, nil) + if err != nil { + t.Fatalf("hostname GetKeyValue: %v", err) + } + if got != "router1" { + t.Errorf("hostname = %q, want router1", got) + } +} + +// TestConfigServerCache_InstanceIntentDelete_RemovesFromSeam is the +// regression test for the ghost-intent bug this ticket exists to fix: +// deleting an intent must make it unreadable via the port immediately, not +// only after some later, unrelated reconcile. +func TestConfigServerCache_InstanceIntentDelete_RemovesFromSeam(t *testing.T) { + ctx := context.Background() + c, reader := newTestConfigServerCache(t) + target := configserver.Target{Namespace: testNamespace, Name: testTarget} + reader.Seed(target, &configserver.Document{Name: "intent1", Namespace: testNamespace}) + + if err := c.InstanceIntentDelete(ctx, testCacheName, testNamespace+".intent1", false); err != nil { + t.Fatalf("InstanceIntentDelete() error = %v", err) + } + + if _, err := c.InstanceIntentGet(ctx, testCacheName, testNamespace+".intent1"); !errors.Is(err, configserver.ErrNotFound) { + t.Errorf("InstanceIntentGet() after Delete: err = %v, want ErrNotFound", err) + } +} + +// TestConfigServerCache_InstanceIntentDelete_MissingIsNoop matches the +// ConfigSnapshotService.Delete contract: deleting an intent that was never +// present is a no-op success under this backend regardless of +// ignoreNonExisting — TargetSnapshot's membership model has no tombstone to +// distinguish "already gone" from "never existed". +func TestConfigServerCache_InstanceIntentDelete_MissingIsNoop(t *testing.T) { + ctx := context.Background() + c, _ := newTestConfigServerCache(t) + + if err := c.InstanceIntentDelete(ctx, testCacheName, "missing", false); err != nil { + t.Errorf("InstanceIntentDelete() of missing intent: %v, want no-op success", err) + } +} + +// TestConfigServerCache_InstanceIntentModify_MalformedDatastoreName and +// TestConfigServerCache_InstanceIntentDelete_MalformedDatastoreName lock the +// same ErrMalformedDatastoreName contract the read callers already have, +// for the write callers. +func TestConfigServerCache_InstanceIntentModify_MalformedDatastoreName(t *testing.T) { + ctx := context.Background() + c, _ := newTestConfigServerCache(t) + + err := c.InstanceIntentModify(ctx, "no-dot-here", &tree_persist.Intent{IntentName: "intent1"}) + if !errors.Is(err, ErrMalformedDatastoreName) { + t.Errorf("InstanceIntentModify() error = %v, want ErrMalformedDatastoreName", err) + } +} + +func TestConfigServerCache_InstanceIntentDelete_MalformedDatastoreName(t *testing.T) { + ctx := context.Background() + c, _ := newTestConfigServerCache(t) + + err := c.InstanceIntentDelete(ctx, "no-dot-here", "intent1", false) + if !errors.Is(err, ErrMalformedDatastoreName) { + t.Errorf("InstanceIntentDelete() error = %v, want ErrMalformedDatastoreName", err) + } } diff --git a/pkg/cache/configsnapshot/document.go b/pkg/cache/configsnapshot/document.go new file mode 100644 index 00000000..bc5c49d5 --- /dev/null +++ b/pkg/cache/configsnapshot/document.go @@ -0,0 +1,78 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package configsnapshot owns the last-applied Intent ↔ ConfigSnapshotService +// DTO ↔ ImportConfigAdapter interchange: the Document-shaped wire form plus +// flatten, blob merge, and NewImportAdapter. Document is a Go DTO name only +// — not a domain term (see pkg/cache/CONTEXT.md Last-applied). +// +// The remote Get/List/Modify/Delete port stays in pkg/cache/configserver; +// ConfigServerCache only orchestrates codec ↔ client. +package configsnapshot + +import sdcpb "github.com/sdcio/sdc-protos/sdcpb" + +// Target scopes a Document to a single southbound target, mirroring the +// TargetNamespaceKey/TargetNameKey labels config-server's controller indexes +// Config resources by. +type Target struct { + Namespace string + Name string +} + +// ConfigBlob is a single raw path + JSON value entry from a Document's +// config payload — shaped so it parses cleanly into the same form +// importer/json.JsonTreeImporter already consumes. +type ConfigBlob struct { + Path string + // Value is the raw JSON encoding of the value at Path. + Value []byte +} + +// Document is the ConfigSnapshotService interchange representation of one +// config-server Config (joined with its SensitiveConfig, if any). It carries +// exactly the fields needed to build an importer.ImportConfigAdapter: name, +// namespace, priority, non-revertive flag, orphan flag, sensitive paths, and +// the raw config payload. +type Document struct { + // Name is the Config resource's metadata.name — the lookup key + // ConfigSnapshotService uses against TargetSnapshot.Spec.Configs. + Name string + // Namespace is the Config's Kubernetes namespace, populated from + // ConfigEntry.Namespace. Together with Name it forms the owner string + // config-server uses on TransactionSet (config.GetGVKNSN). + Namespace string + Priority int32 + NonRevertive bool + Orphan bool + SensitivePaths []*sdcpb.Path + Config []*ConfigBlob +} + +// IntentName is the owner name data-server and config-server share for a +// Config: a Namespaced name ("."), matching +// config.GetGVKNSN. When Namespace is empty the bare Name is returned, so +// incomplete fixtures stay usable. +func (d *Document) IntentName() string { + return namespacedName(d.Namespace, d.Name) +} + +// namespacedName joins namespace and name with a single '.' (Namespaced name). +// An empty namespace returns the bare name. +func namespacedName(namespace, name string) string { + if namespace == "" { + return name + } + return namespace + "." + name +} diff --git a/pkg/cache/configsnapshot/document_export.go b/pkg/cache/configsnapshot/document_export.go new file mode 100644 index 00000000..a2093994 --- /dev/null +++ b/pkg/cache/configsnapshot/document_export.go @@ -0,0 +1,120 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package configsnapshot + +import ( + "encoding/json" + "fmt" + + "github.com/sdcio/data-server/pkg/utils" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "github.com/sdcio/sdc-protos/tree_persist" + "google.golang.org/protobuf/proto" +) + +// DocumentFromIntent flattens a tree_persist.Intent — the schema-typed tree +// ops.TreeExport produces for one owner's contribution at apply time — into +// the same flat Document shape ConfigSnapshotClient already returns for +// reads, so Modify can send it as a ConfigSnapshotService wire payload +// without a second, parallel field-mapping table. +// +// The whole tree collapses into a single ConfigBlob at path "/": each +// TreeElement becomes a JSON object key, with same-named siblings — the +// shape a list container's instances always take in the exported tree, +// since TreeExport names every list instance after its containing list, +// never its key value — grouped into a JSON array. mergeConfigBlobs's +// mergeRootBlob (the read-side counterpart) already treats path "/" as a +// shallow merge of a JSON object's top-level keys, which is exactly this +// shape. +func DocumentFromIntent(target Target, name string, intent *tree_persist.Intent) (*Document, error) { + value, err := treeElementToJSON(intent.GetRoot()) + if err != nil { + return nil, fmt.Errorf("flatten intent %q: %w", intent.GetIntentName(), err) + } + data, err := json.Marshal(value) + if err != nil { + return nil, fmt.Errorf("marshal intent %q: %w", intent.GetIntentName(), err) + } + + return &Document{ + Name: name, + Namespace: target.Namespace, + Priority: intent.GetPriority(), + NonRevertive: intent.GetNonRevertive(), + Orphan: intent.GetOrphan(), + SensitivePaths: intent.GetSensitivePaths(), + Config: []*ConfigBlob{{Path: "/", Value: data}}, + }, nil +} + +// treeElementToJSON renders el into the plain `any` shape encoding/json can +// marshal directly: a decoded scalar/array/object for a leaf (off its +// TypedValue), or a nested object for a container — see +// childrenToJSON for how same-named Childs (list instances) group into an +// array. +func treeElementToJSON(el *tree_persist.TreeElement) (any, error) { + if el == nil { + return nil, nil + } + if len(el.GetChilds()) > 0 { + return childrenToJSON(el.GetChilds()) + } + if len(el.GetLeafVariant()) == 0 { + // A container-shaped element with neither children nor a value of + // its own, e.g. an empty presence container. + return map[string]any{}, nil + } + + tv := &sdcpb.TypedValue{} + if err := proto.Unmarshal(el.GetLeafVariant(), tv); err != nil { + return nil, fmt.Errorf("unmarshal leaf %q: %w", el.GetName(), err) + } + return utils.GetJsonValue(tv, true) +} + +// childrenToJSON groups childs by Name into a JSON object, collecting +// same-named siblings into a JSON array under that key. TreeExport always +// names every instance of a list after the list itself (never the +// instance's key value), so grouping by Name is exactly how list instances +// recombine — no schema access needed to tell "this name is a list" from +// "this name is a plain container": JsonTreeImporter.GetElements already +// treats a bare object and a one-element array under the same key +// identically on the read side, so a single-instance list can stay +// ungrouped without losing information. +func childrenToJSON(childs []*tree_persist.TreeElement) (any, error) { + order := make([]string, 0, len(childs)) + byName := map[string][]any{} + for _, c := range childs { + v, err := treeElementToJSON(c) + if err != nil { + return nil, err + } + if _, seen := byName[c.GetName()]; !seen { + order = append(order, c.GetName()) + } + byName[c.GetName()] = append(byName[c.GetName()], v) + } + + result := make(map[string]any, len(order)) + for _, name := range order { + vals := byName[name] + if len(vals) > 1 { + result[name] = vals + } else { + result[name] = vals[0] + } + } + return result, nil +} diff --git a/pkg/cache/configsnapshot/document_export_test.go b/pkg/cache/configsnapshot/document_export_test.go new file mode 100644 index 00000000..5d2bd8e8 --- /dev/null +++ b/pkg/cache/configsnapshot/document_export_test.go @@ -0,0 +1,215 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package configsnapshot + +import ( + "encoding/json" + "testing" + + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "github.com/sdcio/sdc-protos/tree_persist" + "google.golang.org/protobuf/proto" +) + +// leafVariant marshals tv the same way TreeExport does (le.ValueAsBytes()), +// so fixtures build TreeElements the way a real export would. +func leafVariant(t *testing.T, tv *sdcpb.TypedValue) []byte { + t.Helper() + b, err := proto.Marshal(tv) + if err != nil { + t.Fatalf("marshal TypedValue: %v", err) + } + return b +} + +// TestDocumentFromIntent_singleLeaf covers the simplest case: one scalar +// leaf directly under the root, no containers, no lists. +func TestDocumentFromIntent_singleLeaf(t *testing.T) { + intent := &tree_persist.Intent{ + IntentName: "ns1.intent1", + Priority: 10, + Root: &tree_persist.TreeElement{ + Childs: []*tree_persist.TreeElement{ + {Name: "hostname", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "router1"}})}, + }, + }, + } + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + if doc.Name != "intent1" || doc.Namespace != "ns1" || doc.Priority != 10 { + t.Fatalf("doc = %+v", doc) + } + if len(doc.Config) != 1 || doc.Config[0].Path != "/" { + t.Fatalf("Config = %+v, want a single root blob", doc.Config) + } + + var got map[string]any + if err := json.Unmarshal(doc.Config[0].Value, &got); err != nil { + t.Fatalf("unmarshal root blob: %v", err) + } + if got["hostname"] != "router1" { + t.Errorf("root blob = %+v, want hostname=router1", got) + } +} + +// TestDocumentFromIntent_nestedContainer covers a leaf nested two levels +// deep under plain (non-list) containers. +func TestDocumentFromIntent_nestedContainer(t *testing.T) { + intent := &tree_persist.Intent{ + IntentName: "ns1.intent1", + Root: &tree_persist.TreeElement{ + Childs: []*tree_persist.TreeElement{ + {Name: "system", Childs: []*tree_persist.TreeElement{ + {Name: "config", Childs: []*tree_persist.TreeElement{ + {Name: "hostname", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "router1"}})}, + }}, + }}, + }, + }, + } + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + + var got map[string]any + if err := json.Unmarshal(doc.Config[0].Value, &got); err != nil { + t.Fatalf("unmarshal root blob: %v", err) + } + system, ok := got["system"].(map[string]any) + if !ok { + t.Fatalf("root blob = %+v, want a system container", got) + } + config, ok := system["config"].(map[string]any) + if !ok { + t.Fatalf("system = %+v, want a config container", system) + } + if config["hostname"] != "router1" { + t.Errorf("config = %+v, want hostname=router1", config) + } +} + +// TestDocumentFromIntent_listEntriesGroupIntoArray covers TreeExport's list +// encoding: repeated same-named siblings under a list container's parent +// must render as a JSON array, matching what mergeConfigBlobs/ +// JsonTreeImporter already expect on read. +func TestDocumentFromIntent_listEntriesGroupIntoArray(t *testing.T) { + mkInterface := func(name string, mtu int64) *tree_persist.TreeElement { + return &tree_persist.TreeElement{ + Name: "interface", + Childs: []*tree_persist.TreeElement{ + {Name: "name", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: name}})}, + {Name: "mtu", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_UintVal{UintVal: uint64(mtu)}})}, + }, + } + } + intent := &tree_persist.Intent{ + IntentName: "ns1.intent1", + Root: &tree_persist.TreeElement{ + Childs: []*tree_persist.TreeElement{ + mkInterface("eth0", 1500), + mkInterface("eth1", 9000), + }, + }, + } + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + + var got map[string]any + if err := json.Unmarshal(doc.Config[0].Value, &got); err != nil { + t.Fatalf("unmarshal root blob: %v", err) + } + ifaces, ok := got["interface"].([]any) + if !ok || len(ifaces) != 2 { + t.Fatalf("interface = %+v, want a 2-element array", got["interface"]) + } +} + +// TestDocumentFromIntent_singleListEntryStaysUngrouped covers the +// single-instance-list case: TreeExport gives no signal distinguishing a +// lone list entry from a plain container sharing that name, so it renders +// as a bare object — tolerated identically to a one-element array by +// JsonTreeImporter.GetElements on the read side. +func TestDocumentFromIntent_singleListEntryStaysUngrouped(t *testing.T) { + intent := &tree_persist.Intent{ + IntentName: "ns1.intent1", + Root: &tree_persist.TreeElement{ + Childs: []*tree_persist.TreeElement{ + {Name: "interface", Childs: []*tree_persist.TreeElement{ + {Name: "name", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "eth0"}})}, + }}, + }, + }, + } + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + + var got map[string]any + if err := json.Unmarshal(doc.Config[0].Value, &got); err != nil { + t.Fatalf("unmarshal root blob: %v", err) + } + if _, isObject := got["interface"].(map[string]any); !isObject { + t.Errorf("interface = %+v (%T), want a bare object for a single instance", got["interface"], got["interface"]) + } +} + +// TestDocumentFromIntent_fieldMapping locks the incidental-field mapping off +// the Intent itself, independent of tree content. +func TestDocumentFromIntent_fieldMapping(t *testing.T) { + intent := &tree_persist.Intent{ + IntentName: "ns1.intent1", + Priority: 7, + NonRevertive: true, + Orphan: true, + SensitivePaths: []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "secret"}}}}, + Root: &tree_persist.TreeElement{Childs: []*tree_persist.TreeElement{{Name: "x", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_BoolVal{BoolVal: true}})}}}, + } + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + if doc.Priority != 7 || !doc.NonRevertive || !doc.Orphan { + t.Errorf("doc = %+v", doc) + } + if len(doc.SensitivePaths) != 1 { + t.Errorf("SensitivePaths = %+v", doc.SensitivePaths) + } +} + +// TestDocumentFromIntent_nilRoot covers an Intent with no Root at all (e.g. +// an explicit-deletes-only export) — must not panic, and produces a Config +// list callers can still send onward without crashing. +func TestDocumentFromIntent_nilRoot(t *testing.T) { + intent := &tree_persist.Intent{IntentName: "ns1.intent1"} + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + if len(doc.Config) != 1 { + t.Fatalf("Config = %+v", doc.Config) + } +} diff --git a/pkg/cache/configserver/localread_test.go b/pkg/cache/configsnapshot/document_test.go similarity index 98% rename from pkg/cache/configserver/localread_test.go rename to pkg/cache/configsnapshot/document_test.go index 3b379460..9a0e695a 100644 --- a/pkg/cache/configserver/localread_test.go +++ b/pkg/cache/configsnapshot/document_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configserver +package configsnapshot import "testing" diff --git a/pkg/cache/configserver/importer.go b/pkg/cache/configsnapshot/importer.go similarity index 88% rename from pkg/cache/configserver/importer.go rename to pkg/cache/configsnapshot/importer.go index f591a6c0..7b9ab3f7 100644 --- a/pkg/cache/configserver/importer.go +++ b/pkg/cache/configsnapshot/importer.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configserver +package configsnapshot import ( "fmt" @@ -22,8 +22,8 @@ import ( sdcpb "github.com/sdcio/sdc-protos/sdcpb" ) -// documentImporter adapts a Document — the local-read seam's representation -// of a config-server Config (joined with its SensitiveConfig) — into an +// documentImporter adapts a Document — the ConfigSnapshotService DTO for one +// config-server Config (joined with its SensitiveConfig) — into an // importer.ImportConfigAdapter, per the ADR's field-mapping table. Traversal // of the merged config payload is delegated to // importer/json.JsonTreeImporter, which already knows how to walk the @@ -40,7 +40,7 @@ type documentImporter struct { func NewImportAdapter(doc *Document) (importer.IntentAdapter, error) { root, err := mergeConfigBlobs(doc.Config) if err != nil { - return nil, fmt.Errorf("configserver: building config for %q: %w", doc.Name, err) + return nil, fmt.Errorf("configsnapshot: building config for %q: %w", doc.Name, err) } return &documentImporter{ JsonTreeImporter: jsonimporter.NewJsonTreeImporter(root, doc.IntentName(), doc.Priority, doc.NonRevertive), diff --git a/pkg/cache/configserver/importer_test.go b/pkg/cache/configsnapshot/importer_test.go similarity index 99% rename from pkg/cache/configserver/importer_test.go rename to pkg/cache/configsnapshot/importer_test.go index 51a3a3fd..dadf09aa 100644 --- a/pkg/cache/configserver/importer_test.go +++ b/pkg/cache/configsnapshot/importer_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configserver +package configsnapshot import ( "testing" diff --git a/pkg/cache/configserver/merge.go b/pkg/cache/configsnapshot/merge.go similarity index 99% rename from pkg/cache/configserver/merge.go rename to pkg/cache/configsnapshot/merge.go index 59146dc1..277b13bd 100644 --- a/pkg/cache/configserver/merge.go +++ b/pkg/cache/configsnapshot/merge.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configserver +package configsnapshot import ( "encoding/json" diff --git a/pkg/cache/configserver/merge_test.go b/pkg/cache/configsnapshot/merge_test.go similarity index 99% rename from pkg/cache/configserver/merge_test.go rename to pkg/cache/configsnapshot/merge_test.go index be0dea2f..dfca28e5 100644 --- a/pkg/cache/configserver/merge_test.go +++ b/pkg/cache/configsnapshot/merge_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configserver +package configsnapshot import ( "testing" diff --git a/pkg/cache/configsnapshot/round_trip_test.go b/pkg/cache/configsnapshot/round_trip_test.go new file mode 100644 index 00000000..273f7089 --- /dev/null +++ b/pkg/cache/configsnapshot/round_trip_test.go @@ -0,0 +1,112 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package configsnapshot + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "github.com/sdcio/sdc-protos/tree_persist" + "google.golang.org/protobuf/testing/protocmp" +) + +// TestRoundTrip_IntentDocumentAdapter is the primary seam for this package: +// Intent → Document → ImportConfigAdapter must preserve metadata and tree +// content that DocumentFromIntent and NewImportAdapter share. +func TestRoundTrip_IntentDocumentAdapter(t *testing.T) { + sensitive := []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "secret"}}}} + intent := &tree_persist.Intent{ + IntentName: "ns1.intent1", + Priority: 7, + NonRevertive: true, + Orphan: true, + SensitivePaths: sensitive, + Root: &tree_persist.TreeElement{ + Childs: []*tree_persist.TreeElement{ + {Name: "system", Childs: []*tree_persist.TreeElement{ + {Name: "config", Childs: []*tree_persist.TreeElement{ + {Name: "hostname", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "router1"}})}, + }}, + }}, + {Name: "interface", Childs: []*tree_persist.TreeElement{ + {Name: "name", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "eth0"}})}, + {Name: "mtu", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_UintVal{UintVal: 9000}})}, + }}, + {Name: "interface", Childs: []*tree_persist.TreeElement{ + {Name: "name", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "eth1"}})}, + {Name: "mtu", LeafVariant: leafVariant(t, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_UintVal{UintVal: 1500}})}, + }}, + }, + }, + } + + doc, err := DocumentFromIntent(Target{Namespace: "ns1", Name: "target1"}, "intent1", intent) + if err != nil { + t.Fatalf("DocumentFromIntent: %v", err) + } + + adapter, err := NewImportAdapter(doc) + if err != nil { + t.Fatalf("NewImportAdapter: %v", err) + } + + if got := adapter.GetName(); got != "ns1.intent1" { + t.Errorf("GetName() = %q, want ns1.intent1", got) + } + if got := adapter.GetPriority(); got != 7 { + t.Errorf("GetPriority() = %d, want 7", got) + } + if !adapter.GetNonRevertive() { + t.Error("GetNonRevertive() = false, want true") + } + if !adapter.GetOrphan() { + t.Error("GetOrphan() = false, want true") + } + if diff := cmp.Diff(sensitive, adapter.GetSensitivePaths(), protocmp.Transform()); diff != "" { + t.Errorf("GetSensitivePaths() mismatch (-want +got):\n%s", diff) + } + + system := adapter.GetElement("system") + if system == nil { + t.Fatal(`GetElement("system") = nil`) + } + config := system.GetElement("config") + if config == nil { + t.Fatal(`GetElement("system").GetElement("config") = nil`) + } + if config.GetElement("hostname") == nil { + t.Fatal(`GetElement("hostname") = nil`) + } + hostnameVal, err := config.GetElement("hostname").GetKeyValue(t.Context(), nil) + if err != nil { + t.Fatalf("hostname GetKeyValue: %v", err) + } + if hostnameVal != "router1" { + t.Errorf("hostname = %q, want router1", hostnameVal) + } + + // List instances expand at the parent GetElements() level (same-named + // siblings under one JSON array key), not as children of GetElement("interface"). + var ifaces int + for _, e := range adapter.GetElements() { + if e.GetName() == "interface" { + ifaces++ + } + } + if ifaces != 2 { + t.Fatalf("interface entries via GetElements() = %d, want 2", ifaces) + } +} diff --git a/pkg/cache/docs/adr/0001-config-server-backed-cache-client.md b/pkg/cache/docs/adr/0001-config-server-backed-cache-client.md index 2406eab8..92795c6b 100644 --- a/pkg/cache/docs/adr/0001-config-server-backed-cache-client.md +++ b/pkg/cache/docs/adr/0001-config-server-backed-cache-client.md @@ -1,14 +1,18 @@ # A config-server-backed `cache.Client` reads real Intents from the colocated `controller`'s cache instead of persisting a local copy +**Status:** accepted + +**Superseded in part by:** [ADR 0003](0003-config-server-write-path-real-last-applied-writes.md) — `InstanceIntentModify`/`InstanceIntentDelete` are no longer unconditional silent no-ops under `Cache.Type: config-server`; they perform real last-applied writes at apply time. The read seam, `"running"` split, and field-mapping table below still stand. The "Scope note" about no local read API also no longer applies: `ConfigSnapshotService` exists and is wired via `GRPCConfigClient`. + data-server gains a second `cache.Client` implementation, selected by `Cache.Type: config-server` in the existing `Server.createCacheClient` switch (`pkg/server/cache.go`), that serves real Intent documents by calling a new local unary Get/List surface on the config-server `controller` container colocated 1:1 with data-server in the StatefulSet — instead of persisting its own copy of intent content the way `LocalCache` does today. `"running"` (the synced device-state pseudo-intent) is unaffected by backend choice: it is split out onto its own accessor and always kept as a minimal in-memory, per-instance store, because it was never config-server's data to begin with. `Cache.Type: local` keeps working completely unmodified — this is an additive backend, not a replacement, and every interface change below is a widening (new methods, or a return-type generalization that `LocalCache` already satisfies trivially) rather than a breaking one for the existing path. ## Why -Today `CacheClientBoundImpl` (`pkg/cache/cacheClientBound.go`) is a thin, backend-agnostic forwarder over a single package-wide `cache.Client`, and `LocalCache` (`pkg/cache/local.go`) is that interface's only implementation: a disk-backed store that treats every intent name — including `"running"` — identically. That homogeneity was a coincidence of `LocalCache` being the only backend, not a real unification: config-server is already the sole northbound writer of real Intents (`docs/architecture.md` §2), and is itself backed by a centralized, watch-synced store (a separate aggregated API server, not the colocated `controller` — see the research below). Persisting a second copy of that same data in `LocalCache` is redundant duplication with its own consistency problems; the colocated `controller`'s existing informer cache can serve reads locally instead. +Today `CacheClientBoundImpl` (`pkg/cache/cacheClientBound.go`) is a thin, backend-agnostic forwarder over a single package-wide `cache.Client`, and `LocalCache` (`pkg/cache/local.go`) is that interface's only implementation: a disk-backed store that treats every intent name — including `"running"` — identically. That homogeneity was a coincidence of `LocalCache` being the only backend, not a real unification: config-server is already the sole northbound writer of real Intents, and is itself backed by a centralized, watch-synced store (a separate aggregated API server, not the colocated `controller` — see the research below). Persisting a second copy of that same data in `LocalCache` is redundant duplication with its own consistency problems; the colocated `controller`'s existing informer cache can serve reads locally instead. -`"running"` is structurally different: it is produced and consumed entirely in-process (`ops.TreeExport` / `treeproto.NewProtoTreeImporter`), nothing reloads it from a cache on startup, and `docs/architecture.md`'s cold-start story already puts recovery on the control plane rather than on durable local state. It has no reason to route through config-server at all, and no reason to be durable under this backend either. +`"running"` is structurally different: it is produced and consumed entirely in-process (`ops.TreeExport` / `treeproto.NewProtoTreeImporter`), nothing reloads it from a cache on startup, and cold-start recovery is expected from the control plane rather than from durable local state. It has no reason to route through config-server at all, and no reason to be durable under this backend either. ## Scope note: this ADR documents a contract, not a config-server implementation diff --git a/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md b/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md new file mode 100644 index 00000000..956ff412 --- /dev/null +++ b/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md @@ -0,0 +1,20 @@ +# Segregate `cache.Client` by capability instead of by backend + +**Status:** accepted + +**Superseded in part by:** [ADR 0003](0003-config-server-write-path-real-last-applied-writes.md) — `ConfigServerCache` no longer composes a no-op `IntentWriter`; it implements `IntentWriter` with real last-applied writes. The capability split itself still stands. Also [ADR 0004](0004-configsnapshot-owns-last-applied-interchange.md) — last-applied Intent↔DTO↔ImportConfigAdapter interchange moves to `pkg/cache/configsnapshot`; the Consequences relocate of `documentImporter`/`mergeConfigBlobs` into `pkg/tree/importer/configserver/` no longer stands. + +ADR 0001 defined `cache.Client`/`CacheClientBound` as one flat interface (13 methods) covering instance lifecycle, real-Intent reads, real-Intent writes, and `running` reads/writes. Landing `ConfigServerCache` against that shape exposed two things a flat interface can't express: `InstanceIntentModify`/`InstanceIntentDelete` are real operations under `Cache.Type: local` and unconditional no-ops under `Cache.Type: config-server`, silently, with nothing in the type system distinguishing the two; and `ConfigServerCache`'s instance-lifecycle methods (`InstanceCreate`/`Delete`/`Close`/`Exists`/`InstancesList`) turned out to govern only its in-memory `running` store, with zero relationship to real-Intent reads under that backend — unlike `LocalCache`, where lifecycle governs the one shared disk store underlying everything. + +We split `Client` (and `CacheClientBound`) into four capability interfaces — `IntentReader`, `IntentWriter`, `RunningStore`, `InstanceLifecycle` — composed back into `Client`/`CacheClientBound` for callers that need the full set (`Datastore`, `Server.createCacheClient`). Callers that only read real Intents (`forEachIntent`, `populateSensitivePathIndex`) narrow their parameter type to `IntentReader` alone. `ConfigServerCache` implements `IntentReader` + `RunningStore` + `InstanceLifecycle` directly; a new generic `noopIntentWriter` (`pkg/cache`) supplies `IntentWriter` for it, composed at the one legitimate type-switch point (`Server.createCacheClient`), rather than living as no-op-shaped methods inside `ConfigServerCache` itself. + +## Considered options + +- **Scope the no-op `IntentWriter` to `pkg/cache/configserver` instead of generic `pkg/cache`.** Rejected: kept generic despite only one current consumer — "a backend that doesn't own writes" is judged reusable enough to name once, ahead of a second need, even though this is a deliberate departure from "two adapters justify a seam." +- **Fold instance lifecycle into `RunningStore`**, since for `ConfigServerCache` that's literally what it manages. Rejected: keeps every interface name matching exactly one concept, and documents `ConfigServerCache`'s asymmetry (lifecycle ≠ "config-server has data for this target") via an explicit doc comment rather than folding it into a same-named-but-different-meaning method set. +- **Extract a standalone `DocumentTreeImporter` type** for config-server's `ImportConfigAdapter` implementation, instead of relocating `documentImporter`/`mergeConfigBlobs` as-is. Deferred: exactly one call site exists today; a new named type wouldn't concentrate anything not already concentrated in one package. Revisit if a second config-server-shaped document source appears. **Revisited in ADR 0004:** still no named type; the halves are colocated as package functions in `pkg/cache/configsnapshot` instead (ADR 0003 made the write half real, so locality matters). +- **Amend ADR 0001 in place** instead of writing this ADR. Rejected: 0001 is a record of reasoning that already shipped (specific commits implement its exact interface); rewriting it to describe a different shape would falsify the history. + +## Consequences + +`Cache.Type: local` requires no behavior change — `LocalCache` already trivially satisfies all four interfaces via its existing methods. `ConfigServerCache`'s `IntentWriter`-shaped no-op becomes a visible, named composition step in `createCacheClient` instead of implicit backend knowledge living inside the backend's own method bodies. Tests for read-only call paths (`forEachIntent`, `populateSensitivePathIndex`, `GetIntent`) can mock the 4-method `IntentReader` instead of the full `Client`/`CacheClientBound`. `documentImporter`/`mergeConfigBlobs` relocate from `pkg/cache/configserver/` to `pkg/tree/importer/configserver/`, alongside their sibling `proto`/`json`/`xml` importers; `pkg/cache/configserver/` keeps only the remote port (`client.go`, `grpc.go`, `fake.go` — the Go port is `ConfigSnapshotClient`). **Superseded by ADR 0004** for that relocate — interchange lives in `pkg/cache/configsnapshot`; `pkg/tree/importer/configserver` is removed. diff --git a/pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md b/pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md new file mode 100644 index 00000000..58122d27 --- /dev/null +++ b/pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md @@ -0,0 +1,48 @@ +# Under `Cache.Type: config-server`, `IntentModify`/`IntentDelete` become real writes at apply time — superseding ADR 0001's no-op clause + +**Supersedes:** [ADR 0001](0001-config-server-backed-cache-client.md)'s decision that `InstanceIntentModify`/`InstanceIntentDelete` are "unconditional silent no-ops" under `Cache.Type: config-server`. Every other part of ADR 0001 (the read seam, `"running"` split, field mapping) stands unmodified. + +**Pairs with:** config-server `.scratch/last-applied-snapshot-write-at-apply/spec.md` (execution plan / implementation decisions for the config-server side of this same fix). + +## Why ADR 0001's no-op clause was wrong + +ADR 0001 reasoned that "config-server/kube-api is the sole writer" for real Intents, so a call to `IntentModify`/`IntentDelete` under this backend had "nothing actionable to do." That's true for *authorship* (who decides an Intent's desired content) but conflates it with *last-applied tracking* (what this datastore actually pushed south) — a second, distinct piece of state that `Cache.Type: local` has always updated at the exact moment of southbound apply, inside `lowlevelTransactionSet`'s apply loop, never gated on anything downstream. + +Under `Cache.Type: config-server`, that second write was dropped on the floor. Config-server's own last-applied record (`TargetSnapshot.Spec.Configs`) was written only after `TransactionConfirm`, via `saveSnapshot` — a best-effort, non-fatal, unrelated-process write that could lag apply by an unbounded amount (a full reconcile round-trip, or longer if `saveSnapshot` failed and only retried on the next unrelated reconcile). CI caught the resulting race concretely: a deleted `intent1` remained "last-applied" in `TargetSnapshot` past its actual southbound deletion, got rehydrated by the next `LoadAllButRunningIntents`, and failed leafref validation during a *different*, unrelated `customer` ConfigSet delete — hanging that teardown for the CI timeout. The same lag independently produces false deviations and incorrect apply-vs-confirm-window reads for `BlameConfig`/`GetIntent`/revert, and risks replaying already-deleted config after a crash-restart (recovery replays only intents still present in the last-applied record). + +An earlier, separate design pass (`target-snapshot-backed-config-read` spec, which repointed `ConfigReadService` reads to `TargetSnapshot`) had concluded a stale snapshot was harmless — "worst case a redundant re-push." **That conclusion is retracted.** It was reached before this bug's evidence existed; a stale-behind last-applied record can rehydrate genuinely deleted config into the validation tree, which is a correctness failure, not a harmless no-op. + +## The fix + +`Cache.Type: config-server`'s `IntentWriter` stops being `noopIntentWriter`. `IntentModify`/`IntentDelete`, called from the same apply-loop call sites `Cache.Type: local` already uses, now perform a real write — synchronously, inside `TransactionSet`, before the response reaches the caller: + +- **`IntentModify`** upserts the applied, encrypted payload for that Intent name into config-server's last-applied record. +- **`IntentDelete`** removes that Intent's entry from the same record. +- **Membership rule:** an Intent is last-applied **iff its entry is present**. Delete removes the entry outright — no tombstone, deleted-annotation, or deleted-label scheme. Simpler mental model, no second "is this really gone" check for every reader (`ConfigReadService`, `LoadAllButRunningIntents`, recovery) to carry, and it matches `Cache.Type: local`'s existing all-or-nothing disk-key semantics exactly. +- **Rollback restores it.** Timeout/cancel rollback re-runs `TransactionSet` on the old intents; those `IntentModify` calls restore any entry a rollback needs to bring back. Last-applied tracking is symmetric with device-state rollback — it was never optional just because the record now lives in config-server instead of on local disk. +- **A failed delete-write must hard-fail the transaction**, matching `IntentModify`'s existing behavior. The pre-fix code left `IntentDelete` failures log-only; keeping that asymmetry here would mean a failed write leaves a silent ghost entry — the exact bug this ADR fixes, just relocated one layer down into "the write path's own failure mode." If the write can silently fail, "last-applied only changes at apply time" isn't actually true. + +Config-server's post-`TransactionConfirm` `saveSnapshot` is **demoted to a backstop**: it may prune entries whose upstream config no longer exists, and refresh incidental metadata (schema hashes), but it is no longer the authority for membership, must never run on a failed/rolled-back transact, and must never full-replace the record in a way that clobbers what the apply-time write (or a rollback) just wrote. See the config-server-side spec for the mechanics (a targeted per-key patch, not get-then-replace, so the apply-time write and the backstop prune can't stomp on each other). + +## Wire contract: one merged service, not read/write split + +The original local RPC surface (`ConfigReadService`, `sdc-protos/config_read.proto`) was deliberately read-only, because ADR 0001 assumed writes would always be no-ops. That assumption no longer holds, and two options were considered for exposing the new write capability: + +1. **A new sibling service** (e.g. a `ConfigSnapshotWriteService` next to the existing `ConfigReadService`), keeping "read" and "write" as separately named, separately registered gRPC services. +2. **One merged service** covering `Get`/`List`/`Modify`/`Delete`, replacing `ConfigReadService` entirely (renamed, since it's no longer read-only). + +**Decision: merge (option 2).** This is a single colocated, single-consumer, localhost-bound sidecar API — one pod, one trust boundary, one backing resource (`TargetSnapshot`), one client (`ConfigServerCache`). There's no multi-tenancy, independent versioning, or security-boundary reason for two services here; two services would just mean two proto files, two server registrations, and two client dials for what is fundamentally one capability surface over one resource. Splitting by capability is still valuable — it's just enforced one layer up, in data-server's Go `cache.Client` interface (`IntentReader`/`IntentWriter`, already segregated per the capability-split refactor this backend's `Client` composition depends on), not in the wire proto. A single generated gRPC client structurally satisfies both narrower Go interfaces regardless of how many RPCs live on one proto service, so merging the wire surface costs nothing at the Go layer that actually needs the segregation for testing/mocking. + +Keeping the old name while adding `Modify`/`Delete` to it was explicitly rejected: `ConfigReadService`'s own doc comment says "a unary, localhost-bound **read** API" — leaving that name on a service that now writes would be the same stale-name-vs-behavior mismatch this ADR spends most of its text fixing for "confirmed" vs "last-applied." The renamed, merged service is `ConfigSnapshotService` (`Get`/`List`/`Modify`/`Delete`), matching the `TargetSnapshot` resource it fronts and the `last-applied` vocabulary this ADR establishes. + +### RPC shape + +- **Per-intent, not batched.** One `Modify`/`Delete` call per intent, matching today's apply-loop call shape exactly (one call site, no new batching/aggregation logic to design or test). Batching per-`TransactionSet` is a legitimate future optimization if per-intent round-trip latency ever matters, not a blocker here. +- **Payload crosses the wire, not a re-read signal.** `IntentModify`'s RPC carries the actual applied payload (the same `protoIntent` `ops.TreeExport` already built for this Intent) — config-server stores that content directly, filling incidental fields (`Revertive`/`Lifecycle`) from its own current `SensitiveConfig`. The alternative (data-server sends only "Intent X applied," config-server re-reads its current desired `SensitiveConfig` to build the record) was rejected: it reintroduces the exact live-vs-last-applied conflation this whole fix exists to close, since config-server's "current" desired value can already differ from what was actually pushed by the time the RPC arrives. + +## Consequences + +- `ConfigServerCache` implements `IntentWriter` directly (real `Modify`/`Delete` through the local seam). The earlier `noopIntentWriter` composition for this backend was removed; a future genuinely read-only backend would introduce its own no-op at `Server.createCacheClient` if needed. +- Every consumer of last-applied state (`LoadAllButRunningIntents`, `calculateDeviations`, `BlameConfig`, `GetIntent`, `performRevert`, crash recovery) gets correct apply-time semantics with no call-site changes — they already read through `Client.InstanceIntent*`/`ConfigReadService`; only what's behind that read changed. +- `pkg/cache/CONTEXT.md` gains the **Last-applied** term, distinct from **Intent** (desired/authored). See that file for the full definition. +- This does not reopen ADR 0001's read-path decisions (the `ImportConfigAdapter` field mapping, the `"running"` split, `ExplicitDeletes`'s documented always-empty limitation) — those stand. diff --git a/pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md b/pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md new file mode 100644 index 00000000..ab2536c9 --- /dev/null +++ b/pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md @@ -0,0 +1,23 @@ +# Extract last-applied interchange into `pkg/cache/configsnapshot` + +**Status:** accepted + +**Supersedes in part:** [ADR 0002](0002-segregate-cache-client-by-capability.md)'s Consequences relocate of `documentImporter`/`mergeConfigBlobs` into `pkg/tree/importer/configserver/`, and revisits that ADR's deferred standalone `DocumentTreeImporter` type. Capability segregation and the rest of 0002 stand unmodified. + +**Pairs with:** `.scratch/open/01-own-document-round-trip.md` (implementation ticket). + +ADR 0003 made last-applied writes real under `Cache.Type: config-server`. The write path flattens `tree_persist.Intent` into the ConfigSnapshotService DTO (`Document` — a Go type name only, not a domain term; see `CONTEXT.md` Last-applied), and the read path merges that DTO back into an `ImportConfigAdapter`. Those two halves of one interchange lived in different packages (`pkg/cache/configserver` vs `pkg/tree/importer/configserver`), with a package cycle on the DTO and no single place to ask “what does a Modify round-trip look like?” + +**Decision:** own the interchange in `pkg/cache/configsnapshot` — `Target`, `Document`, `ConfigBlob`, `DocumentFromIntent`, blob merge, and `NewImportAdapter` as package functions. `pkg/cache/configserver` keeps only the remote port (`ConfigSnapshotClient` / gRPC / fake) and imports the DTO from `configsnapshot`. `ConfigServerCache` only orchestrates (codec ↔ client). Delete `pkg/tree/importer/configserver` (no re-export shim). Prefer round-trip tests in `configsnapshot`, plus a Fake Modify→Get that asserts tree content. + +## Considered options + +- **Keep halves split; add a cross-package round-trip test only.** Rejected: fixes coverage, not locality or the DTO cycle. +- **Expand `pkg/tree/importer/configserver`** (move DTO + flatten in). Rejected: ConfigSnapshotService-shaped types are not tree importers; would still couple the port package oddly. +- **Expand `pkg/cache/configserver`** (move merge/adapter in). Rejected: bloated the remote-port package and reversed 0002's “importers with importers” for the wrong reason — the codec is not the port. +- **Named codec / `DocumentTreeImporter` type.** Rejected for now (same reason 0002 deferred it): one call site, no behavior to vary; package functions suffice. Revisit if a second Document-shaped source appears. +- **Amend ADR 0002 in place.** Rejected: 0002 records a relocate that shipped; a new ADR records why that package home is superseded without falsifying history. + +## Consequences + +Dependency direction becomes one-way: `configserver` (port) → `configsnapshot` → `tree/importer` (+ json). Ticket 03 (splitting Orphan/SensitivePaths off `ImportConfigAdapter`) should land after this extract so the metadata shrink hits one package. diff --git a/pkg/cache/noop_intent_writer.go b/pkg/cache/noop_intent_writer.go deleted file mode 100644 index b5933f49..00000000 --- a/pkg/cache/noop_intent_writer.go +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2024 Nokia -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package cache - -import ( - "context" - - "github.com/sdcio/sdc-protos/tree_persist" -) - -// noopIntentWriter is an IntentWriter whose modify and delete always succeed. -// Read-only backends compose it when another system owns real-Intent writes. -type noopIntentWriter struct{} - -func (noopIntentWriter) InstanceIntentModify(ctx context.Context, cacheName string, intent *tree_persist.Intent) error { - return nil -} - -func (noopIntentWriter) InstanceIntentDelete(ctx context.Context, cacheName string, intentName string, IgnoreNonExisting bool) error { - return nil -} diff --git a/pkg/cache/noop_intent_writer_test.go b/pkg/cache/noop_intent_writer_test.go deleted file mode 100644 index be247bee..00000000 --- a/pkg/cache/noop_intent_writer_test.go +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2024 Nokia -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package cache - -import ( - "context" - "testing" - - "github.com/sdcio/sdc-protos/tree_persist" -) - -var _ IntentWriter = noopIntentWriter{} - -// TestNoopIntentWriter_AlwaysNil: modify and delete succeed for any input. -func TestNoopIntentWriter_AlwaysNil(t *testing.T) { - w := noopIntentWriter{} - ctx := context.Background() - - if err := w.InstanceIntentModify(ctx, "any-cache", &tree_persist.Intent{IntentName: "any-intent"}); err != nil { - t.Errorf("InstanceIntentModify() error = %v, want nil", err) - } - if err := w.InstanceIntentDelete(ctx, "any-cache", "any-intent", false); err != nil { - t.Errorf("InstanceIntentDelete() error = %v, want nil", err) - } -} diff --git a/pkg/config/datastore.go b/pkg/config/datastore.go index 72b67e35..4cf7a49d 100644 --- a/pkg/config/datastore.go +++ b/pkg/config/datastore.go @@ -158,7 +158,7 @@ type CacheConfig struct { Dir string `yaml:"dir,omitempty" json:"dir,omitempty"` // Remote cache attr; also doubles as the config-server cache attr: // address of the colocated config-server controller's local - // ConfigReadService (see the ADR's "New cache.Client implementation" + // ConfigSnapshotService (see the ADR's "New cache.Client implementation" // section). Address string `yaml:"address,omitempty" json:"address,omitempty"` } diff --git a/pkg/datastore/transaction_rpc.go b/pkg/datastore/transaction_rpc.go index 787a3158..368fc885 100644 --- a/pkg/datastore/transaction_rpc.go +++ b/pkg/datastore/transaction_rpc.go @@ -390,7 +390,7 @@ func (d *Datastore) lowlevelTransactionSet(ctx context.Context, transaction *typ case errors.Is(err, ops.ErrorIntentNotPresent): err = d.cacheClient.IntentDelete(ctx, intent.GetName(), intent.GetDeleteIgnoreNonExisting()) if err != nil { - log.Error(err, "failed deleting intent from store") + return nil, fmt.Errorf("failed deleting intent %q from store for %s: %w", intent.GetName(), d.Name(), err) } d.sensitivePathIndex.Delete(intent.GetName()) log.V(logger.VDebug).Info("delete intent from cache") diff --git a/pkg/datastore/transaction_rpc_test.go b/pkg/datastore/transaction_rpc_test.go index 6009f763..4a93370b 100644 --- a/pkg/datastore/transaction_rpc_test.go +++ b/pkg/datastore/transaction_rpc_test.go @@ -13,6 +13,8 @@ import ( "github.com/openconfig/ygot/ygot" "github.com/sdcio/data-server/mocks/mockcacheclient" "github.com/sdcio/data-server/mocks/mocktarget" + "github.com/sdcio/data-server/pkg/cache" + "github.com/sdcio/data-server/pkg/cache/configserver" "github.com/sdcio/data-server/pkg/config" schemaClient "github.com/sdcio/data-server/pkg/datastore/clients/schema" "github.com/sdcio/data-server/pkg/datastore/types" @@ -30,6 +32,7 @@ import ( sdcpb "github.com/sdcio/sdc-protos/sdcpb" "github.com/sdcio/sdc-protos/tree_persist" "go.uber.org/mock/gomock" + "google.golang.org/protobuf/proto" ) func TestTransactionSet_PreviouslyApplied(t *testing.T) { @@ -512,6 +515,339 @@ func pathSliceToXPaths(paths []*sdcpb.Path) []string { return out } +// TestTransactionSet_IntentDeleteFailureHardFailsTransaction is the +// regression test for the asymmetry ADR 0003 fixes: a failed IntentDelete +// RPC must hard-fail the transaction the same way a failed IntentModify +// already does, rather than being logged and swallowed — a swallowed +// failure here leaves exactly the silent ghost last-applied entry this +// whole fix exists to close. +func TestTransactionSet_IntentDeleteFailureHardFailsTransaction(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + fixtureDevice := &sdcio_schema.Device{ + Interface: map[string]*sdcio_schema.SdcioModel_Interface{ + "ethernet-1/1": { + Name: ygot.String("ethernet-1/1"), + Description: ygot.String("existing"), + }, + }, + } + fixtureIntent := buildFixtureIntent(t, scb, "intent1", 10, fixtureDevice) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + wantErr := errors.New("delete rpc failed") + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + ccb.EXPECT(). + IntentGetAll(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, _ []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error) { + intentChan <- treeproto.NewProtoTreeImporter(fixtureIntent) + close(intentChan) + close(errChan) + }).AnyTimes() + ccb.EXPECT().IntentDelete(gomock.Any(), "intent1", gomock.Any()).Return(wantErr) + + sbi := mocktarget.NewMockTarget(ctrl) + sbi.EXPECT().Set(gomock.Any(), gomock.Any()).Return(&sdcpb.SetDataResponse{}, nil).AnyTimes() + + vpf := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)) + tc := tree.NewTreeContext(scb, vpf) + syncTreeRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + if err := syncTreeRoot.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + ds := &Datastore{ + config: &config.DatastoreConfig{ + Validation: config.NewValidationConfig(), + Name: "test-ds", + }, + syncTreeMutex: &sync.RWMutex{}, + syncTree: syncTreeRoot, + taskPool: vpf, + cacheClient: ccb, + sbi: sbi, + dmutex: &sync.Mutex{}, + schemaClient: scb, + sensitivePathIndex: sensitive.NewPathIndex(), + } + ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds)) + + ti := types.NewTransactionIntent("intent1", 10) + ti.SetDeleteFlag() + + _, err = ds.TransactionSet(ctx, "txn-delete-fail", []*types.TransactionIntent{ti}, nil, 10*time.Second, false) + if !errors.Is(err, wantErr) { + t.Fatalf("TransactionSet() error = %v, want wrapped %v", err, wantErr) + } +} + +// TestTransactionRollback_RestoresDeletedIntent verifies a rollback (the +// same path a timeout/cancel drives via TransactionManager.Cancel) re-runs +// TransactionSet on the transaction's old intents, and that a deleted +// intent's IntentModify call during that replay carries the pre-delete +// content back — the "rollback restores it" half of ADR 0003, symmetric +// with device-state rollback. +func TestTransactionRollback_RestoresDeletedIntent(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + fixtureDevice := &sdcio_schema.Device{ + Interface: map[string]*sdcio_schema.SdcioModel_Interface{ + "ethernet-1/1": { + Name: ygot.String("ethernet-1/1"), + Description: ygot.String("existing"), + }, + }, + } + fixtureIntent := buildFixtureIntent(t, scb, "intent1", 10, fixtureDevice) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + var deleteCalls int + var modifyCalls []*tree_persist.Intent + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + ccb.EXPECT(). + IntentGetAll(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, _ []string, intentChan chan<- importer.IntentAdapter, errChan chan<- error) { + intentChan <- treeproto.NewProtoTreeImporter(fixtureIntent) + close(intentChan) + close(errChan) + }).AnyTimes() + ccb.EXPECT(). + IntentDelete(gomock.Any(), "intent1", gomock.Any()). + DoAndReturn(func(context.Context, string, bool) error { + deleteCalls++ + return nil + }).AnyTimes() + ccb.EXPECT(). + IntentModify(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, intent *tree_persist.Intent) error { + modifyCalls = append(modifyCalls, intent) + return nil + }).AnyTimes() + ccb.EXPECT().RunningModify(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + + sbi := mocktarget.NewMockTarget(ctrl) + sbi.EXPECT().Set(gomock.Any(), gomock.Any()).Return(&sdcpb.SetDataResponse{}, nil).AnyTimes() + + vpf := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)) + tc := tree.NewTreeContext(scb, vpf) + syncTreeRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + if err := syncTreeRoot.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + ds := &Datastore{ + config: &config.DatastoreConfig{ + Validation: config.NewValidationConfig(), + Name: "test-ds", + }, + syncTreeMutex: &sync.RWMutex{}, + syncTree: syncTreeRoot, + taskPool: vpf, + cacheClient: ccb, + sbi: sbi, + dmutex: &sync.Mutex{}, + schemaClient: scb, + sensitivePathIndex: sensitive.NewPathIndex(), + } + ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds)) + + ti := types.NewTransactionIntent("intent1", 10) + ti.SetDeleteFlag() + + _, err = ds.TransactionSet(ctx, "txn-rollback", []*types.TransactionIntent{ti}, nil, 10*time.Second, false) + if err != nil { + t.Fatalf("TransactionSet() (delete) error = %v", err) + } + if deleteCalls != 1 { + t.Fatalf("IntentDelete call count = %d, want 1", deleteCalls) + } + if len(modifyCalls) != 0 { + t.Fatalf("IntentModify call count after delete = %d, want 0", len(modifyCalls)) + } + + if err := ds.transactionManager.Cancel(ctx, "txn-rollback"); err != nil { + t.Fatalf("Cancel() (rollback) error = %v", err) + } + + if len(modifyCalls) != 1 { + t.Fatalf("IntentModify call count after rollback = %d, want 1 (restore)", len(modifyCalls)) + } + restored := modifyCalls[0] + if got := restored.GetIntentName(); got != "intent1" { + t.Errorf("restored intent name = %q, want %q", got, "intent1") + } + if got := findLeafStringValue(t, restored.GetRoot(), "description"); got != "existing" { + t.Errorf("restored intent's description leaf = %q, want %q (the pre-delete content, not just the name)", got, "existing") + } +} + +// findLeafStringValue depth-first searches el for a leaf named name and +// returns its decoded string value, so rollback/restore tests can assert on +// actual content rather than just the intent's name/priority. +func findLeafStringValue(t *testing.T, el *tree_persist.TreeElement, name string) string { + t.Helper() + if el == nil { + return "" + } + if el.GetName() == name && len(el.GetLeafVariant()) > 0 { + tv := &sdcpb.TypedValue{} + if err := proto.Unmarshal(el.GetLeafVariant(), tv); err != nil { + t.Fatalf("unmarshal leaf %q: %v", name, err) + } + return tv.GetStringVal() + } + for _, c := range el.GetChilds() { + if v := findLeafStringValue(t, c, name); v != "" { + return v + } + } + return "" +} + +// TestConfigServerBackend_DeleteApply_NoRehydration is the ghost-intent +// regression test named directly in ticket 03's checklist: under +// Cache.Type: config-server, a delete-apply must remove the last-applied +// entry immediately, so the very next LoadAllButRunningIntents — the exact +// call that rehydrated a deleted intent1 in the original bug — does not +// bring it back. Unlike the other tests in this file, this one wires a real +// *cache.ConfigServerCache over configserver.FakeConfigSnapshotClient (not a +// generic mockcacheclient), so it exercises the actual seam this ticket +// built, not just "some IntentWriter got called." +func TestConfigServerBackend_DeleteApply_NoRehydration(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + fakeClient := configserver.NewFakeConfigSnapshotClient() + const cacheName = "ns1.target1" + ccb := cache.NewCacheClientBound(cacheName, cache.NewConfigServerCache(fakeClient)) + if err := ccb.InstanceCreate(ctx); err != nil { + t.Fatalf("InstanceCreate() error = %v", err) + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + sbi := mocktarget.NewMockTarget(ctrl) + sbi.EXPECT().Set(gomock.Any(), gomock.Any()).Return(&sdcpb.SetDataResponse{}, nil).AnyTimes() + + vpf := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)) + tc := tree.NewTreeContext(scb, vpf) + syncTreeRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + if err := syncTreeRoot.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + ds := &Datastore{ + config: &config.DatastoreConfig{ + Validation: config.NewValidationConfig(), + Name: "test-ds", + }, + syncTreeMutex: &sync.RWMutex{}, + syncTree: syncTreeRoot, + taskPool: vpf, + cacheClient: ccb, + sbi: sbi, + dmutex: &sync.Mutex{}, + schemaClient: scb, + sensitivePathIndex: sensitive.NewPathIndex(), + } + ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds)) + + // Apply #1: create intent1 with real content. + intentJSON, err := ygot.EmitJSON(&sdcio_schema.Device{ + Interface: map[string]*sdcio_schema.SdcioModel_Interface{ + "ethernet-1/1": {Name: ygot.String("ethernet-1/1"), Description: ygot.String("existing")}, + }, + }, &ygot.EmitJSONConfig{Format: ygot.RFC7951, SkipValidation: false}) + if err != nil { + t.Fatalf("marshal intent content: %v", err) + } + tiCreate := types.NewTransactionIntent("intent1", 10) + updates, err := treetypes.ExpandAndConvertIntent(ctx, scb, "intent1", 10, []*sdcpb.Update{{ + Path: &sdcpb.Path{}, + Value: &sdcpb.TypedValue{Value: &sdcpb.TypedValue_JsonVal{JsonVal: []byte(intentJSON)}}, + }}, time.Now().Unix()) + if err != nil { + t.Fatalf("expand intent: %v", err) + } + tiCreate.AddUpdates(updates) + if _, err := ds.TransactionSet(ctx, "txn-create", []*types.TransactionIntent{tiCreate}, nil, 10*time.Second, false); err != nil { + t.Fatalf("TransactionSet() (create) error = %v", err) + } + if err := ds.transactionManager.Confirm("txn-create"); err != nil { + t.Fatalf("Confirm() (create) error = %v", err) + } + + // Sanity: intent1 is now visible via the seam. + preRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + if err := preRoot.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + names, err := ds.LoadAllButRunningIntents(ctx, preRoot) + if err != nil { + t.Fatalf("LoadAllButRunningIntents() (pre-delete) error = %v", err) + } + if diff := cmp.Diff([]string{"ns1.intent1"}, names); diff != "" { + t.Fatalf("LoadAllButRunningIntents() (pre-delete) mismatch (-want +got):\n%s", diff) + } + + // Apply #2: delete intent1. + tiDelete := types.NewTransactionIntent("intent1", 10) + tiDelete.SetDeleteFlag() + if _, err := ds.TransactionSet(ctx, "txn-delete", []*types.TransactionIntent{tiDelete}, nil, 10*time.Second, false); err != nil { + t.Fatalf("TransactionSet() (delete) error = %v", err) + } + + // The regression: the very next LoadAllButRunningIntents must not + // rehydrate intent1. + postRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + if err := postRoot.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + names, err = ds.LoadAllButRunningIntents(ctx, postRoot) + if err != nil { + t.Fatalf("LoadAllButRunningIntents() (post-delete) error = %v", err) + } + if len(names) != 0 { + t.Errorf("LoadAllButRunningIntents() (post-delete) = %v, want empty — intent1 was rehydrated after delete-apply", names) + } +} + // TestForEachIntent_NarrowIntentReader: forEachIntent visits every intent // from a BoundIntentReader. func TestForEachIntent_NarrowIntentReader(t *testing.T) { diff --git a/pkg/server/cache.go b/pkg/server/cache.go index 91a89e0b..0e484925 100644 --- a/pkg/server/cache.go +++ b/pkg/server/cache.go @@ -66,10 +66,10 @@ func (s *Server) createLocalCacheClient(ctx context.Context) error { } // createConfigServerCacheClient wires the colocated config-server -// controller's local ConfigReadService (ticket 07) in as the real-intent -// backend, per the ADR's "New cache.Client implementation" section: -// real Intents are read through configserver.GRPCConfigReader, "running" -// stays independent in-memory state owned by cache.ConfigServerCache itself. +// controller's local ConfigSnapshotService in as the real-intent backend, +// per the ADR's "New cache.Client implementation" section: real Intents are +// read and written through configserver.GRPCConfigClient, "running" stays +// independent in-memory state owned by cache.ConfigServerCache itself. func (s *Server) createConfigServerCacheClient(ctx context.Context) error { log := logf.FromContext(ctx) log.Info("initializing config-server cache client", "address", s.config.Cache.Address) @@ -77,8 +77,8 @@ func (s *Server) createConfigServerCacheClient(ctx context.Context) error { if err != nil { return err } - reader := configserver.NewGRPCConfigReader(cc) - s.cacheClient = cache.NewConfigServerClient(reader) + client := configserver.NewGRPCConfigClient(cc) + s.cacheClient = cache.NewConfigServerCache(client) log.Info("config-server cache client created") return nil } diff --git a/pkg/server/cache_test.go b/pkg/server/cache_test.go index e5365ffc..43716cf9 100644 --- a/pkg/server/cache_test.go +++ b/pkg/server/cache_test.go @@ -16,10 +16,12 @@ package server import ( "context" + "errors" "testing" "github.com/sdcio/data-server/pkg/cache" "github.com/sdcio/data-server/pkg/config" + "github.com/sdcio/sdc-protos/tree_persist" ) func TestCreateConfigServerCacheClient(t *testing.T) { @@ -45,12 +47,14 @@ func TestCreateConfigServerCacheClient(t *testing.T) { } } -// TestCreateConfigServerCacheClient_WritesAreNoOps verifies the config-server -// case composes a Client whose IntentModify/IntentDelete behavior is the -// generic no-op (noopIntentWriter) — reachable without ever going through -// the read seam (configserver.LocalConfigReader), since a real backend would -// need network/reader wiring to answer at all. -func TestCreateConfigServerCacheClient_WritesAreNoOps(t *testing.T) { +// TestCreateConfigServerCacheClient_WritesReachTheClientSeam verifies the +// config-server case composes a Client whose IntentModify/IntentDelete are +// real writes reaching the GRPCConfigClient seam (see +// pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md) +// rather than a no-op IntentWriter. It only exercises the malformed-name +// fast path — asserting ErrMalformedDatastoreName rather than a silent nil — +// since anything past that would need a live config-server connection to answer. +func TestCreateConfigServerCacheClient_WritesReachTheClientSeam(t *testing.T) { s := &Server{ config: &config.Config{ Cache: &config.CacheConfig{ @@ -64,10 +68,12 @@ func TestCreateConfigServerCacheClient_WritesAreNoOps(t *testing.T) { t.Fatalf("createConfigServerCacheClient() error = %v", err) } - if err := s.cacheClient.InstanceIntentModify(context.Background(), "ns1.target1", nil); err != nil { - t.Errorf("InstanceIntentModify() error = %v, want nil", err) + ctx := context.Background() + const malformed = "no-dot-here" + if err := s.cacheClient.InstanceIntentModify(ctx, malformed, &tree_persist.Intent{IntentName: "any-intent"}); !errors.Is(err, cache.ErrMalformedDatastoreName) { + t.Errorf("InstanceIntentModify() error = %v, want ErrMalformedDatastoreName", err) } - if err := s.cacheClient.InstanceIntentDelete(context.Background(), "ns1.target1", "intent1", false); err != nil { - t.Errorf("InstanceIntentDelete() error = %v, want nil", err) + if err := s.cacheClient.InstanceIntentDelete(ctx, malformed, "any-intent", false); !errors.Is(err, cache.ErrMalformedDatastoreName) { + t.Errorf("InstanceIntentDelete() error = %v, want ErrMalformedDatastoreName", err) } } diff --git a/pkg/tree/ops/validation/validation_entry_leafref.go b/pkg/tree/ops/validation/validation_entry_leafref.go index ab9156a2..70e91670 100644 --- a/pkg/tree/ops/validation/validation_entry_leafref.go +++ b/pkg/tree/ops/validation/validation_entry_leafref.go @@ -218,7 +218,7 @@ func validateLeafRefs(ctx context.Context, e api.Entry, resultChan chan<- *types generateOptionalWarning(e, lref, resultChan) return } - owner := "unknown" + owner := types.UnknownOwner highest := e.GetLeafVariants().GetHighestPrecedence(false, false, false) if highest != nil { owner = highest.Owner() @@ -253,7 +253,7 @@ func validateLeafRefs(ctx context.Context, e api.Entry, resultChan chan<- *types func generateOptionalWarning(s api.Entry, lref string, resultChan chan<- *types.ValidationResultEntry) { lrefval := s.GetLeafVariants().GetHighestPrecedence(false, true, false) if lrefval == nil { - resultChan <- types.NewValidationResultEntry("unknown", fmt.Errorf("no leafvariant found for entry %s", s.SdcpbPath()), types.ValidationResultEntryTypeError) + resultChan <- types.NewValidationResultEntry(types.UnknownOwner, fmt.Errorf("no leafvariant found for entry %s", s.SdcpbPath()), types.ValidationResultEntryTypeError) return } tvVal := lrefval.Value() diff --git a/pkg/tree/ops/validation/validation_entry_mandatory.go b/pkg/tree/ops/validation/validation_entry_mandatory.go index 093f0690..109cbc41 100644 --- a/pkg/tree/ops/validation/validation_entry_mandatory.go +++ b/pkg/tree/ops/validation/validation_entry_mandatory.go @@ -99,11 +99,11 @@ func validateMandatoryWithKeys(ctx context.Context, e api.Entry, level int, attr if !success { // if it is not a choice if choiceName == "" { - resultChan <- types.NewValidationResultEntry("unknown", fmt.Errorf("error mandatory child %s does not exist, path: %s", attributes, e.SdcpbPath().ToXPath(false)), types.ValidationResultEntryTypeError) + resultChan <- types.NewValidationResultEntry(types.UnknownOwner, fmt.Errorf("error mandatory child %s does not exist, path: %s", attributes, e.SdcpbPath().ToXPath(false)), types.ValidationResultEntryTypeError) return } // if it is a mandatory choice - resultChan <- types.NewValidationResultEntry("unknown", fmt.Errorf("error mandatory choice %s [attributes: %s] does not exist, path: %s", choiceName, attributes, e.SdcpbPath().ToXPath(false)), types.ValidationResultEntryTypeError) + resultChan <- types.NewValidationResultEntry(types.UnknownOwner, fmt.Errorf("error mandatory choice %s [attributes: %s] does not exist, path: %s", choiceName, attributes, e.SdcpbPath().ToXPath(false)), types.ValidationResultEntryTypeError) return } } diff --git a/pkg/tree/ops/validation/validation_entry_min_max_elements.go b/pkg/tree/ops/validation/validation_entry_min_max_elements.go index 84d51492..2c2c51f4 100644 --- a/pkg/tree/ops/validation/validation_entry_min_max_elements.go +++ b/pkg/tree/ops/validation/validation_entry_min_max_elements.go @@ -25,7 +25,7 @@ func validateMinMaxElements(_ context.Context, e api.Entry, resultChan chan<- *t // get all the childs, skipping the key levels childs, err := ops.GetListChilds(e) if err != nil { - resultChan <- types.NewValidationResultEntry("unknown", fmt.Errorf("error getting childs for min/max-elements check %v", err), types.ValidationResultEntryTypeError) + resultChan <- types.NewValidationResultEntry(types.UnknownOwner, fmt.Errorf("error getting childs for min/max-elements check %v", err), types.ValidationResultEntryTypeError) } intMin := int(contSchema.GetMinElements()) diff --git a/pkg/tree/ops/validation/validation_entry_must.go b/pkg/tree/ops/validation/validation_entry_must.go index 853e472e..499f074b 100644 --- a/pkg/tree/ops/validation/validation_entry_must.go +++ b/pkg/tree/ops/validation/validation_entry_must.go @@ -42,7 +42,7 @@ func validateMustStatements(ctx context.Context, e api.Entry, resultChan chan<- lexer.Parse() prog, err := lexer.CreateProgram(exprStr) if err != nil { - owner := "unknown" + owner := types.UnknownOwner highest := e.GetLeafVariants().GetHighestPrecedence(false, false, false) if highest != nil { owner = highest.Owner() @@ -67,7 +67,7 @@ func validateMustStatements(ctx context.Context, e api.Entry, resultChan chan<- log.Error(err, "stack underflow", "path", e.SdcpbPath().ToXPath(false), "must-expression", exprStr) continue } - owner := "unknown" + owner := types.UnknownOwner // must statement might be assigned on a container, hence we might not have any LeafVariants leafVariants := e.GetLeafVariants() diff --git a/pkg/tree/types/validation_result.go b/pkg/tree/types/validation_result.go index 14d31e89..40cd54a4 100644 --- a/pkg/tree/types/validation_result.go +++ b/pkg/tree/types/validation_result.go @@ -8,6 +8,15 @@ import ( "sync" ) +// UnknownOwner is the intent name emitted by container-level validators +// (mandatory child, must-statement on a container, leafref fallback) when no +// single intent can be identified as the owner of the violation. Containers +// have no leaf-variant of their own; the constraint failure is the result of +// the interplay of multiple intents' leaves, so no single owner can be +// named. Code that processes ValidationResults must handle this value +// explicitly — it is intentional, not an error in attribution logic. +const UnknownOwner = "unknown" + // ValidationResults is map[string]*ValidationResultIntent so consider iterating via range type ValidationResults map[string]*ValidationResultIntent