From 1c84132c13e5a76f6ad073ee8b2784a5cbfce529 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:14:18 +0200 Subject: [PATCH 01/11] chore(deps): pin sdc-protos to rebased ConfigSnapshotService write-RPC commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins to sdcio/sdc-protos@2e109b71c1dd — a rebase of the dangling last-applied-snapshot-write-at-apply branch's ConfigReadService -> ConfigSnapshotService rename + Modify/Delete RPC commit onto this stack's cache-read-backend tip (sdc-protos#134). Opened as https://github.com/sdcio/sdc-protos/pull/135; swap for a tagged release once #123/#134/#135 merge. Signed-off-by: Markus Vahlenkamp --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) 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= From 86257f0a2494d68f5d19b8e0008c39b781a76b5d Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:21:23 +0200 Subject: [PATCH 02/11] feat(cache): real IntentWriter for Cache.Type: config-server (ticket 03) IntentModify/IntentDelete now write through to config-server's ConfigSnapshotService synchronously at apply time, replacing the noopIntentWriter composition ADR 0001 used. Last-applied tracking under this backend now matches Cache.Type: local's write-at-apply timing, closing the ghost-intent race where a deleted intent stayed last-applied until the next best-effort snapshot refresh. - ConfigServerCache gains a LocalConfigClient seam (read+write) and implements IntentWriter directly; NewConfigServerClient no longer composes noopIntentWriter. - New tree-to-JSON flattener (document_export.go) converts an applied tree_persist.Intent into the flat Document shape the read path already understands, for GRPCConfigClient.Modify's wire payload. - Fix the log-only IntentDelete failure in lowlevelTransactionSet: a failed delete-write now hard-fails the transaction, matching IntentModify's existing behavior (ADR 0003's asymmetry fix). - GRPCConfigReader/FakeLocalConfigReader renamed to GRPCConfigClient/FakeLocalConfigClient; mocks/mockconfigread regenerated against the renamed ConfigSnapshotServiceClient. - Tests: seam-level modify/delete round trips, malformed-datastore-name coverage for the new write callers, and datastore-level regression tests for delete-failure hard-fail and rollback-restores-a-deleted- intent. - Commit the previously-drafted ADR 0003 and CONTEXT.md Last-applied term update. Adapted from origin/config-server-cache-backend's 52d842e: reused the existing buildFixtureIntent test helper from sync_test.go instead of duplicating it, fixed the two new IntentGetAll mock expectations to the importer.IntentAdapter channel type (ticket 05's IntentAdapter/ ImportConfigAdapter split postdates this commit's original branch), and dropped ticket06's now-superseded TestConfigServerCache_WritesAreNoOps. Signed-off-by: Markus Vahlenkamp --- Makefile | 2 +- mocks/mockconfigread/client.go | 78 +++++-- pkg/cache/CONTEXT.md | 26 ++- pkg/cache/configserver.go | 85 +++++-- pkg/cache/configserver/document_export.go | 120 ++++++++++ .../configserver/document_export_test.go | 215 ++++++++++++++++++ pkg/cache/configserver/fake.go | 46 ++-- pkg/cache/configserver/fake_test.go | 76 +++++-- pkg/cache/configserver/grpc.go | 69 ++++-- pkg/cache/configserver/grpc_test.go | 36 +-- pkg/cache/configserver/localwrite.go | 48 ++++ pkg/cache/configserver_test.go | 152 +++++++++---- ...ver-write-path-real-last-applied-writes.md | 48 ++++ pkg/datastore/transaction_rpc.go | 2 +- pkg/datastore/transaction_rpc_test.go | 184 +++++++++++++++ pkg/server/cache.go | 12 +- pkg/server/cache_test.go | 27 ++- 17 files changed, 1060 insertions(+), 166 deletions(-) create mode 100644 pkg/cache/configserver/document_export.go create mode 100644 pkg/cache/configserver/document_export_test.go create mode 100644 pkg/cache/configserver/localwrite.go create mode 100644 pkg/cache/docs/adr/0003-config-server-write-path-real-last-applied-writes.md 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/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..0fa73bab 100644 --- a/pkg/cache/CONTEXT.md +++ b/pkg/cache/CONTEXT.md @@ -7,10 +7,28 @@ 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) and "expected state" (says +nothing about whether it's desired-or-applied). **Running**: The synced, on-device configuration state — produced and consumed entirely diff --git a/pkg/cache/configserver.go b/pkg/cache/configserver.go index 592bd718..d05a1c2f 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -28,43 +28,40 @@ import ( ) // 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.LocalConfigClient seam 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. type ConfigServerCache struct { - reader configserver.LocalConfigReader + client configserver.LocalConfigClient 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 { +func NewConfigServerCache(client configserver.LocalConfigClient) *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), - } +// NewConfigServerClient returns a client-backed *ConfigServerCache as a +// Client. ConfigServerCache satisfies IntentWriter directly now (Modify/ +// Delete write through the LocalConfigClient seam to config-server), so +// unlike other Cache.Type backends this needs no noopIntentWriter +// composition. +func NewConfigServerClient(client configserver.LocalConfigClient) Client { + return NewConfigServerCache(client) } // target derives the target namespace/name from cacheInstanceName, mirroring @@ -159,7 +156,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,7 +174,7 @@ 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 } @@ -192,7 +189,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,7 +216,7 @@ 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 @@ -262,3 +259,43 @@ func (c *ConfigServerCache) InstanceRunningModify(ctx context.Context, cacheName c.running[cacheName] = intent return nil } + +// InstanceIntentModify flattens intent into a Document (see +// configserver.DocumentFromIntent) and writes it through the seam +// 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 := configserver.DocumentFromIntent(target, name, intent) + if err != nil { + return err + } + return c.client.Modify(ctx, target, doc) +} + +// InstanceIntentDelete writes through the seam's Delete synchronously. +// ignoreNonExisting is accepted for interface compatibility but is always a +// no-op in practice: LocalConfigWriter.Delete is already idempotent on a +// missing name (TargetSnapshot's membership model has no tombstone to +// distinguish "already gone" from "never existed"), so there is no +// non-idempotent behavior left to gate. +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/document_export.go b/pkg/cache/configserver/document_export.go new file mode 100644 index 00000000..6aafdb1e --- /dev/null +++ b/pkg/cache/configserver/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 configserver + +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 LocalConfigReader already returns for +// reads, so LocalConfigWriter.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/configserver/document_export_test.go b/pkg/cache/configserver/document_export_test.go new file mode 100644 index 00000000..09f2df1c --- /dev/null +++ b/pkg/cache/configserver/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 configserver + +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/fake.go b/pkg/cache/configserver/fake.go index 354dda80..04940ab0 100644 --- a/pkg/cache/configserver/fake.go +++ b/pkg/cache/configserver/fake.go @@ -20,25 +20,27 @@ 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 { +// FakeLocalConfigClient is an in-memory LocalConfigClient, seedable with +// test fixtures. It stands in for the real config-server local-read/write +// 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 FakeLocalConfigClient struct { mu sync.RWMutex docs map[Target]map[string]*Document } -// NewFakeLocalConfigReader returns an empty FakeLocalConfigReader. Use Seed +// NewFakeLocalConfigClient returns an empty FakeLocalConfigClient. Use Seed // to populate it with fixtures. -func NewFakeLocalConfigReader() *FakeLocalConfigReader { - return &FakeLocalConfigReader{ +func NewFakeLocalConfigClient() *FakeLocalConfigClient { + return &FakeLocalConfigClient{ 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 *FakeLocalConfigClient) Seed(target Target, docs ...*Document) { f.mu.Lock() defer f.mu.Unlock() @@ -53,13 +55,13 @@ func (f *FakeLocalConfigReader) Seed(target Target, docs ...*Document) { } // Reset removes every seeded Document for every target. -func (f *FakeLocalConfigReader) Reset() { +func (f *FakeLocalConfigClient) 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 *FakeLocalConfigClient) Get(ctx context.Context, target Target, name string) (*Document, error) { f.mu.RLock() defer f.mu.RUnlock() @@ -74,7 +76,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 *FakeLocalConfigClient) List(ctx context.Context, target Target) ([]*Document, error) { f.mu.RLock() defer f.mu.RUnlock() @@ -88,4 +90,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, under the LocalConfigWriter name — +// 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 *FakeLocalConfigClient) 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 (see LocalConfigWriter). +func (f *FakeLocalConfigClient) Delete(ctx context.Context, target Target, name string) error { + f.mu.Lock() + defer f.mu.Unlock() + delete(f.docs[target], name) + return nil +} + +var _ LocalConfigClient = (*FakeLocalConfigClient)(nil) diff --git a/pkg/cache/configserver/fake_test.go b/pkg/cache/configserver/fake_test.go index 06a31d41..3777cd23 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 TestFakeLocalConfigClient_GetNotFound(t *testing.T) { + f := NewFakeLocalConfigClient() _, 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 TestFakeLocalConfigClient_GetNotFound_UnknownTarget(t *testing.T) { + f := NewFakeLocalConfigClient() 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 TestFakeLocalConfigClient_SeedAndGet(t *testing.T) { + f := NewFakeLocalConfigClient() 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 TestFakeLocalConfigClient_List(t *testing.T) { + f := NewFakeLocalConfigClient() 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 TestFakeLocalConfigClient_ListEmptyForUnknownTarget(t *testing.T) { + f := NewFakeLocalConfigClient() 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 TestFakeLocalConfigClient_SeedReplacesByName(t *testing.T) { + f := NewFakeLocalConfigClient() 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 TestFakeLocalConfigClient_Reset(t *testing.T) { + f := NewFakeLocalConfigClient() 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) } } + +// TestFakeLocalConfigClient_ModifyThenGet locks read-after-write: a Modify +// must be immediately visible to Get, the real-write behavior this fake +// exists to let ConfigServerCache's writer be tested against. +func TestFakeLocalConfigClient_ModifyThenGet(t *testing.T) { + f := NewFakeLocalConfigClient() + 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) + } +} + +// TestFakeLocalConfigClient_DeleteRemovesEntry locks read-after-delete. +func TestFakeLocalConfigClient_DeleteRemovesEntry(t *testing.T) { + f := NewFakeLocalConfigClient() + 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) + } +} + +// TestFakeLocalConfigClient_DeleteMissingIsNoop locks idempotent-delete, +// matching the real ConfigSnapshotService.Delete contract. +func TestFakeLocalConfigClient_DeleteMissingIsNoop(t *testing.T) { + f := NewFakeLocalConfigClient() + + 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..73f3e5b0 100644 --- a/pkg/cache/configserver/grpc.go +++ b/pkg/cache/configserver/grpc.go @@ -24,23 +24,27 @@ 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 LocalConfigClient over the real +// config_read.ConfigSnapshotService, the localhost-bound gRPC surface +// ticket 07 serves 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 both the +// LocalConfigReader and LocalConfigWriter halves of the seam (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 LocalConfigClient 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 +60,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 +// Get calls ConfigSnapshotService.Get, mapping a NotFound gRPC status to // ErrNotFound per the LocalConfigReader 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 +78,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, @@ -112,4 +116,39 @@ func documentFromEntry(e *config_read.ConfigEntry) *Document { } } -var _ LocalConfigReader = (*GRPCConfigReader)(nil) +// Modify calls ConfigSnapshotService.Modify, mapping doc onto the wire +// ConfigEntry shape — the inverse of documentFromEntry. +func (r *GRPCConfigClient) Modify(ctx context.Context, target Target, doc *Document) error { + 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}) + } + _, err := r.client.Modify(ctx, &config_read.ModifyConfigRequest{ + TargetNamespace: target.Namespace, + TargetName: target.Name, + Config: &config_read.ConfigEntry{ + Name: doc.Name, + Namespace: doc.Namespace, + NonRevertive: doc.NonRevertive, + Orphan: doc.Orphan, + Priority: doc.Priority, + SensitivePaths: doc.SensitivePaths, + Config: blobs, + }, + }) + return err +} + +// Delete calls ConfigSnapshotService.Delete. The server side already treats +// a missing key/snapshot as a no-op success (see the config-server ticket +// 02 handler), 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 _ LocalConfigClient = (*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/localwrite.go b/pkg/cache/configserver/localwrite.go new file mode 100644 index 00000000..3672490c --- /dev/null +++ b/pkg/cache/configserver/localwrite.go @@ -0,0 +1,48 @@ +// 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 + +import "context" + +// LocalConfigWriter is the local-write seam a config-server-backed +// cache.Client depends on: upsert one Document by name, or remove it by +// name, scoped to a target — the write-path counterpart to +// LocalConfigReader. It writes 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 LocalConfigWriter interface { + // 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 +} + +// LocalConfigClient is the full local seam a config-server-backed +// cache.Client depends on: LocalConfigReader for real-Intent reads, +// LocalConfigWriter for real-Intent writes. Kept as one interface — rather +// than threading two separate seam parameters everywhere a caller needs +// both — because every concrete implementation (GRPCConfigClient, +// FakeLocalConfigClient) always satisfies both: there is exactly one +// backing resource (TargetSnapshot) and one wire client +// (config_read.ConfigSnapshotServiceClient) behind either seam. +type LocalConfigClient interface { + LocalConfigReader + LocalConfigWriter +} diff --git a/pkg/cache/configserver_test.go b/pkg/cache/configserver_test.go index 0fdadd02..a1645743 100644 --- a/pkg/cache/configserver_test.go +++ b/pkg/cache/configserver_test.go @@ -25,18 +25,30 @@ 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.FakeLocalConfigClient) { t.Helper() - reader := configserver.NewFakeLocalConfigReader() - return NewConfigServerCache(reader), reader + client := configserver.NewFakeLocalConfigClient() + return NewConfigServerCache(client), client } // TestConfigServerCache_InstanceIntentGet_FieldMapping verifies @@ -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 @@ -413,17 +400,100 @@ 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, now +// that Modify/Delete are real writes against the LocalConfigWriter seam +// rather than the generic noopIntentWriter. +func TestConfigServerCache_ImplementsClient(t *testing.T) { + c := NewConfigServerCache(configserver.NewFakeLocalConfigClient()) + var _ Client = c +} + +// TestConfigServerCache_InstanceIntentModify_CreatesAndIsReadableBack +// covers the write-then-read round trip through the seam: 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()) + } +} + +// 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 seam 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/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..6148fa65 --- /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 + +- `noopIntentWriter` (`pkg/cache/noop_intent_writer.go`) is no longer composed for `Cache.Type: config-server`. It remains available in `pkg/cache` for a genuinely read-only future backend to compose — the type itself isn't wrong, only its use here was. +- 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/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..736b9259 100644 --- a/pkg/datastore/transaction_rpc_test.go +++ b/pkg/datastore/transaction_rpc_test.go @@ -512,6 +512,190 @@ 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)) + } + if got := modifyCalls[0].GetIntentName(); got != "intent1" { + t.Errorf("restored intent name = %q, want %q", got, "intent1") + } +} + // 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..92c36665 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.NewConfigServerClient(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..135ee310 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,15 @@ 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 the generic no-op writer other backends fall back to. It only +// exercises the malformed-name fast path — asserting ErrMalformedDatastoreName +// rather than the generic noop 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 +69,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) } } From e387b1f30c948e8bde2a91b775461f1072f51488 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:23:41 +0200 Subject: [PATCH 03/11] test(cache): strengthen ticket 03 review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec review of the real-IntentWriter commit found the delete-no-rehydration checklist item was backed by a weaker test than described, and the rollback-restore test only checked the intent's name, not its content. - Add TestConfigServerBackend_DeleteApply_NoRehydration: wires a real *cache.ConfigServerCache over configserver.FakeLocalConfigClient (not a generic mock) through two TransactionSet calls and asserts the exact regression signature named in the ticket — LoadAllButRunningIntents does not rehydrate a deleted intent. - Strengthen TestTransactionRollback_RestoresDeletedIntent to assert the restored leaf content, not just the intent name. - Clarify ConfigServerCache's ignoreNonExisting doc comment: unused by today's LocalConfigWriter implementations, not an inherent design guarantee. - Note in GRPCConfigClient.Modify why the wire payload is plaintext (ADR 0003's "encrypted payload" describes the persisted record; config-server encrypts on receipt, same as northbound SensitiveConfig). Standards review separately flagged Document<->ConfigEntry mapping duplicated across grpc.go/document_export.go; added configEntryFromDocument as the Modify-side counterpart to the existing documentFromEntry to remove the wire-mapping half of that duplication. Signed-off-by: Markus Vahlenkamp --- pkg/cache/configserver.go | 12 +- pkg/cache/configserver/grpc.go | 34 ++++-- pkg/datastore/transaction_rpc_test.go | 154 +++++++++++++++++++++++++- 3 files changed, 182 insertions(+), 18 deletions(-) diff --git a/pkg/cache/configserver.go b/pkg/cache/configserver.go index d05a1c2f..893a45b9 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -279,11 +279,13 @@ func (c *ConfigServerCache) InstanceIntentModify(ctx context.Context, cacheName } // InstanceIntentDelete writes through the seam's Delete synchronously. -// ignoreNonExisting is accepted for interface compatibility but is always a -// no-op in practice: LocalConfigWriter.Delete is already idempotent on a -// missing name (TargetSnapshot's membership model has no tombstone to -// distinguish "already gone" from "never existed"), so there is no -// non-idempotent behavior left to gate. +// ignoreNonExisting is part of the IntentWriter signature but unused here: +// LocalConfigWriter.Delete's current implementations (GRPCConfigClient, +// FakeLocalConfigClient) 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 LocalConfigWriter 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 { diff --git a/pkg/cache/configserver/grpc.go b/pkg/cache/configserver/grpc.go index 73f3e5b0..c153b86d 100644 --- a/pkg/cache/configserver/grpc.go +++ b/pkg/cache/configserver/grpc.go @@ -116,25 +116,35 @@ func documentFromEntry(e *config_read.ConfigEntry) *Document { } } -// Modify calls ConfigSnapshotService.Modify, mapping doc onto the wire -// ConfigEntry shape — the inverse of documentFromEntry. -func (r *GRPCConfigClient) Modify(ctx context.Context, target Target, doc *Document) error { +// 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: &config_read.ConfigEntry{ - Name: doc.Name, - Namespace: doc.Namespace, - NonRevertive: doc.NonRevertive, - Orphan: doc.Orphan, - Priority: doc.Priority, - SensitivePaths: doc.SensitivePaths, - Config: blobs, - }, + Config: configEntryFromDocument(doc), }) return err } diff --git a/pkg/datastore/transaction_rpc_test.go b/pkg/datastore/transaction_rpc_test.go index 736b9259..c5892a33 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) { @@ -691,9 +694,158 @@ func TestTransactionRollback_RestoresDeletedIntent(t *testing.T) { if len(modifyCalls) != 1 { t.Fatalf("IntentModify call count after rollback = %d, want 1 (restore)", len(modifyCalls)) } - if got := modifyCalls[0].GetIntentName(); got != "intent1" { + 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.FakeLocalConfigClient (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.NewFakeLocalConfigClient() + const cacheName = "ns1.target1" + ccb := cache.NewCacheClientBound(cacheName, cache.NewConfigServerClient(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: treetypes.NewSensitivePathIndex(), + } + 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 d494482701ee56783b49bc41b1fc9f7c8f9441d0 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:25:18 +0200 Subject: [PATCH 04/11] Rename stale ConfigReadService references Renames doc/comment references from ConfigReadService to ConfigSnapshotService in pkg/cache/CONTEXT.md, pkg/cache/configserver.go, pkg/cache/configserver/localread.go, and pkg/config/datastore.go. Follow-up carried over from ticket 06, which deliberately deferred it until ConfigSnapshotService actually existed (landed by the real-IntentWriter commit earlier in this ticket). Signed-off-by: Markus Vahlenkamp --- pkg/cache/configserver.go | 2 +- pkg/cache/configserver/localread.go | 2 +- pkg/config/datastore.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cache/configserver.go b/pkg/cache/configserver.go index 893a45b9..db411fee 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -73,7 +73,7 @@ func (c *ConfigServerCache) target(cacheInstanceName string) (configserver.Targe } // lookupConfigName maps an owner/intent name onto the bare Config resource -// name ConfigReadService keys TargetSnapshot.Spec.Configs by. GetGVKNSN +// 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. diff --git a/pkg/cache/configserver/localread.go b/pkg/cache/configserver/localread.go index 7e90a01a..3649180b 100644 --- a/pkg/cache/configserver/localread.go +++ b/pkg/cache/configserver/localread.go @@ -58,7 +58,7 @@ type ConfigBlob struct { // 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. + // 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 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"` } From 03e81504661f6c58a01ddd50291eb8bde7a62062 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:30:17 +0200 Subject: [PATCH 05/11] chore(cache): remove dead noopIntentWriter and refresh stale comments Drop the unused no-op IntentWriter and dual NewConfigServerClient wrapper, and align comments/ADRs with real config-server last-applied writes. Kept HEAD's already-diverged always-union sensitive-redaction comments in intent_rpc.go/sensitive_path_union_test.go (ticket 04 rewrote that scoping independently of this source branch's snapshot at the time) rather than reapplying this commit's now-stale 'scoped semantics' wording verbatim. Signed-off-by: Markus Vahlenkamp --- pkg/cache/cache.go | 7 +++- pkg/cache/configserver.go | 11 +----- pkg/cache/configserver/grpc.go | 6 +-- pkg/cache/configserver/localread.go | 15 ++++---- pkg/cache/configserver_test.go | 5 +-- .../0001-config-server-backed-cache-client.md | 4 ++ ...02-segregate-cache-client-by-capability.md | 20 ++++++++++ ...ver-write-path-real-last-applied-writes.md | 2 +- pkg/cache/noop_intent_writer.go | 33 ----------------- pkg/cache/noop_intent_writer_test.go | 37 ------------------- pkg/datastore/transaction_rpc_test.go | 2 +- pkg/server/cache.go | 2 +- pkg/server/cache_test.go | 7 ++-- 13 files changed, 50 insertions(+), 101 deletions(-) create mode 100644 pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md delete mode 100644 pkg/cache/noop_intent_writer.go delete mode 100644 pkg/cache/noop_intent_writer_test.go 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 db411fee..9ff08d31 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -48,6 +48,8 @@ type ConfigServerCache struct { // 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. +// ConfigServerCache satisfies IntentWriter directly (Modify/Delete write +// through the LocalConfigClient seam to config-server). func NewConfigServerCache(client configserver.LocalConfigClient) *ConfigServerCache { return &ConfigServerCache{ client: client, @@ -55,15 +57,6 @@ func NewConfigServerCache(client configserver.LocalConfigClient) *ConfigServerCa } } -// NewConfigServerClient returns a client-backed *ConfigServerCache as a -// Client. ConfigServerCache satisfies IntentWriter directly now (Modify/ -// Delete write through the LocalConfigClient seam to config-server), so -// unlike other Cache.Type backends this needs no noopIntentWriter -// composition. -func NewConfigServerClient(client configserver.LocalConfigClient) Client { - return NewConfigServerCache(client) -} - // target derives the target namespace/name from cacheInstanceName, mirroring // config-server's own . datastore-naming // convention. It returns ErrMalformedDatastoreName when cacheInstanceName diff --git a/pkg/cache/configserver/grpc.go b/pkg/cache/configserver/grpc.go index c153b86d..1c698390 100644 --- a/pkg/cache/configserver/grpc.go +++ b/pkg/cache/configserver/grpc.go @@ -26,7 +26,7 @@ import ( // GRPCConfigClient implements LocalConfigClient over the real // config_read.ConfigSnapshotService, the localhost-bound gRPC surface -// ticket 07 serves from inside the colocated config-server controller. +// 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 both the // LocalConfigReader and LocalConfigWriter halves of the seam (see ADR @@ -150,8 +150,8 @@ func (r *GRPCConfigClient) Modify(ctx context.Context, target Target, doc *Docum } // Delete calls ConfigSnapshotService.Delete. The server side already treats -// a missing key/snapshot as a no-op success (see the config-server ticket -// 02 handler), so no NotFound mapping is needed here. +// 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, diff --git a/pkg/cache/configserver/localread.go b/pkg/cache/configserver/localread.go index 3649180b..5dfcecea 100644 --- a/pkg/cache/configserver/localread.go +++ b/pkg/cache/configserver/localread.go @@ -13,14 +13,15 @@ // 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. +// cache.Client and the outside world it reads and writes real Intents +// through. // -// 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. +// LocalConfigReader is exactly what that backend needs for reads: get one +// Document by name, list every Document for a target. 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. package configserver import ( diff --git a/pkg/cache/configserver_test.go b/pkg/cache/configserver_test.go index a1645743..fd3a1f57 100644 --- a/pkg/cache/configserver_test.go +++ b/pkg/cache/configserver_test.go @@ -401,9 +401,8 @@ func TestConfigServerCache_InstanceRunningGet_UnknownInstance(t *testing.T) { } // TestConfigServerCache_ImplementsClient verifies *ConfigServerCache -// satisfies the full cache.Client directly — including IntentWriter, now -// that Modify/Delete are real writes against the LocalConfigWriter seam -// rather than the generic noopIntentWriter. +// satisfies the full cache.Client directly — including IntentWriter, since +// Modify/Delete are real writes against the LocalConfigWriter seam. func TestConfigServerCache_ImplementsClient(t *testing.T) { c := NewConfigServerCache(configserver.NewFakeLocalConfigClient()) var _ Client = c 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..e9101818 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,5 +1,9 @@ # 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. 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..fce5fcf0 --- /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. + +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. +- **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 reader seam (`localread.go`, `grpc.go`, `fake.go`). 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 index 6148fa65..58122d27 100644 --- 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 @@ -42,7 +42,7 @@ Keeping the old name while adding `Modify`/`Delete` to it was explicitly rejecte ## Consequences -- `noopIntentWriter` (`pkg/cache/noop_intent_writer.go`) is no longer composed for `Cache.Type: config-server`. It remains available in `pkg/cache` for a genuinely read-only future backend to compose — the type itself isn't wrong, only its use here was. +- `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/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/datastore/transaction_rpc_test.go b/pkg/datastore/transaction_rpc_test.go index c5892a33..611dd53c 100644 --- a/pkg/datastore/transaction_rpc_test.go +++ b/pkg/datastore/transaction_rpc_test.go @@ -746,7 +746,7 @@ func TestConfigServerBackend_DeleteApply_NoRehydration(t *testing.T) { fakeClient := configserver.NewFakeLocalConfigClient() const cacheName = "ns1.target1" - ccb := cache.NewCacheClientBound(cacheName, cache.NewConfigServerClient(fakeClient)) + ccb := cache.NewCacheClientBound(cacheName, cache.NewConfigServerCache(fakeClient)) if err := ccb.InstanceCreate(ctx); err != nil { t.Fatalf("InstanceCreate() error = %v", err) } diff --git a/pkg/server/cache.go b/pkg/server/cache.go index 92c36665..0e484925 100644 --- a/pkg/server/cache.go +++ b/pkg/server/cache.go @@ -78,7 +78,7 @@ func (s *Server) createConfigServerCacheClient(ctx context.Context) error { return err } client := configserver.NewGRPCConfigClient(cc) - s.cacheClient = cache.NewConfigServerClient(client) + 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 135ee310..43716cf9 100644 --- a/pkg/server/cache_test.go +++ b/pkg/server/cache_test.go @@ -51,10 +51,9 @@ func TestCreateConfigServerCacheClient(t *testing.T) { // 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 the generic no-op writer other backends fall back to. It only -// exercises the malformed-name fast path — asserting ErrMalformedDatastoreName -// rather than the generic noop nil — since anything past that would need a -// live config-server connection to answer. +// 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{ From 6a44a9d2240af7eca5fdb0f6e72f8fd3f1ba5f1b Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:35:02 +0200 Subject: [PATCH 06/11] Extract last-applied Document interchange into configsnapshot. Colocate Intent flatten, blob merge, and NewImportAdapter so Modify->Get round-trips live in one package; configserver keeps only the remote port. Adapted the rename/rename conflict against ticket 06's own configserver/ package layout (which had already moved importer.go/merge.go there from pkg/tree/importer/configserver): re-homed those four files into configsnapshot instead, and kept NewImportAdapter returning importer.IntentAdapter (ticket 05's split) rather than this commit's original pre-split importer.ImportConfigAdapter return type. Signed-off-by: Markus Vahlenkamp --- pkg/cache/CONTEXT.md | 6 +- pkg/cache/configserver.go | 9 +- pkg/cache/configserver/grpc.go | 2 +- pkg/cache/configserver/localread.go | 59 ++------- pkg/cache/configserver_test.go | 14 +++ pkg/cache/configsnapshot/document.go | 71 +++++++++++ .../document_export.go | 2 +- .../document_export_test.go | 2 +- .../document_test.go} | 2 +- .../importer.go | 13 +- .../importer_test.go | 2 +- .../{configserver => configsnapshot}/merge.go | 2 +- .../merge_test.go | 2 +- pkg/cache/configsnapshot/round_trip_test.go | 112 ++++++++++++++++++ ...02-segregate-cache-client-by-capability.md | 6 +- ...gsnapshot-owns-last-applied-interchange.md | 23 ++++ 16 files changed, 260 insertions(+), 67 deletions(-) create mode 100644 pkg/cache/configsnapshot/document.go rename pkg/cache/{configserver => configsnapshot}/document_export.go (99%) rename pkg/cache/{configserver => configsnapshot}/document_export_test.go (99%) rename pkg/cache/{configserver/localread_test.go => configsnapshot/document_test.go} (98%) rename pkg/cache/{configserver => configsnapshot}/importer.go (76%) rename pkg/cache/{configserver => configsnapshot}/importer_test.go (99%) rename pkg/cache/{configserver => configsnapshot}/merge.go (99%) rename pkg/cache/{configserver => configsnapshot}/merge_test.go (99%) create mode 100644 pkg/cache/configsnapshot/round_trip_test.go create mode 100644 pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md diff --git a/pkg/cache/CONTEXT.md b/pkg/cache/CONTEXT.md index 0fa73bab..7ae9f082 100644 --- a/pkg/cache/CONTEXT.md +++ b/pkg/cache/CONTEXT.md @@ -27,8 +27,10 @@ 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) and "expected state" (says -nothing about whether it's desired-or-applied). +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 diff --git a/pkg/cache/configserver.go b/pkg/cache/configserver.go index 9ff08d31..b846e4ac 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -22,6 +22,7 @@ import ( "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" @@ -171,7 +172,7 @@ func (c *ConfigServerCache) InstanceIntentGet(ctx context.Context, cacheName str if err != nil { return nil, err } - return configserver.NewImportAdapter(doc) + return configsnapshot.NewImportAdapter(doc) } // InstanceIntentExists calls Get and maps "not found" to (false, nil), @@ -216,7 +217,7 @@ func (c *ConfigServerCache) InstanceIntentGetAll(ctx context.Context, cacheName } for _, d := range docs { - adapter, err := configserver.NewImportAdapter(d) + adapter, err := configsnapshot.NewImportAdapter(d) if err != nil { errChan <- err return @@ -254,7 +255,7 @@ func (c *ConfigServerCache) InstanceRunningModify(ctx context.Context, cacheName } // InstanceIntentModify flattens intent into a Document (see -// configserver.DocumentFromIntent) and writes it through the seam +// configsnapshot.DocumentFromIntent) and writes it through the seam // 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. @@ -264,7 +265,7 @@ func (c *ConfigServerCache) InstanceIntentModify(ctx context.Context, cacheName return err } name := lookupConfigName(target, intent.GetIntentName()) - doc, err := configserver.DocumentFromIntent(target, name, intent) + doc, err := configsnapshot.DocumentFromIntent(target, name, intent) if err != nil { return err } diff --git a/pkg/cache/configserver/grpc.go b/pkg/cache/configserver/grpc.go index 1c698390..6baa4edc 100644 --- a/pkg/cache/configserver/grpc.go +++ b/pkg/cache/configserver/grpc.go @@ -98,7 +98,7 @@ func (r *GRPCConfigClient) List(ctx context.Context, target Target) ([]*Document // documentFromEntry maps a config_read.ConfigEntry onto the seam'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)) diff --git a/pkg/cache/configserver/localread.go b/pkg/cache/configserver/localread.go index 5dfcecea..9c320e9f 100644 --- a/pkg/cache/configserver/localread.go +++ b/pkg/cache/configserver/localread.go @@ -22,65 +22,30 @@ // 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. +// +// The Document-shaped DTO and Intent↔DTO↔ImportConfigAdapter codec live in +// pkg/cache/configsnapshot; this package only owns the remote port. package configserver import ( "context" "errors" - sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "github.com/sdcio/data-server/pkg/cache/configsnapshot" ) // 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 - // 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: ".", 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 -} +// Target, ConfigBlob, and Document are the ConfigSnapshotService interchange +// DTOs owned by configsnapshot. Aliased here so the LocalConfigClient +// contract keeps a stable import path for port callers. +type ( + Target = configsnapshot.Target + ConfigBlob = configsnapshot.ConfigBlob + Document = configsnapshot.Document +) // LocalConfigReader is the local-read seam a config-server-backed // cache.Client depends on: get one Document by name, list every Document diff --git a/pkg/cache/configserver_test.go b/pkg/cache/configserver_test.go index fd3a1f57..9a26751c 100644 --- a/pkg/cache/configserver_test.go +++ b/pkg/cache/configserver_test.go @@ -438,6 +438,20 @@ func TestConfigServerCache_InstanceIntentModify_CreatesAndIsReadableBack(t *test 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 diff --git a/pkg/cache/configsnapshot/document.go b/pkg/cache/configsnapshot/document.go new file mode 100644 index 00000000..c1b99740 --- /dev/null +++ b/pkg/cache/configsnapshot/document.go @@ -0,0 +1,71 @@ +// 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: ".", 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 +} diff --git a/pkg/cache/configserver/document_export.go b/pkg/cache/configsnapshot/document_export.go similarity index 99% rename from pkg/cache/configserver/document_export.go rename to pkg/cache/configsnapshot/document_export.go index 6aafdb1e..c95976ac 100644 --- a/pkg/cache/configserver/document_export.go +++ b/pkg/cache/configsnapshot/document_export.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/document_export_test.go b/pkg/cache/configsnapshot/document_export_test.go similarity index 99% rename from pkg/cache/configserver/document_export_test.go rename to pkg/cache/configsnapshot/document_export_test.go index 09f2df1c..5d2bd8e8 100644 --- a/pkg/cache/configserver/document_export_test.go +++ b/pkg/cache/configsnapshot/document_export_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 ( "encoding/json" 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 76% rename from pkg/cache/configserver/importer.go rename to pkg/cache/configsnapshot/importer.go index f591a6c0..486a22d4 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 @@ -36,11 +36,16 @@ type documentImporter struct { sensitivePaths []*sdcpb.Path } +<<<<<<<< HEAD:pkg/cache/configserver/importer.go // NewImportAdapter builds the importer.IntentAdapter for doc. func NewImportAdapter(doc *Document) (importer.IntentAdapter, error) { +======== +// NewImportAdapter builds the importer.ImportConfigAdapter for doc. +func NewImportAdapter(doc *Document) (importer.ImportConfigAdapter, error) { +>>>>>>>> 6f0d373 (Extract last-applied Document interchange into configsnapshot.):pkg/cache/configsnapshot/importer.go 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/0002-segregate-cache-client-by-capability.md b/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md index fce5fcf0..46916637 100644 --- a/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md +++ b/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md @@ -2,7 +2,7 @@ **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. +**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. @@ -12,9 +12,9 @@ We split `Client` (and `CacheClientBound`) into four capability interfaces — ` - **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. +- **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 reader seam (`localread.go`, `grpc.go`, `fake.go`). +`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 reader seam (`localread.go`, `grpc.go`, `fake.go`). **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/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..9b315334 --- /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 (`LocalConfigClient` / 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. From 615611201563d5874da31a986fc5a87ed4f4c467 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:40:10 +0200 Subject: [PATCH 07/11] Collapse configserver LocalConfig halves into ConfigSnapshotClient. ADR 0003 already merged the wire into one ConfigSnapshotService; nothing typed against Reader or Writer alone, so the half-interfaces and half-files only mirrored a split the adapters refuse. Align the Go port name with the glossary term. Signed-off-by: Markus Vahlenkamp --- pkg/cache/CONTEXT.md | 18 ++++- pkg/cache/configserver.go | 22 +++--- pkg/cache/configserver/client.go | 76 +++++++++++++++++++ pkg/cache/configserver/fake.go | 43 +++++------ pkg/cache/configserver/fake_test.go | 48 ++++++------ pkg/cache/configserver/grpc.go | 15 ++-- pkg/cache/configserver/localread.go | 60 --------------- pkg/cache/configserver/localwrite.go | 48 ------------ pkg/cache/configserver_test.go | 25 +++--- pkg/cache/configsnapshot/document_export.go | 6 +- pkg/cache/configsnapshot/importer.go | 5 -- ...02-segregate-cache-client-by-capability.md | 2 +- ...gsnapshot-owns-last-applied-interchange.md | 2 +- pkg/datastore/transaction_rpc_test.go | 4 +- 14 files changed, 176 insertions(+), 198 deletions(-) create mode 100644 pkg/cache/configserver/client.go delete mode 100644 pkg/cache/configserver/localread.go delete mode 100644 pkg/cache/configserver/localwrite.go diff --git a/pkg/cache/CONTEXT.md b/pkg/cache/CONTEXT.md index 7ae9f082..2d2c4604 100644 --- a/pkg/cache/CONTEXT.md +++ b/pkg/cache/CONTEXT.md @@ -66,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/configserver.go b/pkg/cache/configserver.go index b846e4ac..481b6ab0 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -29,7 +29,7 @@ import ( ) // ConfigServerCache is the Cache.Type: config-server Client: real Intents -// are read through and written to a configserver.LocalConfigClient seam over +// 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 @@ -37,9 +37,9 @@ import ( // (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 { - client configserver.LocalConfigClient + client configserver.ConfigSnapshotClient mu sync.RWMutex running map[string]*tree_persist.Intent @@ -50,8 +50,8 @@ type ConfigServerCache struct { // cacheInstanceName it's given (see target), so a single ConfigServerCache // correctly serves datastores across multiple Kubernetes namespaces. // ConfigServerCache satisfies IntentWriter directly (Modify/Delete write -// through the LocalConfigClient seam to config-server). -func NewConfigServerCache(client configserver.LocalConfigClient) *ConfigServerCache { +// through the ConfigSnapshotClient port to config-server). +func NewConfigServerCache(client configserver.ConfigSnapshotClient) *ConfigServerCache { return &ConfigServerCache{ client: client, running: map[string]*tree_persist.Intent{}, @@ -232,7 +232,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() @@ -255,7 +255,7 @@ func (c *ConfigServerCache) InstanceRunningModify(ctx context.Context, cacheName } // InstanceIntentModify flattens intent into a Document (see -// configsnapshot.DocumentFromIntent) and writes it through the seam +// 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. @@ -272,13 +272,13 @@ func (c *ConfigServerCache) InstanceIntentModify(ctx context.Context, cacheName return c.client.Modify(ctx, target, doc) } -// InstanceIntentDelete writes through the seam's Delete synchronously. +// InstanceIntentDelete writes through the port's Delete synchronously. // ignoreNonExisting is part of the IntentWriter signature but unused here: -// LocalConfigWriter.Delete's current implementations (GRPCConfigClient, -// FakeLocalConfigClient) are unconditionally idempotent on a missing name — +// 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 LocalConfigWriter that needed to distinguish +// 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) 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 04940ab0..40c2138e 100644 --- a/pkg/cache/configserver/fake.go +++ b/pkg/cache/configserver/fake.go @@ -20,27 +20,26 @@ import ( "sync" ) -// FakeLocalConfigClient is an in-memory LocalConfigClient, seedable with -// test fixtures. It stands in for the real config-server local-read/write -// 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 FakeLocalConfigClient 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 } -// NewFakeLocalConfigClient returns an empty FakeLocalConfigClient. Use Seed -// to populate it with fixtures. -func NewFakeLocalConfigClient() *FakeLocalConfigClient { - return &FakeLocalConfigClient{ +// 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 *FakeLocalConfigClient) Seed(target Target, docs ...*Document) { +func (f *FakeConfigSnapshotClient) Seed(target Target, docs ...*Document) { f.mu.Lock() defer f.mu.Unlock() @@ -55,13 +54,13 @@ func (f *FakeLocalConfigClient) Seed(target Target, docs ...*Document) { } // Reset removes every seeded Document for every target. -func (f *FakeLocalConfigClient) Reset() { +func (f *FakeConfigSnapshotClient) Reset() { f.mu.Lock() defer f.mu.Unlock() f.docs = map[Target]map[string]*Document{} } -func (f *FakeLocalConfigClient) 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() @@ -76,7 +75,7 @@ func (f *FakeLocalConfigClient) Get(ctx context.Context, target Target, name str return doc, nil } -func (f *FakeLocalConfigClient) 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() @@ -90,22 +89,22 @@ func (f *FakeLocalConfigClient) List(ctx context.Context, target Target) ([]*Doc return result, nil } -// Modify is Seed for a single Document, under the LocalConfigWriter name — -// 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 *FakeLocalConfigClient) Modify(ctx context.Context, target Target, doc *Document) error { +// 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 (see LocalConfigWriter). -func (f *FakeLocalConfigClient) Delete(ctx context.Context, target Target, name string) error { +// 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 _ LocalConfigClient = (*FakeLocalConfigClient)(nil) +var _ ConfigSnapshotClient = (*FakeConfigSnapshotClient)(nil) diff --git a/pkg/cache/configserver/fake_test.go b/pkg/cache/configserver/fake_test.go index 3777cd23..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 TestFakeLocalConfigClient_GetNotFound(t *testing.T) { - f := NewFakeLocalConfigClient() +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 TestFakeLocalConfigClient_GetNotFound(t *testing.T) { } } -func TestFakeLocalConfigClient_GetNotFound_UnknownTarget(t *testing.T) { - f := NewFakeLocalConfigClient() +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 TestFakeLocalConfigClient_GetNotFound_UnknownTarget(t *testing.T) { } } -func TestFakeLocalConfigClient_SeedAndGet(t *testing.T) { - f := NewFakeLocalConfigClient() +func TestFakeConfigSnapshotClient_SeedAndGet(t *testing.T) { + f := NewFakeConfigSnapshotClient() target := Target{Namespace: "ns1", Name: "target1"} want := &Document{ @@ -70,8 +70,8 @@ func TestFakeLocalConfigClient_SeedAndGet(t *testing.T) { } } -func TestFakeLocalConfigClient_List(t *testing.T) { - f := NewFakeLocalConfigClient() +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 TestFakeLocalConfigClient_List(t *testing.T) { } } -func TestFakeLocalConfigClient_ListEmptyForUnknownTarget(t *testing.T) { - f := NewFakeLocalConfigClient() +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 TestFakeLocalConfigClient_ListEmptyForUnknownTarget(t *testing.T) { } } -func TestFakeLocalConfigClient_SeedReplacesByName(t *testing.T) { - f := NewFakeLocalConfigClient() +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 TestFakeLocalConfigClient_SeedReplacesByName(t *testing.T) { } } -func TestFakeLocalConfigClient_Reset(t *testing.T) { - f := NewFakeLocalConfigClient() +func TestFakeConfigSnapshotClient_Reset(t *testing.T) { + f := NewFakeConfigSnapshotClient() target := Target{Namespace: "ns1", Name: "target1"} f.Seed(target, &Document{Name: "intent1"}) @@ -133,11 +133,11 @@ func TestFakeLocalConfigClient_Reset(t *testing.T) { } } -// TestFakeLocalConfigClient_ModifyThenGet locks read-after-write: a Modify +// 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 writer be tested against. -func TestFakeLocalConfigClient_ModifyThenGet(t *testing.T) { - f := NewFakeLocalConfigClient() +// 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 { @@ -153,9 +153,9 @@ func TestFakeLocalConfigClient_ModifyThenGet(t *testing.T) { } } -// TestFakeLocalConfigClient_DeleteRemovesEntry locks read-after-delete. -func TestFakeLocalConfigClient_DeleteRemovesEntry(t *testing.T) { - f := NewFakeLocalConfigClient() +// 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"}) @@ -171,10 +171,10 @@ func TestFakeLocalConfigClient_DeleteRemovesEntry(t *testing.T) { } } -// TestFakeLocalConfigClient_DeleteMissingIsNoop locks idempotent-delete, +// TestFakeConfigSnapshotClient_DeleteMissingIsNoop locks idempotent-delete, // matching the real ConfigSnapshotService.Delete contract. -func TestFakeLocalConfigClient_DeleteMissingIsNoop(t *testing.T) { - f := NewFakeLocalConfigClient() +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 6baa4edc..60472924 100644 --- a/pkg/cache/configserver/grpc.go +++ b/pkg/cache/configserver/grpc.go @@ -24,13 +24,12 @@ import ( "google.golang.org/grpc/status" ) -// GRPCConfigClient implements LocalConfigClient over the real +// 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 both the -// LocalConfigReader and LocalConfigWriter halves of the seam (see ADR -// 0003's "Wire contract" section). +// (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 @@ -41,7 +40,7 @@ type GRPCConfigClient struct { client config_read.ConfigSnapshotServiceClient } -// NewGRPCConfigClient returns a LocalConfigClient that calls the +// NewGRPCConfigClient returns a ConfigSnapshotClient that calls the // ConfigSnapshotService over cc. func NewGRPCConfigClient(cc grpc.ClientConnInterface) *GRPCConfigClient { return &GRPCConfigClient{client: config_read.NewConfigSnapshotServiceClient(cc)} @@ -61,7 +60,7 @@ func Dial(address string, opts ...grpc.DialOption) (*grpc.ClientConn, error) { } // Get calls ConfigSnapshotService.Get, mapping a NotFound gRPC status to -// ErrNotFound per the LocalConfigReader contract; any other error +// ErrNotFound per the ConfigSnapshotClient contract; any other error // propagates unwrapped. func (r *GRPCConfigClient) Get(ctx context.Context, target Target, name string) (*Document, error) { rsp, err := r.client.Get(ctx, &config_read.GetConfigRequest{ @@ -96,7 +95,7 @@ func (r *GRPCConfigClient) 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 // configsnapshot.NewImportAdapter's job. func documentFromEntry(e *config_read.ConfigEntry) *Document { @@ -161,4 +160,4 @@ func (r *GRPCConfigClient) Delete(ctx context.Context, target Target, name strin return err } -var _ LocalConfigClient = (*GRPCConfigClient)(nil) +var _ ConfigSnapshotClient = (*GRPCConfigClient)(nil) diff --git a/pkg/cache/configserver/localread.go b/pkg/cache/configserver/localread.go deleted file mode 100644 index 9c320e9f..00000000 --- a/pkg/cache/configserver/localread.go +++ /dev/null @@ -1,60 +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 and writes real Intents -// through. -// -// LocalConfigReader is exactly what that backend needs for reads: get one -// Document by name, list every Document for a target. 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. -// -// The Document-shaped DTO and Intent↔DTO↔ImportConfigAdapter codec live in -// pkg/cache/configsnapshot; this package only owns the remote port. -package configserver - -import ( - "context" - "errors" - - "github.com/sdcio/data-server/pkg/cache/configsnapshot" -) - -// 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, ConfigBlob, and Document are the ConfigSnapshotService interchange -// DTOs owned by configsnapshot. Aliased here so the LocalConfigClient -// contract keeps a stable import path for port callers. -type ( - Target = configsnapshot.Target - ConfigBlob = configsnapshot.ConfigBlob - Document = configsnapshot.Document -) - -// 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/localwrite.go b/pkg/cache/configserver/localwrite.go deleted file mode 100644 index 3672490c..00000000 --- a/pkg/cache/configserver/localwrite.go +++ /dev/null @@ -1,48 +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 - -import "context" - -// LocalConfigWriter is the local-write seam a config-server-backed -// cache.Client depends on: upsert one Document by name, or remove it by -// name, scoped to a target — the write-path counterpart to -// LocalConfigReader. It writes 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 LocalConfigWriter interface { - // 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 -} - -// LocalConfigClient is the full local seam a config-server-backed -// cache.Client depends on: LocalConfigReader for real-Intent reads, -// LocalConfigWriter for real-Intent writes. Kept as one interface — rather -// than threading two separate seam parameters everywhere a caller needs -// both — because every concrete implementation (GRPCConfigClient, -// FakeLocalConfigClient) always satisfies both: there is exactly one -// backing resource (TargetSnapshot) and one wire client -// (config_read.ConfigSnapshotServiceClient) behind either seam. -type LocalConfigClient interface { - LocalConfigReader - LocalConfigWriter -} diff --git a/pkg/cache/configserver_test.go b/pkg/cache/configserver_test.go index 9a26751c..309af4d5 100644 --- a/pkg/cache/configserver_test.go +++ b/pkg/cache/configserver_test.go @@ -45,14 +45,14 @@ const ( testCacheName = testNamespace + "." + testTarget ) -func newTestConfigServerCache(t *testing.T) (*ConfigServerCache, *configserver.FakeLocalConfigClient) { +func newTestConfigServerCache(t *testing.T) (*ConfigServerCache, *configserver.FakeConfigSnapshotClient) { t.Helper() - client := configserver.NewFakeLocalConfigClient() + 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() @@ -330,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) @@ -356,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}) @@ -364,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) } } @@ -402,14 +405,14 @@ func TestConfigServerCache_InstanceRunningGet_UnknownInstance(t *testing.T) { // TestConfigServerCache_ImplementsClient verifies *ConfigServerCache // satisfies the full cache.Client directly — including IntentWriter, since -// Modify/Delete are real writes against the LocalConfigWriter seam. +// Modify/Delete are real writes against the ConfigSnapshotClient port. func TestConfigServerCache_ImplementsClient(t *testing.T) { - c := NewConfigServerCache(configserver.NewFakeLocalConfigClient()) + c := NewConfigServerCache(configserver.NewFakeConfigSnapshotClient()) var _ Client = c } // TestConfigServerCache_InstanceIntentModify_CreatesAndIsReadableBack -// covers the write-then-read round trip through the seam: a modified +// 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. @@ -456,7 +459,7 @@ func TestConfigServerCache_InstanceIntentModify_CreatesAndIsReadableBack(t *test // 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 seam immediately, not +// 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() diff --git a/pkg/cache/configsnapshot/document_export.go b/pkg/cache/configsnapshot/document_export.go index c95976ac..a2093994 100644 --- a/pkg/cache/configsnapshot/document_export.go +++ b/pkg/cache/configsnapshot/document_export.go @@ -26,9 +26,9 @@ import ( // 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 LocalConfigReader already returns for -// reads, so LocalConfigWriter.Modify can send it as a ConfigSnapshotService -// wire payload without a second, parallel field-mapping table. +// 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 diff --git a/pkg/cache/configsnapshot/importer.go b/pkg/cache/configsnapshot/importer.go index 486a22d4..7b9ab3f7 100644 --- a/pkg/cache/configsnapshot/importer.go +++ b/pkg/cache/configsnapshot/importer.go @@ -36,13 +36,8 @@ type documentImporter struct { sensitivePaths []*sdcpb.Path } -<<<<<<<< HEAD:pkg/cache/configserver/importer.go // NewImportAdapter builds the importer.IntentAdapter for doc. func NewImportAdapter(doc *Document) (importer.IntentAdapter, error) { -======== -// NewImportAdapter builds the importer.ImportConfigAdapter for doc. -func NewImportAdapter(doc *Document) (importer.ImportConfigAdapter, error) { ->>>>>>>> 6f0d373 (Extract last-applied Document interchange into configsnapshot.):pkg/cache/configsnapshot/importer.go root, err := mergeConfigBlobs(doc.Config) if err != nil { return nil, fmt.Errorf("configsnapshot: building config for %q: %w", doc.Name, err) 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 index 46916637..956ff412 100644 --- a/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md +++ b/pkg/cache/docs/adr/0002-segregate-cache-client-by-capability.md @@ -17,4 +17,4 @@ We split `Client` (and `CacheClientBound`) into four capability interfaces — ` ## 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 reader seam (`localread.go`, `grpc.go`, `fake.go`). **Superseded by ADR 0004** for that relocate — interchange lives in `pkg/cache/configsnapshot`; `pkg/tree/importer/configserver` is removed. +`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/0004-configsnapshot-owns-last-applied-interchange.md b/pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md index 9b315334..ab2536c9 100644 --- a/pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md +++ b/pkg/cache/docs/adr/0004-configsnapshot-owns-last-applied-interchange.md @@ -8,7 +8,7 @@ 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 (`LocalConfigClient` / 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. +**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 diff --git a/pkg/datastore/transaction_rpc_test.go b/pkg/datastore/transaction_rpc_test.go index 611dd53c..4e9f9822 100644 --- a/pkg/datastore/transaction_rpc_test.go +++ b/pkg/datastore/transaction_rpc_test.go @@ -732,7 +732,7 @@ func findLeafStringValue(t *testing.T, el *tree_persist.TreeElement, name string // 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.FakeLocalConfigClient (not a +// *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) { @@ -744,7 +744,7 @@ func TestConfigServerBackend_DeleteApply_NoRehydration(t *testing.T) { } scb := schemaClient.NewSchemaClientBound(schema, sc) - fakeClient := configserver.NewFakeLocalConfigClient() + fakeClient := configserver.NewFakeConfigSnapshotClient() const cacheName = "ns1.target1" ccb := cache.NewCacheClientBound(cacheName, cache.NewConfigServerCache(fakeClient)) if err := ccb.InstanceCreate(ctx); err != nil { From 3b9a2dfb693c3d5cee0970ebd00f2ad7b0c0715d Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:42:27 +0200 Subject: [PATCH 08/11] Colocate config-server Namespaced name codec with ConfigServerCache. Move split/strip next to the backend and keep join as a single helper behind Document.IntentName so the encoding story lives with its owners. Follow-up carried over from ticket 06 (deliberately deferred until configsnapshot's own document.go existed, which this ticket's 6f0d373 equivalent landed earlier). Signed-off-by: Markus Vahlenkamp --- pkg/cache/configserver.go | 14 ------ ...gserver_split.go => configserver_names.go} | 16 ++++++- ...lit_test.go => configserver_names_test.go} | 45 +++++++++++++++++++ pkg/cache/configsnapshot/document.go | 17 ++++--- 4 files changed, 72 insertions(+), 20 deletions(-) rename pkg/cache/{configserver_split.go => configserver_names.go} (69%) rename pkg/cache/{configserver_split_test.go => configserver_names_test.go} (65%) diff --git a/pkg/cache/configserver.go b/pkg/cache/configserver.go index 481b6ab0..2733d8ee 100644 --- a/pkg/cache/configserver.go +++ b/pkg/cache/configserver.go @@ -18,7 +18,6 @@ import ( "context" "errors" "fmt" - "strings" "sync" "github.com/sdcio/data-server/pkg/cache/configserver" @@ -66,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 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. -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. 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/configsnapshot/document.go b/pkg/cache/configsnapshot/document.go index c1b99740..bc5c49d5 100644 --- a/pkg/cache/configsnapshot/document.go +++ b/pkg/cache/configsnapshot/document.go @@ -61,11 +61,18 @@ type Document struct { } // 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. +// 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 { - if d.Namespace == "" { - return d.Name + 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 d.Namespace + "." + d.Name + return namespace + "." + name } From 3a568d17000cf610d488b08f22af34f32ff9f586 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:44:21 +0200 Subject: [PATCH 09/11] refactor(validation): replace hardcoded "unknown" owner literals with types.UnknownOwner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracted the orthogonal, no-behavior-change half of f0c91db (from origin/config-server-cache-backend): the ghost-intent exclusion mechanism that commit also removed was never added to this stack in the first place (ticket 05 skipped 10652b4 entirely), so there's nothing to revert here — just the pure magic-string cleanup, done as its own tiny commit per the ticket 07 notes. Signed-off-by: Markus Vahlenkamp --- pkg/tree/ops/validation/validation_entry_leafref.go | 4 ++-- pkg/tree/ops/validation/validation_entry_mandatory.go | 4 ++-- .../ops/validation/validation_entry_min_max_elements.go | 2 +- pkg/tree/ops/validation/validation_entry_must.go | 4 ++-- pkg/tree/types/validation_result.go | 9 +++++++++ 5 files changed, 16 insertions(+), 7 deletions(-) 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 From 7fc39ba3b009b2fbe89787c7f29052a5478d547a Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 23 Sep 2026 13:46:19 +0200 Subject: [PATCH 10/11] docs(cache): drop dead docs/architecture.md references in ADR 0001 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/architecture.md was never split into this repo's history — fold the two facts it was cited for (config-server as sole northbound Intent writer; cold-start recovery on the control plane) inline instead of pointing at a file that doesn't exist. Extracted the pkg/cache-relevant sliver of origin/config-server-cache-backend's 67f4bdf, which otherwise spans an unrelated repo-wide docs/agent-process reorg out of scope for this ticket. Signed-off-by: Markus Vahlenkamp --- pkg/cache/docs/adr/0001-config-server-backed-cache-client.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 e9101818..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 @@ -10,9 +10,9 @@ data-server gains a second `cache.Client` implementation, selected by `Cache.Typ ## 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 From 68a1b5b0cd59914657e736f8224e519e44fe8051 Mon Sep 17 00:00:00 2001 From: Markus Vahlenkamp Date: Wed, 14 Oct 2026 10:47:29 +0200 Subject: [PATCH 11/11] fix(test): use sensitive.NewPathIndex in transaction RPC tests New tests from ticket 03 referenced treetypes.NewSensitivePathIndex, which does not exist; match the rest of the file and Datastore wiring. Signed-off-by: Markus Vahlenkamp --- pkg/datastore/transaction_rpc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/datastore/transaction_rpc_test.go b/pkg/datastore/transaction_rpc_test.go index 4e9f9822..4a93370b 100644 --- a/pkg/datastore/transaction_rpc_test.go +++ b/pkg/datastore/transaction_rpc_test.go @@ -778,7 +778,7 @@ func TestConfigServerBackend_DeleteApply_NoRehydration(t *testing.T) { sbi: sbi, dmutex: &sync.Mutex{}, schemaClient: scb, - sensitivePathIndex: treetypes.NewSensitivePathIndex(), + sensitivePathIndex: sensitive.NewPathIndex(), } ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds))