From 99f963484b492c86726036682525990866e1ccff Mon Sep 17 00:00:00 2001 From: Repinoid Date: Wed, 30 Sep 2026 21:07:49 +0300 Subject: [PATCH] =?UTF-8?q?fix(core):=20idempotency=20pre-check=20=D0=B1?= =?UTF-8?q?=D0=B5=D0=B7=20=D1=81=D1=85=D0=B5=D0=BC=D1=8B=20=E2=80=94=20?= =?UTF-8?q?=D0=B5=D0=B4=D0=B8=D0=BD=D1=8B=D0=B9=20=D0=B8=D1=81=D1=82=D0=BE?= =?UTF-8?q?=D1=87=D0=BD=D0=B8=D0=BA=20(live)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Код-ревью (раунд 5), п.1/6: pre-check брал схему из GET /instanceOperations/default/{opId}, а payload строился по живой схеме ?fields=cfsParams — два источника. default может расходиться с живой => риск ложного пропуска modify. Решение: pre-check вообще не запрашивает схему. - modifierDesiredEqualsLive(ctx, uid, desired): сравнение по live-кодам (live[lower(code)]), единственный источник — state.params. - Значения: похожи на JSON ({/[) — смысловое сравнение; иначе скалярное с нормализацией (null/"" -> ""; true/false без учёта регистра) — закрывает и регистр bool. - Fail-safe сохранён: пусто/нет кода/ошибка live => modify выполняется. - Удалены: fetchOperationSchemaByID, modifierValuesEqual, lookupLiveParam больше не участвует в pre-check (остаётся для досылки). - Тесты переписаны (RawValuesEqual_Scalars/JSON, DesiredEqualsLive: 4 кейса). Документация: TOOLS/ARCHITECTURE.md -> новый раздел «Modifier Idempotency» (5 правил контракта); HISTORY — журнал раунда 5. Проверено: go build ./... OK; go test ./internal/... -short PASS. --- ...26-09-30_core_and_modifiers_remediation.md | 35 ++++++++++ TOOLS/ARCHITECTURE.md | 19 ++++++ provider/internal/core/client_test.go | 3 - provider/internal/core/modifier_compare.go | 61 ++++++++++------- .../internal/core/modifier_compare_test.go | 68 ++++++++++--------- provider/internal/core/operation_cfs.go | 18 ----- .../internal/core/operation_run_bycode.go | 14 ++-- 7 files changed, 131 insertions(+), 87 deletions(-) diff --git a/HISTORY/2026-09-30_core_and_modifiers_remediation.md b/HISTORY/2026-09-30_core_and_modifiers_remediation.md index eea1bbc..41783b8 100644 --- a/HISTORY/2026-09-30_core_and_modifiers_remediation.md +++ b/HISTORY/2026-09-30_core_and_modifiers_remediation.md @@ -106,4 +106,39 @@ Required с пустым `data_type` (все получают `""`; угадыв **Осталось:** замер владельцем (`terraform plan` ×2 на `DEV_STAND/FullPipe`); B8 (`modifiers.yaml`). +--- + +## Раунд 5 — код-ревью (Opus) и его фиксы + +Промпт: `NOTES/20_prompts/prompt_for_opus_code_review_round5.md`. Opus нашёл 6 пунктов; критичные — 2. + +| # | Находка | Решение | +|---|---|---| +| 1 | **Регрессия B6:** pre-check стоял ПОСЛЕ `POST /instanceOperations` → при совпадении оставался «черновик» операции (pending). | Исправлено: pre-check до создания операции. | +| 2 | Partial state: `resp.State.Set(&data)` мог записать unknown/computed → «invalid new value … unknown». | Исправлено: пишется только `id` (`SetAttribute`). | +| 3 | 401 ретраится, но токен между попытками не обновляется. | Принято как есть; пояснено в комментарии `http.go`. | +| 4 | `ImportState` vs `Read` — конфликта нет. | ок. | +| 5 | `modifierDesiredEqualsCurrent` — мёртвый код (только тесты). | Удалено; тесты переведены на живые функции. | +| 6 | Сравнение по `default`-схеме рискует ложным пропуском (ревизия п.1). | Исправлено: pre-check **без схемы** — только live-коды. | + +**Итоговый контракт pre-check (см. `TOOLS/ARCHITECTURE.md` → «Modifier Idempotency»):** +1. pre-check выполняется **до** `POST /instanceOperations`; +2. единственный источник — live (`state.params`), схема операции не запрашивается + (`default/{opId}` может расходиться с живой; живая доступна только после создания); +3. поиск значения — по самому коду (`live[lower(code)]`); +4. fail-safe: пусто/нет кода/ошибка live → modify выполняется; +5. сравнение: похоже на JSON — смысловое (порядок ключей не значим), иначе — скалярное + с нормализацией (`null`/`""` → `""`; `true`/`false` без учёта регистра). + +Файлы: `core/modifier_compare.go` (новые `modifierRawValuesEqual`, `looksLikeJSON`, +`normalizeRawScalar`, `modifierDesiredEqualsLive` без схемы), `core/operation_run_bycode.go`, +`core/operation_cfs.go` (удалена `fetchOperationSchemaByID`), `core/client_test.go`, +`core/modifier_compare_test.go`, `TOOLS/ARCHITECTURE.md`. + +Тесты: `TestRunInstanceOperationByIdempotent_SkipsWithoutCreatingOperation` (POST операции не +вызывается при совпадении), `TestModifierRawValuesEqual_Scalars/JSON`, +`TestModifierDesiredEqualsLive` (совпало / отличается / нет кода / live недоступен). +Проверено: `go build ./...` OK, `go test ./internal/... -short` PASS. + + diff --git a/TOOLS/ARCHITECTURE.md b/TOOLS/ARCHITECTURE.md index 124bfe9..ef0ea13 100644 --- a/TOOLS/ARCHITECTURE.md +++ b/TOOLS/ARCHITECTURE.md @@ -134,6 +134,25 @@ Each operation has a kind: provider code nor the captured HAR contain `DELETE /instanceOperations/{uid}`. - Transient 401 is retried for GET (see `isRetryable`). +### Modifier Idempotency (decided 2026-09-30) + +Modifiers may opt into an idempotency pre-check (`RunInstanceOperationUniversalByIdempotent`, +used by `nubes_vc_nsxt_snat`). Rules: + +- The pre-check runs **before** `POST /instanceOperations`. Otherwise a skipped modify + leaves a created-but-never-run operation ("draft", pending) and the next call waits + for idle until timeout. +- The **only** comparison source is the live instance state (`state.params` via + `instanceLiveParams`). The operation schema (`/instanceOperations/default/{opId}`, + `?fields=cfsParams`) is **deliberately not used**: before the operation exists only the + `default` schema is available, and it may diverge from the live one — comparing through + it risks a false skip. (Using the live schema would require creating the operation + first — which is exactly the regression above.) +- Values are matched by the code itself (`live[lower(code)]`). +- Fail-safe: empty live, code absent, or live read error → the modify runs (never skipped). +- JSON-looking values (`{`/`[`) are compared semantically (key order ignored); scalars are + normalized (`null`/`""` → empty; `true`/`false` case-insensitive). + ### Generated Code Resilience - **Partial state при ошибке create** (шаблон `instance.go`): если инстанс успел создаться diff --git a/provider/internal/core/client_test.go b/provider/internal/core/client_test.go index 0426403..778deba 100644 --- a/provider/internal/core/client_test.go +++ b/provider/internal/core/client_test.go @@ -88,9 +88,6 @@ func TestRunInstanceOperationByIdempotent_SkipsWithoutCreatingOperation(t *testi case r.Method == http.MethodGet && r.URL.Path == "/instances/inst-1": w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"instance":{"instanceUid":"inst-1","serviceId":22,"explainedStatus":"running","isDeleted":false,"operationIsPending":false,"operationIsInProgress":false,"availableOperations":[{"svcOperationId":207,"operation":"modify"}],"state":{"params":{"ipSpaceName":"internet-ipv4-v1"}}}}`)) - case r.Method == http.MethodGet && r.URL.Path == "/instanceOperations/default/207": - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"svcOperation":{"cfsParams":[{"svcOperationCfsParamId":372,"code":"ipSpaceName","dataType":"string"}]}}`)) case r.Method == http.MethodPost && r.URL.Path == "/instanceOperations": opPosts.Add(1) w.WriteHeader(http.StatusCreated) diff --git a/provider/internal/core/modifier_compare.go b/provider/internal/core/modifier_compare.go index 7057633..193cc40 100644 --- a/provider/internal/core/modifier_compare.go +++ b/provider/internal/core/modifier_compare.go @@ -7,30 +7,46 @@ import ( "terraform-provider-nubes/internal/core/jsonutil" ) -// modifierValuesEqual сравнивает одно значение с учётом типа параметра: -// - array*/map*/json — смысловое JSON-сравнение (порядок ключей не значим); -// - скаляры — нормализация через normalizeUniversalValueV6 и сравнение строками. -func modifierValuesEqual(wanted, current string, param universalCfsParam) bool { - dataType := strings.ToLower(strings.TrimSpace(param.DataType)) - if strings.HasPrefix(dataType, "array") || - strings.HasPrefix(dataType, "map") || - strings.Contains(dataType, "json") { - return jsonutil.JSONStringsEquivalent(wanted, current) +// modifierRawValuesEqual сравнивает два значения БЕЗ схемы операции (dataType может быть +// недоступен до создания операции, или отличаться между default- и live-схемой): +// - оба выглядят как JSON (`{`/`[`) — смысловое JSON-сравнение (порядок ключей не значим); +// - иначе — строковое сравнение с нормализацией null/""/true/false (без учёта регистра). +func modifierRawValuesEqual(wanted, current string) bool { + w := strings.TrimSpace(wanted) + c := strings.TrimSpace(current) + if looksLikeJSON(w) && looksLikeJSON(c) { + return jsonutil.JSONStringsEquivalent(w, c) } - return normalizeUniversalValueV6(wanted, param) == normalizeUniversalValueV6(current, param) + return normalizeRawScalar(w) == normalizeRawScalar(c) +} + +func looksLikeJSON(v string) bool { + return strings.HasPrefix(v, "{") || strings.HasPrefix(v, "[") +} + +// normalizeRawScalar приводит скаляр к каноническому виду без схемы: +// null/"" -> "", true/false — к lower-case, остальное — trim. +func normalizeRawScalar(v string) string { + switch { + case strings.EqualFold(v, "null"), v == `""`: + return "" + case strings.EqualFold(v, "true"): + return "true" + case strings.EqualFold(v, "false"): + return "false" + } + return v } // modifierDesiredEqualsLive — idempotency-pre-check по ЖИВЫМ параметрам инстанса -// (state.params), а не по paramValue формы операции. +// (state.params). Источник — ЕДИНСТВЕННЫЙ: live. Схема операции НЕ запрашивается (см. +// ARCHITECTURE.md, «Modifier idempotency»): до создания операции доступна только +// default-схема, а она может расходиться с живой — сравнение по ней рискует ложным +// пропуском. Поиск значения — по самому коду (live keyed by lower(code)). // -// Зачем: paramValue из cfsParams — это дефолт ФОРМЫ операции, а не состояние -// инстанса (см. instanceLiveParams, HAR/edge_.har). Пропуск modify на основе -// формы может быть ложным. Здесь источник — live. -// -// (true, nil) возвращается только если ВСЕ desired-поля найдены в live и совпали. -// Любая неопределённость (пусто, поле не найдено, ошибка) → (false, …): modify +// Fail-safe: live пуст, код не найден или ошибка чтения live → (false, …): modify // лучше выполнить лишний раз, чем пропустить нужный. -func (c *UniversalClient) modifierDesiredEqualsLive(ctx context.Context, instanceUid string, desired map[string]string, cfsParams []universalCfsParam) (bool, error) { +func (c *UniversalClient) modifierDesiredEqualsLive(ctx context.Context, instanceUid string, desired map[string]string) (bool, error) { if len(desired) == 0 { return false, nil } @@ -41,17 +57,12 @@ func (c *UniversalClient) modifierDesiredEqualsLive(ctx context.Context, instanc if len(live) == 0 { return false, nil } - codeToParam := modifierCodeMap(cfsParams) for code, wanted := range desired { - param, ok := codeToParam[strings.ToLower(strings.TrimSpace(code))] + current, ok := live[strings.ToLower(strings.TrimSpace(code))] if !ok { return false, nil } - current, ok := lookupLiveParam(live, param) - if !ok { - return false, nil - } - if !modifierValuesEqual(wanted, current, param) { + if !modifierRawValuesEqual(wanted, current) { return false, nil } } diff --git a/provider/internal/core/modifier_compare_test.go b/provider/internal/core/modifier_compare_test.go index 5173c1c..de21126 100644 --- a/provider/internal/core/modifier_compare_test.go +++ b/provider/internal/core/modifier_compare_test.go @@ -6,53 +6,46 @@ import ( "testing" ) -func TestModifierValuesEqual_Scalars(t *testing.T) { +func TestModifierRawValuesEqual_Scalars(t *testing.T) { tests := []struct { name string wanted, current string - param universalCfsParam want bool }{ - {"bool совпал", "false", "false", universalCfsParam{DataType: "boolean"}, true}, - {"bool расхождение", "true", "false", universalCfsParam{DataType: "boolean"}, false}, - {"int с пробелом", " 3 ", "3", universalCfsParam{DataType: "integer > 0"}, true}, - {"int расхождение", "1", "3", universalCfsParam{DataType: "integer > 0"}, false}, - {"string совпал", "QoS-100Mbit", "QoS-100Mbit", universalCfsParam{DataType: "string"}, true}, + {"bool совпал", "false", "false", true}, + {"bool регистр", "True", "true", true}, + {"bool расхождение", "true", "false", false}, + {"int с пробелом", " 3 ", "3", true}, + {"int расхождение", "1", "3", false}, + {"null и пустое", "null", "", true}, + {"string совпал", "QoS-100Mbit", "QoS-100Mbit", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := modifierValuesEqual(tt.wanted, tt.current, tt.param); got != tt.want { - t.Errorf("modifierValuesEqual() = %v, want %v", got, tt.want) + if got := modifierRawValuesEqual(tt.wanted, tt.current); got != tt.want { + t.Errorf("modifierRawValuesEqual(%q, %q) = %v, want %v", tt.wanted, tt.current, got, tt.want) } }) } } -func TestModifierValuesEqual_JSON(t *testing.T) { - mapParam := universalCfsParam{DataType: "map-fixed"} - // map-fixed: порядок ключей не значим - if !modifierValuesEqual( +func TestModifierRawValuesEqual_JSON(t *testing.T) { + // JSON: порядок ключей не значим + if !modifierRawValuesEqual( `{"mainDns":"81.22.46.22","ipAddrPool":"10.10.102.0/24"}`, - `{"ipAddrPool":"10.10.102.0/24","mainDns":"81.22.46.22"}`, - mapParam) { - t.Errorf("map-fixed с разным порядком ключей должен считаться равным") + `{"ipAddrPool":"10.10.102.0/24","mainDns":"81.22.46.22"}`) { + t.Errorf("JSON с разным порядком ключей должен считаться равным") } - - arrParam := universalCfsParam{DataType: "array-map-fixed"} - // array-map-fixed: порядок элементов массива значим - if !modifierValuesEqual(`[{"name":"a","count":1},{"name":"b","count":2}]`, `[{"name":"a","count":1},{"name":"b","count":2}]`, arrParam) { - t.Errorf("array-map-fixed с тем же порядком должен быть равен") + // JSON-массив: порядок элементов значим + if !modifierRawValuesEqual(`[{"name":"a"}]`, `[{"name":"a"}]`) { + t.Errorf("эквивалентные JSON-массивы должны быть равны") } - if modifierValuesEqual(`[{"name":"a","count":1},{"name":"b","count":2}]`, `[{"name":"b","count":2},{"name":"a","count":1}]`, arrParam) { - t.Errorf("array-map-fixed с другим порядком не должен быть равен") + if modifierRawValuesEqual(`[{"name":"a"},{"name":"b"}]`, `[{"name":"b"},{"name":"a"}]`) { + t.Errorf("JSON-массивы с другим порядком не должны быть равны") } } func TestModifierDesiredEqualsLive(t *testing.T) { - cfsParams := []universalCfsParam{ - {SvcOperationCfsParamId: 372, Code: "ipSpaceName", DataType: "string"}, - } - newClient := func(liveParams string) (*UniversalClient, func()) { return makeTestClient(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -63,7 +56,7 @@ func TestModifierDesiredEqualsLive(t *testing.T) { t.Run("совпало с live", func(t *testing.T) { c, cleanup := newClient(`{"ipSpaceName":"internet-ipv4-v1"}`) defer cleanup() - equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}, cfsParams) + equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}) if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -75,12 +68,24 @@ func TestModifierDesiredEqualsLive(t *testing.T) { t.Run("отличается от live", func(t *testing.T) { c, cleanup := newClient(`{"ipSpaceName":"no-needed"}`) defer cleanup() - equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}, cfsParams) + equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}) if err != nil { t.Fatalf("unexpected err: %v", err) } if equal { - t.Errorf("expected not equal when live differs from desired") + t.Errorf("expected not equal when live differs") + } + }) + + t.Run("код не найден в live — не пропускаем", func(t *testing.T) { + c, cleanup := newClient(`{"otherParam":"x"}`) + defer cleanup() + equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if equal { + t.Errorf("must not report equal when code is absent in live") } }) @@ -89,7 +94,7 @@ func TestModifierDesiredEqualsLive(t *testing.T) { w.WriteHeader(http.StatusInternalServerError) }) defer cleanup() - equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}, cfsParams) + equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}) if err == nil { t.Errorf("expected error when live is unavailable") } @@ -98,3 +103,4 @@ func TestModifierDesiredEqualsLive(t *testing.T) { } }) } + diff --git a/provider/internal/core/operation_cfs.go b/provider/internal/core/operation_cfs.go index acfb361..fccb41e 100644 --- a/provider/internal/core/operation_cfs.go +++ b/provider/internal/core/operation_cfs.go @@ -74,21 +74,3 @@ func (c *UniversalClient) fetchOperationCfsParams(ctx context.Context, opUid str } return def.SvcOperation.CfsParams, nil } - -// fetchOperationSchemaByID возвращает схему операции по svcOperationId БЕЗ создания -// операции: GET /instanceOperations/default/{opId}. -// -// Зачем: idempotency pre-check обязан выполняться ДО POST /instanceOperations — иначе -// при пропуске останется созданная-но-незапущенная операция («черновик», pending), -// и следующий modify будет ждать idle до таймаута. -func (c *UniversalClient) fetchOperationSchemaByID(ctx context.Context, opId int) ([]universalCfsParam, error) { - defResp, _, err := c.doRequest(ctx, "GET", fmt.Sprintf("/instanceOperations/default/%d", opId), nil) - if err != nil { - return nil, fmt.Errorf("не удалось получить схему операции %d: %w", opId, err) - } - var def universalOpDefaultResponse - if uerr := json.Unmarshal(defResp, &def); uerr != nil { - return nil, fmt.Errorf("не удалось разобрать схему операции %d: %w", opId, uerr) - } - return def.SvcOperation.CfsParams, nil -} diff --git a/provider/internal/core/operation_run_bycode.go b/provider/internal/core/operation_run_bycode.go index 9e45c55..2e264ea 100644 --- a/provider/internal/core/operation_run_bycode.go +++ b/provider/internal/core/operation_run_bycode.go @@ -41,18 +41,12 @@ func (c *UniversalClient) runInstanceOperationByCode(ctx context.Context, instan return fmt.Errorf("операция %s недоступна для экземпляра %s", action, instanceUid) } - // Схема операции нужна и для idempotency pre-check, и для маппинга кодов. - // Получаем её ДО создания операции (GET /instanceOperations/default/{opId}): иначе - // при пропуске останется созданная, но незапущенная операция («черновик», pending). - schemaParams, err := c.fetchOperationSchemaByID(ctx, opId) - if err != nil { - return err - } - // Idempotency pre-check: если все desired уже равны LIVE-значениям инстанса — - // выходим, НЕ создавая операцию вовсе (источник — state.params, см. instanceLiveParams). + // выходим, НЕ создавая операцию вовсе. Схема операции здесь принципиально НЕ + // запрашивается: pre-check идёт по live-кодам (см. modifierDesiredEqualsLive и + // ARCHITECTURE.md «Modifier idempotency»). if idempotent { - equal, liveErr := c.modifierDesiredEqualsLive(ctx, instanceUid, params, schemaParams) + equal, liveErr := c.modifierDesiredEqualsLive(ctx, instanceUid, params) if liveErr == nil && equal { return nil }