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

IntuneDeviceConfigurationPolicyWindows10: Support setting assignment groups by display name #3763

Merged
merged 19 commits into from
Oct 17, 2023

Conversation

ricmestre
Copy link
Contributor

Pull Request (PR) description

Currently all Intune policies rely on having assignments as a CIM instance of type MSFT_DeviceManagementConfigurationPolicyAssignments, this is not ideal since it doesn't specify exactly which type of policy it is and if any change needs to be done on a specific one then all Intune resources need to be changed in one big sweep at the same time.

This is a first step by decoupling IntuneDeviceConfigurationPolicyWindows10 and have the assignments CIM instance be set to a new type called MSFT_IntuneDeviceConfigurationPolicyWindows10Assignments which in turn allows to make changes on it, and this PR is to show that.

I've added a new property called groupDisplayName so that if it always exports both the Id and DisplayName of the groups, then when updating it looks for the group (through the Id) and if not found (or Id wasn't defined in the blueprint) then it searches for the Display Name, finally if group cannot be found through both properties it skips and checks next assignment.

If this change is ok (even if amendments are required) then I'd like to change the DRG so that it creates new resources with this logic and then we can change current resources 1 by 1, nevertheless this might be proven to be tricky since some of them were created manually without the DRG and have completely different logic on how to make the assignments.

This Pull Request (PR) fixes the following issues

None.

@ricmestre
Copy link
Contributor Author

Forgot to mention, the intention with this is to be able to to apply the policies to several tenants and keep the same assignments, currently I'm relying on a script that basically translates names into Ids and another one that reverses the process but I'd like to have this change done instead.

@ricmestre
Copy link
Contributor Author

Question, since I always export both groupId and groupDisplayName in Get-TargetResource then when calling for instance Assert-M365DSCBlueprint to compare the blueprint with the tenant it still complains that it's not right if groupId is empty in the blueprint so how can I solve this? During export call only assign groupId to the hashtable if there are multiple groups with the same displayName otherwise only add displayName?

@andikrueger
Copy link
Collaborator

Assert would call into New-M365DSCDeltaReport which in this case does not handle the display name and id conversion properly. I would call this a new bug in this case. The DeltaReport should compare based on the primary key - for resources with display name and id set, it should use the ID to match first and then fall back to the display name.

@ricmestre
Copy link
Contributor Author

@andikruger but this is not a problem of the resource itself, it's with the Assignments property and that one of course doesn't have any primary key, I have changed the code like I said and made groupId has null (which removes it from the blueprint) and end up with this:

Assignments                                          = @(
    MSFT_IntuneDeviceConfigurationPolicyWindows10Assignments{
        groupDisplayName = 'Aad.static.MEM.device-W10_Device_Restrictions_Baseline_Exclude'
        deviceAndAppManagementAssignmentFilterType = 'none'
        dataType = '#microsoft.graph.exclusionGroupAssignmentTarget'
    }
    MSFT_IntuneDeviceConfigurationPolicyWindows10Assignments{
        groupDisplayName = 'Aad.static.MEM.device-W10_Device_Restrictions_Baseline_Include'
        deviceAndAppManagementAssignmentFilterType = 'none'
        dataType = '#microsoft.graph.groupAssignmentTarget'
    }
);

But now New-M365DSCDeltaReport has another problem since it shows as if both entries are mixed up and always returns the below even if I call up Source and Destination on the same file:

    "ResourceName":  "IntuneDeviceConfigurationPolicyWindows10",                                                                                                                                                       
    "Key":  "Assignments",                                                                                                                                                                                             
    "KeyValue":  null,                                                                                                                                                                                                 
    "Properties":  [                                                                                                                                                                                                                      
        {                                                                                                                                                                                                                      
            "ValueInDestination":  "#microsoft.graph.groupAssignmentTarget",                                                                                                                                                   
            "CIMInstanceKey":  "",                                                                                                                                                                                             
            "ParameterName":  "dataType",                                                                                                                                                                                      
            "ValueInSource":  "#microsoft.graph.exclusionGroupAssignmentTarget",                                                                                                                                               
            "CIMInstanceValue":  null                                                                                                                                                                                      
        }                                                                                                                                                                                                              
    ],                                                                                                                                                                                                 
    "ResourceInstanceName":  "IntuneDeviceConfigurationPolicyWindows10-REDACTED"                                                                                    }                                                                                                                                                                                                              

@andikrueger
Copy link
Collaborator

Just to understand it better: The issue is about comparing nested CIM Instances of the resource. In this case, it would be similar to this issue #3766

@ricmestre
Copy link
Contributor Author

It's not the same thing but it's related so it made me look into Get-M365DSCCimInstanceKey and solved my issue, the report shows 0 drifts now. I solved it by changing the name of subproperty dataType to odataType which is then defined as primary key of the CIM instance by Get-M365DSCCimInstanceKey.

I'll push my changes in a minute.

@andikrueger
Copy link
Collaborator

good find. Will this change be a breaking one then?

@ricmestre
Copy link
Contributor Author

ricmestre commented Oct 9, 2023

Does this actually count as breaking change? Yes, it breaks if someone has assignments for this policy since it still has property dataType and not odataType but this is a subproperty from a CIM instance which is changing name but it's not set has mandatory in the schema.

So that being said I can add code to Set-TargetResource which in the back looks if odataType is not set but dataType is then assigns its value which then would not count as breaking change, but I'd also argue that this is then actually wrong at the moment for all Intune resources since Assignments CIM instance is using dataType instead of odataType for all of them (except this one I'm changing) and therefore is not comparing between reports correctly.

@ricmestre
Copy link
Contributor Author

Another option for the time being would be to just add dataType as a lookup key in Get-M365DSCCimInstanceKey

@andikrueger
Copy link
Collaborator

Are there any other resources with cim instance that might also have Datatype instead of o-datatype?

@ricmestre
Copy link
Contributor Author

Almost all Intune resources that have Assignments use dataType, from a quick look I think it's plus some other in the newly added IntunePolicySets. I'm on mobile right now and can't check it further.

@ricmestre
Copy link
Contributor Author

@andikrueger just confirmed it with a grep, it's only used for Assignments in all Intune resources plus MSFT_DeviceManagementConfigurationPolicyItems on IntunePolicySets, this resource was just added a few days ago.

@andikrueger
Copy link
Collaborator

Thanks. I just saw your other PR #3769 addressing this issue. Thanks for looking into it.

@@ -1,10 +1,11 @@
[ClassVersion("1.0.0.0")]
class MSFT_DeviceManagementConfigurationPolicyAssignments
class MSFT_IntuneDeviceConfigurationPolicyWindows10Assignments
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would result in a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change really needed? The CIM Instance is embedded into this resource. Are there any other CIM Instances with the previous name somewhere else (and do they have the same properties?

Copy link
Contributor Author

@ricmestre ricmestre Oct 11, 2023

Choose a reason for hiding this comment

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

Basically almost all Intune resources that support Assignments use exactly the same CIM instance MSFT_DeviceManagementConfigurationPolicyAssignments, which means that in order to support the changes I made in this PR without changing the CIM instance name I would need to change all Intune resources at the same time otherwise it gives error messages when importing the module.

I discussed this privately with Nik several months ago.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do all other resources need the display name parameter? If yes, we should change it globally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with this PR starting by addressing one resource, and then later submit a larger PR to address them all. However, we need to confirm whether or not we really need to rename the CIMInstance right now for this to work or if this can be done in April. I can hold today's release for a few hours, but if we can't resolve the renaming question soon, this will need to get pushed to next week at a minimum. Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. What I am suggesting for now is to keep the additional code in, but keep the current CIMInstance name. Then later submit two other PRs. The first one to add the data type to all resources, which could get released anytime when ready, then a second one to rename the CIMInstance which would get release as breaking changes in April. Would that be feasible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

groupDisplayName, not dataType :) But yeah that's what I mentioned keep the code I added, change CIMInstance name back to the original but then I'll need to add groupDisplayName property to all resources in this same PR otherwise it won't work, all resources must be changed at the same time, then the code to make them work could be added later on.

That being said, do you still prefer to have individual CIMInstance names per resource (making those changes in April) or keep the same name for all of them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coming from a standpoint, that we should reuse as much code as possible, and if there is no eminent reason to change the CIM Instance name, I would stick one name across all resources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I was explaining the current situation to a colleague, I realized that what I am suggesting above doesn't make sense. In doing so, we would end up having two sets of definition for the same CIMInstance namespace. One where datatype is present and one where it isn't. Off course the fix would be to make sure datatype is added to all MSFT_DeviceManagementConfigurationPolicyAssignments schemas. Sorry for not picking up on this earlier. I will go ahead and release 1.23.1011.1 without this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In latest iteration all, well almost all, Intune resources have now an additional property called groupDisplayName inside MSFT_DeviceManagementConfigurationPolicyAssignments.

@NikCharlebois
Copy link
Collaborator

Changing the CIMInstance name would result in a breaking change. One recommendation would be to split PRs. Keep this one focused on the datatype and keep the same CIM instance name, while opening a separate one with the breaking change to rename the CIM Instance.

@ricmestre
Copy link
Contributor Author

ricmestre commented Oct 11, 2023

@NikCharlebois The discussion about dataType was AFTER I saw the problem with the code I had before but now with my other PR merged then there is nothing about dataType here, the whole reasoning of raising this PR in the first place was to make it support groups by their displayName instead of Ids and to do that without changing all Intune resources at once was to decouple it by changing the CIM instance.

Is there any other way to make it a non-breaking change and not have to wait until April? My intention was to move next into making these changes into the DRG.

@andikrueger
Copy link
Collaborator

@ricmestre Could you check, if there is really the need to rename the CIM Instance? see my comment within the code above.

@@ -1,10 +1,11 @@
[ClassVersion("1.0.0.0")]
class MSFT_DeviceManagementConfigurationPolicyAssignments
class MSFT_IntuneDeviceConfigurationPolicyWindows10Assignments
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I was explaining the current situation to a colleague, I realized that what I am suggesting above doesn't make sense. In doing so, we would end up having two sets of definition for the same CIMInstance namespace. One where datatype is present and one where it isn't. Off course the fix would be to make sure datatype is added to all MSFT_DeviceManagementConfigurationPolicyAssignments schemas. Sorry for not picking up on this earlier. I will go ahead and release 1.23.1011.1 without this change.

@NikCharlebois NikCharlebois merged commit f7fff6d into microsoft:Dev Oct 17, 2023
2 checks passed
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.

3 participants