Repository navigation
fix(rest): refuse props edits on create doors, honour definitions_only, drop tracker numbers - #216
Conversation
|
Important Review skippedToo many files! This PR contains 103 files, which is 3 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
a963c86 to
8a65508
Compare
8a65508 to
af675d1
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The refusals and the definitions_only branch are correct. Two smaller things block.
-
Five satellite log lines still carry tracker numbers, and the new test doesn't catch them. The pattern in
operator_messages_test.go:20matchesBugbut notBUG, sopkg/satellite/reconciler.gostill logsBUG-028at lines 2536 and 3334, andBUG-048at 2891, 2899 and 2984. They are the same kind oflog.FromContext(ctx).Infoline this PR rewrites forBug 360andBug 366. With(?i)added, the test reports exactly these five. It leaves the persisted annotation keybug136.blockstor.cozystack.io/...alone, since nothing separatesbugfrom136. I also tried a wider pattern (lower case,bug#,phase10,scenario N) and found nothing else. -
The two new test files have no
// SPDX-License-Identifier: Apache-2.0header. AGENTS.md asks for it on every blockstor-authored Go file, and these are the only.gofiles underpkg,internalandcmdwithout one.
Things I checked and found fine:
- No real client sends the props-modify fields on these two calls. golinstor v0.60.0's
ResourceDefinitionCreateandResourceGroupSpawndon't have them. python-linstor doesn't send them. linstor-csi never calls spawn at all: it usesResourceDefinitions.Createwith an empty envelope. Upstream'sJsonGenTypesfor both calls don't declare these fields either, so upstream already refuses them as unknown properties. The refusal useswriteErrorlike the other 400s, so golinstor decodes it. definitions_onlynow matches upstream'sspawnResourceDefinition: 201, definition and volumes created, no autoplace.- Each mutation turns its test red: reverting either refusal, dropping any
switchcase, removing thedefinitions_onlybranch, and putting(Bug 180)back into a literal. - No Go test, e2e script or replay YAML matches the old wording of the rewritten strings, so the renames don't change behaviour.
CI never ran on af675d1. The force-push came two seconds before the base switched to main, and GitHub still shows the PR as conflicting. The compare API says 2 ahead and 0 behind main, and a local merge is clean, so pushing again should get both a fresh mergeability check and a CI run. The previous run on 8a65508 (same two patches, older base) failed TestGroupKWFSpawnAndDependentReAutoplace and recovery-deleting-convert. Both have failed on unrelated branches too (runs 40125502076, 40125493977, 40334484413). The spawn test runs spawn-resources without --definition-only and never reaches the new branch. DCO complains about 1652387, which is the squashed #190 commit on main, not one of the commits here.
Non-blocking:
- The CLI flag is
--definition-only(seersc_grp_cmds.pyin linstor-client). The comment atcreate_doors_props_modify_test.go:80and the PR body say--definitions-only. pkg/api/v1/resource_group.go:101still says spawn is "the call linstor-csi uses to actually create a Resource from a group". That isn't true, and this PR already removed the same claim fromspawn.go.- There are text conflicts with #215 in
cmd/controller/main.go(the--enable-rest-apihelp) and with #192 and #222 invd_passphrase_bug_233.go. The other open PRs merge cleanly.
| // "Bug 174", "Bug-020" and "Phase 12". They mean nothing to an operator | ||
| // reading an error, an event or a log line, so no string literal in the | ||
| // shipped code carries one; comments may. | ||
| var internalTrackerRef = regexp.MustCompile(`\bBug[- ][0-9]|\bPhase [0-9]`) |
There was a problem hiding this comment.
This misses BUG-028/BUG-048, which still ship in five reconciler.go log lines. (?i) catches them and still leaves the bug136. annotation key alone.
| @@ -0,0 +1,72 @@ | |||
| package rest | |||
There was a problem hiding this comment.
Missing // SPDX-License-Identifier: Apache-2.0 header.
| @@ -0,0 +1,159 @@ | |||
| package rest | |||
There was a problem hiding this comment.
Missing // SPDX-License-Identifier: Apache-2.0 header.
|
Thanks, head c386940. The pattern is case-insensitive now, the five I'll resolve the text conflicts with #215 and #222 on whichever lands second. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) Both blockers from my last review are fixed at c386940. If I put a BUG-048 literal back into a reconciler.go log line, the tracker test fails, and it passes again without the (?i). The SPDX headers are in, and the new shell and replay files match their neighbours, which carry no header either.
I re-read the whole diff and ran the tests on a trial merge with main at 7bb6606. The new tests pass, and each of the three fixes fails its own test when I disable it. The other pkg/rest failures are the known 2s server-shutdown flake.
The red E2E lane 3 is recovery-deleting-convert (exit 2). It fails the same way on f64cf3b from #217 and on d9d2c29, and neither contains these commits. The scenario only sends override_props to the modify PUT, which this PR doesn't touch, so I don't think the diff causes it.
Two things that don't block. #215 touches the neighbouring flag help string in cmd/controller/main.go, so whichever PR lands second gets a small conflict there. CRD field descriptions still say "Phase 10.x", and operators see those in kubectl explain; that is outside the errors, events and logs this PR covers, so a follow-up is fine.
rd create and rg spawn decoded override_props, delete_props and
delete_namespaces and dropped them, answering success for an edit that
never happened. A create has no props to edit yet, and neither
golinstor nor python-linstor sends these fields there, so a non-empty
one is now refused with 400 naming the field and where props go
instead.
rg spawn also ignored definitions_only and placed replicas anyway; it
now stops after the definition and its volumes.
Error messages, events and log lines no longer carry internal tracker
numbers ("Bug 174", "pending Phase 12"). A test walks the shipped
string literals and fails on one. The src_snap_name refusal says what
is unsupported instead of naming a phase, and the stale comment that
called the field a no-op is gone.
Fixes #210
Fixes #211
Assisted-by: LLM
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
`linstor rg spawn-resources --definition-only` placed replicas before the fix. An L6 cli-matrix cell and an L7 replay check that the definition and its volume exist and that no replica appears over a hold window. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…ines Five satellite log lines still carried BUG-028 and BUG-048, which the test missed because it matched the mixed-case spelling only. The test now matches in any case, and the two new test files carry the license header. The spawn payload comment no longer says linstor-csi calls it. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
c386940 to
1ac8621
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) Still LGTM, only the rebase changed since c386940. The conflict with #215 in the --enable-rest-api help keeps the loopback default. TestPlainListenerFlagUsesTheLoopbackDefault passes, and fails if I put ":3370" back. CI is green on 1ac8621.
rd createandrg spawndecodedoverride_props,delete_propsanddelete_namespacesand dropped them, answering success for an edit that never happened. A create has no props to edit yet, and neither golinstor nor python-linstor sends these fields on either call, so a non-empty one is now refused with 400 naming the field and where props go instead. With #190 this covers what #202 lists for spawn.partialis still decoded and not acted on; that predates this PR and stays open.rg spawnalso ignoreddefinitions_only(linstor rg spawn --definition-only) and placed replicas anyway. It now stops after the definition and its volumes.Operator-facing strings no longer carry internal tracker numbers: errors, events and log lines said things like "(Bug 174)" or "pending Phase 12". The
src_snap_namerefusal now says what is not supported, and the comment that still called that field a no-op is gone. A test walks every string literal inpkg,internalandcmdand fails on a tracker number, so new ones don't creep back.Fixes #210
Fixes #211
Testing
definitions_onlybranch were reverted in turn, and the named test fails each time.rg-spawn-definitions-only) pin the definitions-only spawn at operator-CLI level. Not run on a stand.go test ./...: green apart from the knownpkg/restshutdown flake.