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 pillar support for vault policy assignment #62889

Closed
wants to merge 4 commits into from

Conversation

PhilippLemke
Copy link

What does this PR do?

SALT.MODULES.VAULT:

  • Add pillar support for vault policy assignment

Previous Behavior

  • Assignment of templated vault policies only via grains

New Behavior

  • Assignment of templated vault policies via grains and pillar

Merge requirements satisfied?

  • [ x] Docs
  • [ x] Tests written/updated

Commits signed with GPG?

  • no

@PhilippLemke PhilippLemke requested a review from a team as a code owner October 14, 2022 10:11
@PhilippLemke PhilippLemke requested review from Ch3LL and removed request for a team October 14, 2022 10:11
@welcome
Copy link

welcome bot commented Oct 14, 2022

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

policy_patterns = config.get(
"policies", ["saltstack/minion/{minion}", "saltstack/minions"]
)
mappings = {"minion": minion_id, "grains": grains or {}}
mappings = {"minion": minion_id, "grains": grains or {}, "pillar": pillar or {}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this also impact the vault execution module. I believe it should based off the code I read, but wanted to verify this since you are only editing the runner module.

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

This will also require a changelog and I just had one clarifying question

@lkubb
Copy link
Contributor

lkubb commented Oct 17, 2022

This approach has the problem that

a) either the token policies are not reliable (when only cached minion pillar data is used, as should be the case here) or
b) that it introduces a cyclic dependency while refreshing the pillar (when a token is requested to compile pillar values that rely on the vault execution module)

For context, see the linked (unmerged) PRs in #62674 (shameless plug: PR I wrote to overcome these issues, still needs a review :]).

@PhilippLemke
Copy link
Author

This PR has a very limited scope -> only the policy assignment via pillar

a) either the token policies are not reliable (when only cached minion pillar data is used, as should be the case here) or
-> yes, it's necessary to keep track that the minion has current pillar data. But that is also important for other states, modules that rely on pillar data.

b) that it introduces a cyclic dependency while refreshing the pillar (when a token is requested to compile pillar values that rely on the vault execution module)
-> Indeed that's not handled in this PR. I think its comparable with the ext pillar.stack module, where you're also able to define pillars based on pillars from salt default pillar env.

PR #62674 looks very promising

@lkubb
Copy link
Contributor

lkubb commented Oct 19, 2022

-> yes, it's necessary to keep track that the minion has current pillar data. But that is also important for other states, modules that rely on pillar data.

I agree that only relying on cached pillar data is sufficient in most cases. There have been several PRs with the exact same proposed change that were denied on the grounds of b) (e.g. #43288), but I am pretty sure the described cyclic dependency cannot happen if you only rely on salt.utils.minions.get_minion_data. It might have been a misunderstanding.

-> Indeed that's not handled in this PR. I think its comparable with the ext pillar.stack module, where you're also able to define pillars based on pillars from salt default pillar env.

This problem is specific to the vault implementation and is only present when you refresh the pillar during policy rendering:

minion requests token -> master renders policies -> refreshes pillar -> some pillar value uses the vault integration -> impersonating master requests token -> master renders policies -> refreshes pillar etc

@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 31, 2022

@PhilippLemke are you stating you want to close this one in favor of #62674?

@PhilippLemke
Copy link
Author

PhilippLemke commented Nov 8, 2022 via email

@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 22, 2022

Closing in favor of #62674

@Ch3LL Ch3LL closed this Nov 22, 2022
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.

None yet

3 participants