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

Show GPO settings, raise error if trying to set gpo managed settings #47552

Merged
merged 5 commits into from May 25, 2018

Conversation

Projects
None yet
4 participants
@twangboy
Contributor

twangboy commented May 9, 2018

What does this PR do?

Fixes issues with conflicts between GPO managed SNMP settings and those managed by the SNMP gui. In the GUI, the SNMP settings are greyed out when they are being managed by Group Policy.

What issues does this PR fix or reference?

#46981

Previous Behavior

get_community_names would always return the SNMP gui settings
set_community_names would successfully set SNMP settings without error but would not apply if Group Policy was also managing those settings

New Behavior

get_community_names will now return the Group Policy settings if they are applied. Otherwise, they will return the normal SNMP settings.

GPO Managed
----------
test_community:
    Managed by GPO

SNMP Managed
----------
test_community:
    Read Only

set_community_names now raises a CommandExecutionError if the settings are being managed by Group Policy.

Tests written?

Yes

Commits signed with GPG?

Yes

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows May 9, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 15, 2018

@twangboy This is causing the following 2 tests to fail:

  • unit.modules.test_win_snmp.WinSnmpTestCase.test_get_community_names
  • unit.modules.test_win_snmp.WinSnmpTestCase.test_set_community_names

Can you take a look?

twangboy added some commits May 9, 2018

@twangboy twangboy force-pushed the twangboy:fix_46981 branch from c1647a7 to 008af0a May 16, 2018

@twangboy

This comment has been minimized.

Contributor

twangboy commented May 16, 2018

@rallytime Fixed the failing tests... added new tests to test for instances where the settings are found in gpo

@dwoz

dwoz approved these changes May 18, 2018

@rallytime rallytime requested review from lomeroe and lorengordon May 21, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 21, 2018

@lomeroe and @lorengordon Can you review this PR? Since you were involved in the original discussion in #46981?

@@ -340,6 +412,11 @@ def set_community_names(communities):
'''
values = dict()
if __salt__['reg.key_exists'](_HKEY, _COMMUNITIES_GPO_KEY):
_LOG.debug('Communities on this system are managed by Group Policy')
raise CommandExecutionError(

This comment has been minimized.

@lorengordon

lorengordon May 21, 2018

Contributor

I agree it is better to raise here, but I think it is at least worth a release note that this is a change in behavior (if not a deprecation path). Someone may be using this function on a system now where Group Policy happens to be managing the communities. Salt would succeed previously (if a little pointlessly), and this change will cause a bit of breakage when they update.

This comment has been minimized.

@twangboy

twangboy May 22, 2018

Contributor

The state wouldn't fail, as it checks for the CommandExecutionError. Only a Command line run or a salt api run on the execution module would fail.

This comment has been minimized.

@lorengordon

lorengordon May 23, 2018

Contributor

Right... The point remains, yes? The behavior is changing in a way that breaks prior usage of this function. Certainly salt states are designed to handle that, but salt states are not the sole user of execution modules.

This comment has been minimized.

@twangboy

twangboy May 23, 2018

Contributor

Right. I added this to the release notes.

@twangboy

This comment has been minimized.

Contributor

twangboy commented May 23, 2018

@lomeroe @lorengordon Could you look at this again.

@rallytime rallytime merged commit 799fce9 into saltstack:2017.7 May 25, 2018

5 of 9 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5246 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10216 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19302 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25436 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17524 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23180 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22154 — SUCCESS
Details

@twangboy twangboy deleted the twangboy:fix_46981 branch May 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment