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

💡 Feature Request - Simplify management group customization #158

Closed
olljanat opened this issue Feb 21, 2022 · 20 comments
Closed

💡 Feature Request - Simplify management group customization #158

olljanat opened this issue Feb 21, 2022 · 20 comments
Assignees

Comments

@olljanat
Copy link
Contributor

Describe the solution you'd like

I'm trying to get ALZ implementation which is done with combination of 3rd party tool(s) and manual work under control with these modules and prefer doing it on way that I would be able to use these modules without customizations so I can have automation which syncs latest module versions to our GIT repo with pull requests.

However I found some limitation which prevents me to doing it so opening this issue to discuss if those changes makes sense.
Changes which I would like to see on managementGroups module are:

  • Landing Zones management groups should be moved to separate file as I want skip those Corp and Online MGs.
  • There should be option for top level MG suffix (as we use mg-alz as prefix and mg-alz-root as root MG).
  • There should be option for top level MG parent as other why -WhatIf deployment will every time show that there is changes available.
@jtracey93
Copy link
Contributor

Thanks for raising this @olljanat & @rjygraham, I have combined the 2 issues into 1 here.

Our aim is that the code can be taken and customized to suit your needs. As we have authored the code in such a way to be easy to read and tweak it 👍

When tweaking this you need to be careful to update the other dependencies like ALZ Default Policy Assignments as this assumes the default Management Group hierarchy, but it is easy to tweak and adjust as you need too.

Would love to here some suggestions from both of you as to how you may want this to work? Or is taking the module and tweaking it a good solution for you?

Keeping in mind we want to make the default hierarchy and following modules all work smoothly to deliver the ALZ conceptual architecture

Looking forward to hearing more from you both 👍👍👍

Assigning to you @rjygraham as we discussed yesterday offline 👍

@jtracey93 jtracey93 added Area: Management Groups and removed Needs: Triage 🔍 Needs triaging by the team labels Feb 23, 2022
@rjygraham
Copy link
Contributor

I put together a POC on how Mgmt Groups can be easily customized via the following parameter file:

{
    "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentParameters.json#",
    "contentVersion": "1.0.0.0",
    "parameters": {
        "parManagementGroupHierarchy": {
            "value": [
                {
                    "name": "alz",
                    "displayName": "Azure Landing Zones",
                    "children": [
                        {
                            "name": "alz-decommissioned",
                            "displayName": "Decommissioned",
                            "children": []
                        },
                        {
                            "name": "alz-landingzones",
                            "displayName": "Landing Zones",
                            "children": [
                                {
                                    "name": "alz-landingzones-corp",
                                    "displayName": "Corp",
                                    "children": []
                                },
                                {
                                    "name": "alz-landingzones-online",
                                    "displayName": "Online",
                                    "children": []
                                }
                            ]
                        },
                        {
                            "name": "alz-platform",
                            "displayName": "Platform",
                            "children": [
                                {
                                    "name": "alz-platform-connectivity",
                                    "displayName": "Connectivity",
                                    "children": []
                                },
                                {
                                    "name": "alz-platform-identity",
                                    "displayName": "Identity",
                                    "children": []
                                },
                                {
                                    "name": "alz-platform-management",
                                    "displayName": "Management",
                                    "children": []
                                }
                            ]
                        },
                        {
                            "name": "alz-sandbox",
                            "displayName": "Sandbox",
                            "children": []
                        }
                    ]
                }
            ]
        }
    }
}

Running my POC against this parameter file would yield the default ALZ Management group structure as it exists today:

image

@jtracey93 as you rightfully pointed out, we'd have to account for downstream dependencies like Azure Policy assignments.

@jtracey93
Copy link
Contributor

I put together a POC on how Mgmt Groups can be easily customized via the following parameter file:

{

    "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentParameters.json#",

    "contentVersion": "1.0.0.0",

    "parameters": {

        "parManagementGroupHierarchy": {

            "value": [

                {

                    "name": "alz",

                    "displayName": "Azure Landing Zones",

                    "children": [

                        {

                            "name": "alz-decommissioned",

                            "displayName": "Decommissioned",

                            "children": []

                        },

                        {

                            "name": "alz-landingzones",

                            "displayName": "Landing Zones",

                            "children": [

                                {

                                    "name": "alz-landingzones-corp",

                                    "displayName": "Corp",

                                    "children": []

                                },

                                {

                                    "name": "alz-landingzones-online",

                                    "displayName": "Online",

                                    "children": []

                                }

                            ]

                        },

                        {

                            "name": "alz-platform",

                            "displayName": "Platform",

                            "children": [

                                {

                                    "name": "alz-platform-connectivity",

                                    "displayName": "Connectivity",

                                    "children": []

                                },

                                {

                                    "name": "alz-platform-identity",

                                    "displayName": "Identity",

                                    "children": []

                                },

                                {

                                    "name": "alz-platform-management",

                                    "displayName": "Management",

                                    "children": []

                                }

                            ]

                        },

                        {

                            "name": "alz-sandbox",

                            "displayName": "Sandbox",

                            "children": []

                        }

                    ]

                }

            ]

        }

    }

}

Running my POC against this parameter file would yield the default ALZ Management group structure as it exists today:

image

@jtracey93 as you rightfully pointed out, we'd have to account for downstream dependencies like Azure Policy assignments.

Thanks @rjygraham let's discuss this offline. As whilst this does indeed work we need to balance this against how complex and error prone the parameter input may be for consumers.

Especially as we can do limited validation checking of the input to the parameter 😢

@olljanat Would really appreciate your input on this based on your initial points and asks when raising this request. Really keen to make sure we understand if going down this, or a similar approach, as @rjygraham has suggested is something that would work. Or is a simpler solution like Boolean flags for child management groups etc. an option for example?

As stated we are not opposed to you cloning the code and customising it to suit your needs. This is the main reason for authoring the bicep modules in this fashion, so they are accessible and customisable by all. Instead of being super flexible and complex.

There's pros and cons to both sides here so really value everyone's inputs 👍👍👍

@ghost
Copy link

ghost commented Feb 27, 2022

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@jtracey93
Copy link
Contributor

Any feedback on this @olljanat ?

@olljanat
Copy link
Contributor Author

Sorry about late answer. POC proposed by @rjygraham would works to my needs but I need to say that I haven't read all those landing zone design/best practices documentations to and I'm not sure that which parts there are best practices and which more like examples.

However I would expect that no one really uses landing zones with names "Corp" or "Online" in real production environment but that at least those are just examples which why I originally proposed that those would be just moved to another file.

@ghost ghost added Needs: Attention 👋 Needs attention from the maintainers and removed Needs: Author Feedback labels Feb 28, 2022
@jtracey93
Copy link
Contributor

Thanks @olljanat for coming back, really appreciate it 👍

Out of interest would you feel comfortable editing the Management Group module itself once you have forked/cloned it? And then you can customise the names etc and structure to whatever you desire? Or would you prefer having options or a complex input parameter to define your entire Management Group hierarchy?

Really just looking for insights and justification to help us make a decision here about moving forward 👍

@jtracey93 jtracey93 added Needs: Author Feedback and removed Needs: Attention 👋 Needs attention from the maintainers labels Feb 28, 2022
@olljanat

This comment was marked as off-topic.

@ghost ghost added Needs: Attention 👋 Needs attention from the maintainers and removed Needs: Author Feedback labels Feb 28, 2022
@jtracey93
Copy link
Contributor

Thanks for sharing @olljanat.

Just to update everyone we are looking for more feedback and perspectives on this issue and @rjygraham will drive this to ensure that we have enough evidence to support making a decision on the approach here.

Therefore, we ask everyone to pitch in here and let us know you preferred approach and why so we can make a decision.

@jtracey93 jtracey93 added more evidence required and removed Needs: Attention 👋 Needs attention from the maintainers labels Mar 3, 2022
@jtracey93 jtracey93 pinned this issue Mar 3, 2022
@KiZach
Copy link
Contributor

KiZach commented Mar 4, 2022

To support 'Testing approach for enterprise-scale' we have implemented a small change to the managementGroups module in the managementGroups.bicep file. Just added a bool parameter like this:

