davissp14 / etcdv3-ruby

Etcd v3 Ruby Client
MIT License
52 stars 17 forks source link

BUGFIX: pass metadata to locking APIs #138

Closed forestgagnon closed 2 years ago

forestgagnon commented 2 years ago

I am trying to use this gem against an etcd deployment protected by username/password authentication. Some operations work fine, but there is a bug in the locking methods that causes a mysterious "user name is empty" error, even though the client instance is configured with a username and password and works for other operations like get or lease_grant.

After some digging I noticed that for the locking APIs, there is a metadata object which is not being passed through by the wrappers to the protobuf functions. This object is passed though by the other wrappers I looked at, so it appeared to be an oversight specific to the addition of the locking API.

This change fixes the bugs I ran into and adds some regression tests for them.

codecov[bot] commented 2 years ago

Codecov Report

Merging #138 (26fd6a9) into master (d47a9a8) will increase coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   99.19%   99.20%   +0.01%     
==========================================
  Files          39       40       +1     
  Lines        2115     2149      +34     
==========================================
+ Hits         2098     2132      +34     
  Misses         17       17              
Impacted Files Coverage Δ
lib/etcdv3/lock.rb 100.00% <100.00%> (ø)
lib/etcdv3/namespace/lock.rb 100.00% <100.00%> (ø)
spec/etcdv3/lock_spec.rb 100.00% <100.00%> (ø)
spec/etcdv3/namespace/lock_spec.rb 100.00% <100.00%> (ø)
spec/helpers/connections.rb 95.83% <100.00%> (+0.83%) :arrow_up:
spec/helpers/metadata_passthrough.rb 100.00% <100.00%> (ø)
spec/helpers/test_instance.rb 87.50% <100.00%> (+0.40%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d47a9a8...26fd6a9. Read the comment docs.

davissp14 commented 2 years ago

Hey, thanks for this!

I'm not sure what version you're running, but there was a known bug in Etcd that would invalidate basic auth tokens when the auth revision changed. JWT tokens were also broken last time I checked. 🙄

https://github.com/etcd-io/etcd/pull/13262

forestgagnon commented 2 years ago

interesting. I've been testing against a brand new 3.5.1 etcd cluster. I suspect it is not related to that issue but could be wrong, I am just getting started with etcd and am not an expert on it. However I do know that this code change fixes the bug on my end, I'm able to call the lock API with username/password auth now.

forestgagnon commented 2 years ago

here's a code snippet:

#!/usr/bin/env ruby

require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  # gem 'etcdv3', '0.11.4',
  gem 'etcdv3', git: 'https://github.com/forestgagnon/etcdv3-ruby', branch: 'fix-authenticated-locks'
end

require 'etcdv3'
client = Etcdv3.new(endpoints: 'http://127.0.0.1:2379', user: 'root', password: ENV.fetch("ETCD_ROOT_PASSWORD"))

begin 
  lease_id = client.lease_grant(20)['ID']
  lock_key = client.lock('some-lock', lease_id, timeout: 10).key
  puts lock_key
  sleep 2
  puts "do thing 1"
  sleep 5
  puts "do thing 2"
ensure
  client.unlock(lock_key, timeout: 10)
end

0.11.4 succeeds up to client.lock, the version from this branch successfully completes the script

davissp14 commented 2 years ago

Thanks again. Just cut a new release with your changes.

forestgagnon commented 2 years ago

appreciate the quick release!