Skip to content

fix consul engine bug when calling Set and List#283

Merged
git-hulk merged 8 commits intoapache:unstablefrom
bseto:bseto/consul-key-fix
Mar 19, 2025
Merged

fix consul engine bug when calling Set and List#283
git-hulk merged 8 commits intoapache:unstablefrom
bseto:bseto/consul-key-fix

Conversation

@bseto
Copy link
Contributor

@bseto bseto commented Mar 19, 2025

This PR fixes two bugs with the Consul Engine Implementation

  1. When trying to run the example with consul as the engine, I get an error about the key starting with a /
./_build/kvctl create namespace test-ns
error: Invalid key. Key must not begin with a '/': /kvrocks/metadata/test-ns
  1. Listing values from consul only produces 1 character results
./_build/kvctl list namespaces
t 

Setup

# running setup for the consul and other engines
make setup

# on another 3 terminals, I'm running 3 instances of kvrocks
./build/kvrocks -c kvrocks.conf --cluster-enabled yes --port 6001 --dir /tmp/kvrocks1
./build/kvrocks -c kvrocks.conf --cluster-enabled yes --port 6002 --dir /tmp/kvrocks1
./build/kvrocks -c kvrocks.conf --cluster-enabled yes --port 6003 --dir /tmp/kvrocks1

# run kvctl-server with config-consul, with the addr modified:  addr: "127.0.0.1:9379"
./_build/kvctl-server -c config/config-consul.yaml

# BUG 1
./_build/kvctl create namespace test-ns
error: Invalid key. Key must not begin with a '/': /kvrocks/metadata/test-ns

# after fixing bug 1, continuing with the example and listing the namespace
# BUG 2
./_build/kvctl list namespaces
t

continue
}
key := strings.TrimLeft(string(kv.Key[prefixLen+1]), "/")
key := strings.TrimLeft(string(kv.Key[prefixLen+1:]), "/")
Copy link
Contributor Author

@bseto bseto Mar 19, 2025

Choose a reason for hiding this comment

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

Bug 2 fix. I noticed that this was missing the : which I could find in the etcd and other implementations

Comment on lines +321 to +323
if len(key) > 0 && key[0] == '/' {
key = strings.TrimPrefix(key, "/")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

trimming the leading / to avoid the error
https://github.com/hashicorp/consul/blob/main/api/kv.go#L208-L210


ctx := context.Background()
keys := []string{"a/b/c0", "a/b/c1", "a/b/c2"}
keys := []string{"/a/b/c0", "/a/b/c1", "/a/b/c2"}
Copy link
Contributor Author

@bseto bseto Mar 19, 2025

Choose a reason for hiding this comment

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

the error really happened because the nsPrefix has a leading /, but I think if we just keep the tests consistent with the other engines and it'll be ok?

@bseto bseto marked this pull request as ready for review March 19, 2025 12:58
@bseto bseto changed the title Bseto/consul key fix fix consul engine bug when calling Set and List Mar 19, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 46.34%. Comparing base (6c56470) to head (3b365b9).
⚠️ Report is 77 commits behind head on unstable.

Files with missing lines Patch % Lines
store/engine/consul/consul.go 90.90% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #283      +/-   ##
============================================
+ Coverage     43.38%   46.34%   +2.95%     
============================================
  Files            37       45       +8     
  Lines          2971     4296    +1325     
============================================
+ Hits           1289     1991     +702     
- Misses         1544     2097     +553     
- Partials        138      208      +70     
Flag Coverage Δ
unittests 46.34% <90.90%> (+2.95%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@git-hulk
Copy link
Member

git-hulk commented Mar 19, 2025

@bseto Great! Thanks for your fix, will have a look soon.

@git-hulk git-hulk requested a review from Copilot March 19, 2025 14:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes two critical issues in the Consul engine implementation: ensuring that keys do not start with a leading slash and correcting the key trimming logic in the List function to return complete namespace names.

  • Updated Consul configuration to point to the correct Consul agent address.
  • Modified engine methods (Get, Exists, Set, Delete, List) to sanitize keys using a new helper function.
  • Adjusted tests to provide keys with a leading slash, verifying that key sanitization works as expected.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
config/config-consul.yaml Updated Consul address to match test setup requirements.
store/engine/consul/consul.go Introduced key sanitization in Consul methods and fixed key slicing in List.
store/engine/consul/consul_test.go Modified test keys to include a leading slash to trigger sanitization.

@git-hulk git-hulk merged commit 1be45f1 into apache:unstable Mar 19, 2025
4 checks passed
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.

4 participants