Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: pass metadata to locking APIs #138

Merged
merged 2 commits into from
Dec 1, 2021
Merged

BUGFIX: pass metadata to locking APIs #138

merged 2 commits into from
Dec 1, 2021

Conversation

forestgagnon
Copy link
Contributor

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
Copy link

codecov bot commented Dec 1, 2021

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%) ⬆️
spec/helpers/metadata_passthrough.rb 100.00% <100.00%> (ø)
spec/helpers/test_instance.rb 87.50% <100.00%> (+0.40%) ⬆️

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
Copy link
Owner

davissp14 commented Dec 1, 2021

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. 🙄

etcd-io/etcd#13262

@forestgagnon
Copy link
Contributor Author

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
Copy link
Contributor Author

forestgagnon commented Dec 1, 2021

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 davissp14 merged commit c5d673c into davissp14:master Dec 1, 2021
@davissp14
Copy link
Owner

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

@forestgagnon
Copy link
Contributor Author

appreciate the quick release!

@forestgagnon forestgagnon deleted the fix-authenticated-locks branch January 7, 2022 17:05
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.

2 participants