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.

Sign up for GitHub

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

Jump to bottom

Fix/syndic/62618 - external auth #63257

Closed
wants to merge 20 commits into from
Closed

Fix/syndic/62618 - external auth #63257

wants to merge 20 commits into from

Conversation

waynew
Copy link
Contributor

@waynew waynew commented Dec 8, 2022 •
edited
Loading

What does this PR do?

Fixes #62618 - syndic eauth

Previous Behavior

Syndics didn't work with eauth... now they do. It also fixes #62577. And adds a fairly comprehensive integration test suite for syndics.

Merge requirements satisfied?

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

Commits signed with GPG?

Yes

I'm definitely going to be refactoring chunks of this, but I wanted to get this PR open in case anyone had comments for stuff that either I've missed or could do better.

Bits I still have planned:

  • - remove/redo logging
  • - consider how I'm firing the event in _list_valid_minions - is there a better way to get that from the syndic master to the salt-syndic service?
  • - refactor the do_something and order_masters bits of code around that.
  • - restore some of the authorization error messages
  • - adjust the path for communication if I can't figure out a way to get it on the event bus
  • - take a closer look at the AsyncReqChannel that enables returns. It may or may not be the best way to do that.

waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/channel/client.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/client/__init__.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/client/__init__.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/client/__init__.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/crypt.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/minion.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/minion.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/minion.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/minion.py Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/modules/state.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/returners/local_cache.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/transport/ipc.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 8, 2022
View reviewed changes
salt/utils/minions.py Outdated Show resolved Hide resolved
@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Dec 8, 2022
@waynew waynew marked this pull request as ready for review December 9, 2022 02:03
@waynew waynew requested a review from a team as a code owner December 9, 2022 02:03
@waynew waynew requested review from whytewolf and removed request for a team December 9, 2022 02:03
waynew
waynew commented Dec 12, 2022
View reviewed changes
salt/master.py Show resolved Hide resolved
waynew
waynew commented Dec 12, 2022
View reviewed changes
salt/master.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 12, 2022
View reviewed changes
salt/client/__init__.py Outdated Show resolved Hide resolved
waynew
waynew commented Dec 12, 2022
View reviewed changes
salt/minion.py Show resolved Hide resolved
salt/returners/local_cache.py Show resolved Hide resolved
salt/utils/minions.py Show resolved Hide resolved
salt/utils/minions.py Show resolved Hide resolved
dwoz
dwoz reviewed Dec 13, 2022
View reviewed changes
salt/minion.py Outdated Show resolved Hide resolved
salt/client/__init__.py Outdated Show resolved Hide resolved
salt/client/__init__.py Outdated Show resolved Hide resolved
salt/master.py Outdated Show resolved Hide resolved
salt/master.py Show resolved Hide resolved
@waynew
Copy link
Contributor Author

waynew commented Dec 13, 2022

@Ch3LL changelog files have been added. I think I may have pushed them before but I was trying to rebase and I lost them in the shuffle 😓

And I haven't completely rebased either 😂 😅

@waynew waynew force-pushed the fix/syndic/62618 branch 2 times, most recently from 28fe95f to ffb8afe Compare December 14, 2022 01:40
@waynew
Skip if docker not found
0fe8db6 
Do our tests care we cannot connect to docker in the environment? We do
not. If docker can't get the client to connect it's OK, we'll just skip
the tests. At least *one* of our platforms should be running these
tests, which is enough.
@waynew
Also skip when keys cannot be accepted
d40ae8d
@waynew
Rewrite warning
ed5389a
@waynew
Another test skip
39b079c 
No need to fail when we can't clean up. VMs *should* be going away, and
if you're running locally you can clean up yourself 👍
twangboy
twangboy requested changes Dec 16, 2022
View reviewed changes
Copy link
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.

Just a minor docs change

doc/topics/topology/syndic.rst Outdated Show resolved Hide resolved
@waynew waynew added this to the Sulphur v3006.0 milestone Dec 16, 2022
@waynew waynew mentioned this pull request Dec 16, 2022
Closed
@Ch3LL Ch3LL requested a review from dwoz December 16, 2022 19:31
Ch3LL
Ch3LL previously approved these changes Dec 16, 2022
View reviewed changes
@waynew
Copy link
Contributor Author

waynew commented Dec 16, 2022

Looks like at least one of these failures is 9f95af8

twangboy
twangboy previously approved these changes Dec 16, 2022
View reviewed changes
@davama davama mentioned this pull request Dec 18, 2022
Closed
2 tasks
@waynew waynew mentioned this pull request Dec 22, 2022
Merged
3 tasks
@dwoz dwoz mentioned this pull request Jan 7, 2023
Merged
3 tasks
@Ch3LL
Copy link
Contributor

Ch3LL commented Jan 12, 2023

@waynew can we close this one in favor of #63428 ?

@waynew
Copy link
Contributor Author

waynew commented Jan 13, 2023

Yup - #63428 deprecates? obviates? the need for this PR.

@waynew waynew closed this Jan 13, 2023
@waynew waynew deleted the fix/syndic/62618 branch January 13, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anilsil anilsil anilsil left review comments

@twangboy twangboy twangboy approved these changes

@whytewolf whytewolf whytewolf approved these changes

@Ch3LL Ch3LL Ch3LL left review comments

@dwoz dwoz Awaiting requested review from dwoz

Assignees
No one assigned
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Milestone
Sulfur v3006.0
Development

Successfully merging this pull request may close these issues.

[BUG]External Auth with Syndic not Working as Expected [BUG] 3005 broke syndication
7 participants
@waynew @Ch3LL @whytewolf @dwoz @anilsil @twangboy @garethgreenaway