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

Add etcd API v3 implementation #61911

Merged
merged 29 commits into from
May 31, 2022
Merged

Add etcd API v3 implementation #61911

merged 29 commits into from
May 31, 2022

Conversation

MKLeb
Copy link
Contributor

@MKLeb MKLeb commented Apr 2, 2022

What does this PR do?

Adds an implementation for etcd API v3.

Notes: The libraries are completely incompatible out of the box, the names for similar queries are completely different (which, albeit, is expected). However, etcd v2 and v3 are fundamentally incompatible with each other. Their structures are different: v2 has a hierarchical construction, meaning it essentially reads like a file system; v3 has no directories, and thus no hierarchy (by design) - its keys are simply interpreted as strings.

This PR allows the use of both APIs, but there are certainly some improvements that could be made (not done here due to time constraints). Right now, we have a base superclass sitting underneath both API "adapters". However, the ideal way to do this would be to have the base construction (and all common code) factored out to the top level, using the adapter pattern underneath for all of the fundamental functionality and ensuring they return consistent output to the higher-level object. Basically, we want the adapters to only implement the CRUD functionality of etcd (and also the watch idea). Then, the single higher-level object would assume their results follow a consistent pattern and would be able to use common code refactored out to do the rest of the processing.

What issues does this PR fix or reference?

Fixes: #60325 and all sub-issues of it

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@MKLeb MKLeb requested a review from a team as a code owner April 2, 2022 17:25
@MKLeb MKLeb requested review from Ch3LL and removed request for a team April 2, 2022 17:25
@MKLeb MKLeb requested a review from waynew April 11, 2022 14:35
@MKLeb MKLeb changed the title [WIP] Add etcd API v3 implementation Add etcd API v3 implementation Apr 11, 2022
@Ch3LL Ch3LL added the Phosphorus v3005.0 Release code name and version label Apr 11, 2022
twangboy
twangboy previously approved these changes Apr 11, 2022
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

I think this is probably mostly good, got a few questions, and also would like to poke at this

requirements/static/ci/common.in Show resolved Hide resolved
salt/modules/etcd_mod.py Show resolved Hide resolved
salt/pillar/etcd_pillar.py Show resolved Hide resolved
salt/utils/etcd_util.py Outdated Show resolved Hide resolved
salt/utils/etcd_util.py Outdated Show resolved Hide resolved
salt/utils/etcd_util.py Outdated Show resolved Hide resolved
salt/utils/etcd_util.py Outdated Show resolved Hide resolved
salt/utils/etcd_util.py Show resolved Hide resolved
tests/pytests/functional/modules/test_etcd_mod.py Outdated Show resolved Hide resolved
salt/utils/etcd_util.py Outdated Show resolved Hide resolved
twangboy
twangboy previously approved these changes Apr 19, 2022
Ch3LL
Ch3LL previously approved these changes Apr 19, 2022
@Ch3LL Ch3LL requested a review from waynew April 19, 2022 13:35
@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 28, 2022

@waynew can you re-review this PR please

@MKLeb MKLeb dismissed stale reviews from Ch3LL and twangboy via 6c140c8 April 30, 2022 00:02
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

I haven't checked it out and banged on it a bit, so it's entirely possible that I'm missing something else there, I did catch a couple of things from a visual inspection - otherwise this mostly looks 👍

salt/utils/etcd_util.py Outdated Show resolved Hide resolved
salt/utils/etcd_util.py Show resolved Hide resolved
@waynew
Copy link
Contributor

waynew commented May 11, 2022

Okay, leaving this mostly as a note to self - all identified code changes look like they've been resolved, I'm checking out now and gonna bang on things a bit 👍

@waynew
Copy link
Contributor

waynew commented May 11, 2022

okay, this took me a while to actually get setup and running on my new system, but I was able to give it a cursory glance and it's looking good. I want to spend a bit more time banging on it tomorrow before I 👍 it

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Okay, finally getting to bang on this a bit and I think that something
is off, but I'm not entirely sure if it's my expectation or something else
😂

I'm starting with this docker container:

docker run --rm -it -e ALLOW_NONE_AUTHENTICATION=yes -e ETCD_ENABLE_V2=true --publish 2379:2379 --publish 2380:2380 bitnami/etcd:3

Then I have the following pillar data:

top

base:
  'kevin':
    - 61911_etcd

61911_etcd.sls

some_etcd_config:
  etcd.host: 127.0.0.1
  etcd.port: 2379
  etcd.require_v2: True

And the following states:

top

base:
  'kevin':
    - 61911/base

61911/base.sls

{% if salt['config.get']('some_etcd_config:etcd.require_v2') %}
make with a directory:
  etcd.directory:
    - name: some cool dirname/with some spaces/and a couple of things deep
    - profile: some_etcd_config
{%- endif %}

basic key set:
  etcd.set:
    - name: so_basic
    - value: vanilla ice
    - profile: some_etcd_config

set this key yo:
  etcd.set:
    - name: some/cool/key
    - value: more keys are keyful

some deleted watch:
  etcd.wait_rm:
    - name: del/taco
    - recurse: True

some set watch:
  etcd.wait_set:
    - name: buh/leeted
    - value: this should only set if watch'd

create a thing lulz:
  etcd.set:
    - name: del/taco/del/mar
    - value: goodbye
    - watch_in:
      - etcd: some deleted watch
      - etcd: some set watch

nope:
  etcd.rm:
    - name: del/taco
    - recurse: True

Now, when I have v2 enabled, I can run this with salt kevin state.apply and
all works. I can even run a couple of modules as well:

 salt kevin etcd.get so_basic
 salt kevin etcd.get some/cool/key
 salt kevin etcd.get "some cool dirname/with some spaces/and a couple of things deep"
 salt kevin etcd.ls /
 salt kevin etcd.ls /some/cool
 salt kevin etcd.ls /some/cool/key
 salt kevin etcd.tree /some
 salt kevin etcd.tree /some/cool

 salt kevin etcd.rm /some/cool/key
 salt kevin etcd.watch /some/cool/key timeout=5
 salt kevin etcd.watch /some/cool/key  # then re-run the states

 salt kevin etcd.watch /some/cool/key
 # then run this next one in a different window
 salt kevin etcd.update /some/cool/key awesome

And all of those work. Super swell!

However, when I swap to v3:

sed -i 's/True/False/' srv/pillar/61911_etcd.sls
docker run --rm -it -e ALLOW_NONE_AUTHENTICATION=yes -e ETCD_ENABLE_V2=false --publish 2379:2379 --publish 2380:2380 bitnami/etcd:3

Then running a state.apply fails with a bunch of etcd.EtcdException: Bad Response : 404 page not found. So there's definitely something going on here - not sure if it's just some misconfiguration on my part, but we probably shouldn't be showing that exception to the user anyway.

I can show you what I did tomorrow if that would be helpful 👍

salt/states/etcd_mod.py Outdated Show resolved Hide resolved
@MKLeb
Copy link
Contributor Author

MKLeb commented May 18, 2022

re-run all

MKLeb and others added 4 commits May 26, 2022 14:46
Previously, when the API was v3-only, but the profile wasn't present
leading to the v2 fallback, it would 404 in certain circumstances.

We can't use a more particular exception in the `_etcd_action` because
the salt loader plays havoc with things.
@Ch3LL Ch3LL merged commit 5a18c14 into saltstack:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TECH DEBT] support etcd api v3
5 participants