@description('Set Parameter to true to have the management group prefix added to child management groups.')
param parAppendTopLevelMGPrefixToChildMGs bool = false

This will result in the same naming as the ARM deployment if desired to support multiple MG hierarchies in the same Tenant (Canary) without MGs have the same displaynames.

image

Let me know if I should create a PR for this change, to have it public available. Defaults to false so no braking changes, and we have tested renaming works if later enabled.

@jtracey93
Copy link
Contributor

Hey @KiZach,

thanks for sharing. Do you have any opinion on the above suggestions about adding flexibility to the module?

Don’t create a PR just yet as we are not planning to make any changes to this module without further evidence and an idea on direction that we will take us.

@KiZach
Copy link
Contributor

KiZach commented Mar 4, 2022

Hey @jtracey93

My opinion on this, also due to the user rights needed for deployment I see the MG deployment as more of a one-time/initial step only deployment. It will also require changes to policy assignments to support this flexibility.

From my perspective it is ok to have the groups, even if you have no on-prem on online subscriptions. But I think from a zero trust perspective it makes good sense to have these 2 MGs, as the requirements are quite different.

Prefix and suffix should be easy to implement as parameters.

@rjygraham
Copy link
Contributor

@jtracey93 @olljanat @KiZach Let me get a PR out for this issue and let you all react. I think we'll be able to find a happy middle ground between keeping things simple for those that want to take the default mgmt group structure and those that want more flexibility. Stand by.

@rjygraham
Copy link
Contributor

@olljanat @KiZach curious to your thoughts about this potential solution to simplifying mgmt group customization. There is still work needed for mgmt group names/resource Ids as outputs of the module and we'll need to come up with a way to handle customization impact on other modules like policy.

@olljanat
Copy link
Contributor Author

@rjygraham you mean #178 right? Honestly it feels that you try implement a bit too advanced solution which why you end up to have big challenge those resource id references.

Let me go back to my original request and compare how it can be solved with Terraform version of ALZ implementation (as I now finally had time to study that one too):

* Landing Zones management groups should be moved to separate file as I want skip those _Corp_ and _Online_ MGs.

Terraform module contains variable deploy_demo_landing_zones which based on this implements exactly what I asked for (ability to avoid those demo/example landing zones to be deployed).

* There should be option for top level MG suffix (as we use `mg-alz` as prefix and `mg-alz-root` as root MG).

Terraform module contains variable root_id which does exactly what I asked for.

Based on what @jtracey93 #173 (comment) I understood that it is target to maintain parity between these implementations so maybe it would be easier/better option handle this same way than Terraform version does?

@olljanat
Copy link
Contributor Author

olljanat commented Apr 6, 2022

@rjygraham ping

@jtracey93
Copy link
Contributor

Making a call on this one that we will not implement until custom types and in Bicep are possible and supported

Closed #178 to align to this

@jtracey93
Copy link
Contributor

We recently added some new functionality for child landing zone management group flexibility in #276 that is worth checking out.

@jtracey93
Copy link
Contributor

Discussed in scrum meeting today. We agree to close this issue out for now and see if this is a feature request that is raised again in the future.

As we have recently enabled some of this in #276 that is aligned to the Azure landing zones tailoring guidance https://docs.microsoft.com/en-gb/azure/cloud-adoption-framework/ready/landing-zone/tailoring-alz, We believe that this is sufficient for now.

Once the above Bicep features referenced above in a previous comment are resolved and implemented and available. We will consider potentially a bigger rewrite of the ALZ bicep modules, if suitable and required, to add some additional flexibility into the modules whilst ensuring we are not compromising usability for our persona base of users. (L100/200 mainly)

Also, projects like CARML may be further along by this stage and be more defined, which may mean we can adopt those. This is purely hypothetical at this stage.

@jtracey93 jtracey93 unpinned this issue Jul 21, 2022
@olljanat
Copy link
Contributor Author

@jtracey93 #276 looked to be very useful and I was able to reduce number of customizations a lot. We still need some small ones with naming because of history but at least those are very small tweaking anymore. Thanks you

@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2022
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants