Skip to content

Make the Salt Proxy environment aware#56222

Closed
bdrung wants to merge 2 commits intosaltstack:masterfrom
bdrung:make-salt-proxy-environment-aware
Closed

Make the Salt Proxy environment aware#56222
bdrung wants to merge 2 commits intosaltstack:masterfrom
bdrung:make-salt-proxy-environment-aware

Conversation

@bdrung
Copy link
Copy Markdown
Contributor

@bdrung bdrung commented Feb 21, 2020

The Salt proxy minion is looking for proxy minion modules in salt://_proxy/. It does so however only in the default base environment. On setups which do not use base or shall be executed in a different environment this breaks:

/etc/salt/master:

...
file_roots:
  noc:
    - /srv/salt
...
$ grep "proxyenabled" /srv/salt/_proxy/junos_manager.py
__proxyenabled__ = ['junos_manager']

$ salt-proxy --proxyid=dev1 -l debug
...
[DEBUG   ] rest_sample proxy __virtual__() called...
[INFO    ] ssh_sample proxy __virtual__() called...
[DEBUG   ] Could not LazyLoad junos_manager.grains
[DEBUG   ] Could not LazyLoad junos_manager.init
[ERROR   ] Proxymodule junos_manager is missing an init() or a
shutdown() or both. Check your proxymodule.  Salt-proxy aborted.
[WARNING ] Stopping the Salt Proxy Minion
[ERROR   ] -1
[INFO    ] The proxy minion is shutting down..
[INFO    ] The Salt ProxyMinion is shut down

This is because the loader only looks for _proxy modules in the base environment. This commit fixes this (but might possibly break other things, though I did not find side-effects).

The initial pull request #36704 were merged quite some time ago, but one part of it is missing in the 2019.2 release (again?).

@bdrung bdrung requested a review from a team as a code owner February 21, 2020 11:56
@ghost ghost requested a review from twangboy February 21, 2020 11:56
twangboy
twangboy previously approved these changes Mar 5, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label Mar 23, 2020
@bdrung bdrung force-pushed the make-salt-proxy-environment-aware branch from a5556d4 to 4315cab Compare April 7, 2020 10:36
@bdrung bdrung force-pushed the make-salt-proxy-environment-aware branch from a058a63 to afe5a78 Compare April 14, 2020 13:54
@twangboy
Copy link
Copy Markdown
Contributor

Hey, @bdrung

Looks like this needs to be rebased and have pre-commit run on it. Do you mind doing that?

https://docs.saltstack.com/en/latest/topics/development/contributing.html#quickstart

@bdrung
Copy link
Copy Markdown
Contributor Author

bdrung commented Apr 15, 2020

I rebased it yesterday and pre-commit was successfully run.

@twangboy
Copy link
Copy Markdown
Contributor

Sweet... now we need to figure out why these tests are failing

@bdrung
Copy link
Copy Markdown
Contributor Author

bdrung commented Apr 16, 2020

Only codecov/project is failing, which makes no sense since this merge request just changes one line.

@waynew
Copy link
Copy Markdown
Contributor

waynew commented Apr 16, 2020

codecov can be ignored - on the newest master it should be.

Question: Why is it "saltenv" on this branch, but "environment" on the develop branch PR?

@bdrung
Copy link
Copy Markdown
Contributor Author

bdrung commented Apr 17, 2020

In one salt release, the environment option was replaced by saltenv.

@dwoz dwoz added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Apr 19, 2020
waynew
waynew previously approved these changes Apr 21, 2020
@bdrung bdrung force-pushed the make-salt-proxy-environment-aware branch 6 times, most recently from 132df01 to 1b5fbbd Compare April 30, 2020 10:30
twangboy
twangboy previously approved these changes May 21, 2020
@twangboy
Copy link
Copy Markdown
Contributor

@bdrung Unless this gets a test today, it's not going to make it into Sodium.

The Salt proxy minion is looking for proxy minion modules in
`salt://_proxy/`. It does so however only in the default `base`
environment. On setups which do not use `base` or shall be executed in a
different environment this breaks:

/etc/salt/master:
```
...
file_roots:
  noc:
    - /srv/salt
...
```

```
$ grep "proxyenabled" /srv/salt/_proxy/junos_manager.py
__proxyenabled__ = ['junos_manager']

$ salt-proxy --proxyid=dev1 -l debug
...
[DEBUG   ] rest_sample proxy __virtual__() called...
[INFO    ] ssh_sample proxy __virtual__() called...
[DEBUG   ] Could not LazyLoad junos_manager.grains
[DEBUG   ] Could not LazyLoad junos_manager.init
[ERROR   ] Proxymodule junos_manager is missing an init() or a
shutdown() or both. Check your proxymodule.  Salt-proxy aborted.
[WARNING ] Stopping the Salt Proxy Minion
[ERROR   ] -1
[INFO    ] The proxy minion is shutting down..
[INFO    ] The Salt ProxyMinion is shut down
```

This is because the loader only looks for _proxy modules in the `base`
environment. This commit fixes this (but might possibly break other
things, though I did not find side-effects).

Initial pull request: saltstack#36704
Forwarded: saltstack#55932
Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com>
@bdrung bdrung force-pushed the make-salt-proxy-environment-aware branch from b402205 to e8a75e5 Compare December 11, 2021 14:31
@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Oct 18, 2022

@bdrung would you be willing to add a changelog and test coverage?

@twangboy twangboy self-requested a review November 14, 2022 20:43
Copy link
Copy Markdown
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

Please add a changelog and a test case

@dwoz
Copy link
Copy Markdown
Contributor

dwoz commented Dec 10, 2023

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

@dwoz dwoz closed this Dec 10, 2023
@dwoz dwoz added the Abandoned label Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Abandoned has-failing-test help-wanted Community help is needed to resolve this needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants