Skip to content

fix(patch): keep package secrets when no kv operation applies - #467

Open
sakurai-youhei wants to merge 1 commit into
elastic:mainfrom
sakurai-youhei:fix/patch-kv-preserve-secrets
Open

fix(patch): keep package secrets when no kv operation applies#467
sakurai-youhei wants to merge 1 commit into
elastic:mainfrom
sakurai-youhei:fix/patch-kv-preserve-secrets

Conversation

@sakurai-youhei

@sakurai-youhei sakurai-youhei commented Aug 23, 2026

Copy link
Copy Markdown
Member

Issue:

See #466

Cause:

applySecretKVPatch starts with a nil result and only assigns it inside one of the four operation branches. A kv block that triggers no operation — an empty block, or removeKeys whose pattern matches no key in the package — leaves the result nil, and the caller assigns that nil over the package's existing secrets.

Fix:

Initialise the result with the incoming secret list, so a kv block that triggers no operation returns the package secrets unchanged.

Verification:

$ echo '{"app/target":{"k":"v","k2":"v2"}}' | harp from jsonmap > in.bundle
$ cat > empty-kv.yaml <<'EOF'
apiVersion: harp.elastic.co/v1
kind: BundlePatch
meta:
  name: empty-kv
spec:
  rules:
    - selector:
        matchPath:
          regex: "^app/"
      package:
        data:
          kv: {}
EOF
$ bin/harp-linux-amd64 version
cmd/harp/v0.2.8-142-g3e8f8c6 [fix/patch-kv-preserve-secrets:3e8f8c6] (Go: go1.25.8 linux/amd64, Flags: defaults, Date: 2026-08-23T23:32:42Z)
$ bin/harp-linux-amd64 bundle patch --in in.bundle --spec empty-kv.yaml > out.bundle
$ harp bundle dump --in out.bundle --data-only
{"app/target":{"k":"v","k2":"v2"}}
# ↑ The built `bin/harp-linux-amd64` doesn't wipe any package secrets even though no operation is specified.

Closes #466

@sakurai-youhei
sakurai-youhei marked this pull request as ready for review August 23, 2026 03:19
@sakurai-youhei
sakurai-youhei requested a review from a team as a code owner August 23, 2026 03:19

@robinverduijn robinverduijn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this and traced the fix through applySecretKVPatch. The diagnosis in the description is accurate: every operation branch (Remove, Add, Update, ReplaceKeys) reads from kv and overwrites out, so the initializer only takes effect when no branch fires. That is exactly the empty kv: {} and zero-match removeKeys cases from #466, where the old nil result was written over the package secrets. out := kv returns them unchanged, and it is safe: in the no-op path out, kv, and the caller's secrets.Data are the same slice, so the write-back is a real no-op, and the branch paths still pass kv explicitly so their behavior does not change.

I ran the package both ways to confirm the tests are load-bearing: both new cases fail against the pre-fix line and pass with it, and the full pkg/bundle/patch suite is green.

One pre-existing, out-of-scope note (not a blocker here): the operation branches do not compose. Each reads the original kv rather than the accumulated out, so a single kv block that sets more than one operation (for example add plus update, or a matching removeKeys plus add) keeps only the last-fired branch's result and discards the rest. This PR neither introduces nor worsens that, since it is independent of out's initial value. Might be worth a separate issue if multi-operation kv blocks are meant to be supported.

The fix looks correct to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

data.kv wipes all package secrets when no operation matches

2 participants