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

Feat/policy refactor #551

Closed
wants to merge 28 commits into from
Closed

Conversation

StewartW
Copy link
Contributor

@StewartW StewartW commented Oct 24, 2022

Why?

A refactor of how policies (SCP/Tagging) are managed.

Provides backwards compatibility with the current process whilst enabling an additional folder "adf-policies" that allows for SCPs, Tagging policies (and potentially more) to be defined in a central location. Allowing for policies to be-used across multiple targets. (Currently supports OU-names, but also supports extension going forward)

Issue #, if available:

What?

Description of changes:

Creates a new concept called Policy Campaigns. Which is an object used to orchestrate the updating of policies and their targets.

Policy Campaigns have a list of Policies that require creating. (A policy is classed as requiring creation when a policy with the same name does not exist). Policies that require updating (A policy is classed as requiring updating when an existing policy has a different content) and policies that require deleting (Any policy that is ADF managed but does not receive an interaction throughout the campaign is considered for deletion)

Policies themselves do similar logic for targets, and depending on the adf config, a target will either have the default SCP maintained or removed.

The overall logic is the same for the legacy policy management, the primary difference is that adf-policies is now an optional source for policies.


By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.

src/template.yml Outdated Show resolved Hide resolved
@StewartW
Copy link
Contributor Author

StewartW commented Oct 26, 2022

Still todo:

  • Address TODO comments
  • Update documentation
  • [ ]

@StewartW StewartW marked this pull request as ready for review January 24, 2023 19:02
@StewartW StewartW force-pushed the feat/policy-refactor branch from 30c5c40 to 24c05e5 Compare January 24, 2023 22:33
@StewartW StewartW added this to the v3.2.1 milestone Jan 25, 2023
@sbkok sbkok modified the milestones: v3.2.2, v3.3.0 Jan 27, 2023
Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this.

Because of the difference in this approach to applying policies, it is not
currently the default method and will have to be enabled. In order to enable it,
you have to update your serverlessrepo stack in the organizational root account
and set the parameter `EnablePolicyV2' to "TRUE". Once the stack has redeployed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we move this to the extensions feature?
Or maybe even better, lets say we would merge this for the next v4.0.0 release, should we enable it without a property and explain the different approach in the changelog / steps to upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to postpone to V4.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to get your thoughts on this, because the new V2 approach is 100% backwards compatible with old definitions

docs/admin-guide.md Outdated Show resolved Hide resolved
docs/admin-guide.md Outdated Show resolved Hide resolved
docs/admin-guide.md Show resolved Hide resolved
src/template.yml Outdated Show resolved Hide resolved
src/template.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

A few comments still there from the previous review + a few new ones.
Could you look at these via the Files changed tab? That should only show the comments that still apply.

ou_id,
[], # Initial empty array to hold OU Path,
cache
ou_id, [], cache # Initial empty array to hold OU Path,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you revert this change, as the comment is linked to the empty list. Not the cache or all three arguments together?

status = organizations.get_organization_info()
if status.get("feature_set") != "ALL":
LOGGER.info(
"All Features are currently NOT enabled for this Organization, "
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are tasked to apply an SCP or Tagging Policy, maybe it would be better to raise this as a warning or even a fatal error. As it might go unnoticed otherwise. What do you think?

stubber.assert_no_pending_responses()


class SadTestCases(unittest.TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test case that will test that we throw an error when AWS Organizations is not configured with all features that we require for Tagging and Service Control Policies?

exit 1 \
); \
)
# @( \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't reliable on mac. Is there a better way to do this @sbkok ?

@StewartW StewartW closed this Jun 17, 2024
@StewartW
Copy link
Contributor Author

Closing to favour the v4.0.0 compatible version

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.

2 participants