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

TeamsGroupPolicyAssignment Exported configuration cannot be used because of duplicated key value #3054

Closed
sandrola opened this issue Mar 21, 2023 · 12 comments · Fixed by #3057 or #3140
Assignees
Labels
Breaking Changes Bug Something isn't working Teams

Comments

@sandrola
Copy link
Contributor

sandrola commented Mar 21, 2023

The exported configuration of TeamsGroupPolicyAssignment cannot be used for dirftdetection because of the key attribute "rank" used by DSC.

Details of the scenario you tried and the problem that is occurring

Every Grouppolicyassignment starts with Rank 1, If there are multiple groups, also multiple ranks "1" are present. This exported configuration cannot be compiled because of the Key attribute "Rank".

Verbose logs showing the problem

Suggested solution to the issue

As Get-CsGroupPolicyAssignment doesen't generate unique Key in the case above, I suges to Add a unique Key in the Export, and use it as "key" in DSC. (Guid or incremet digit). This was proposed but discarded at the initial Release. We don't thinked about the drift detection. If this is ok, I would implement the solution.

The DSC configuration that is used to reproduce the issue (as detailed as possible)

# insert configuration here
Node localhost
    {
        TeamsGroupPolicyAssignment 86388c59-d8a1-42d7-b768-cfee48d9c406
        {
            ApplicationId         = $ConfigurationData.NonNodeData.ApplicationId;
            CertificateThumbprint = $ConfigurationData.NonNodeData.CertificateThumbprint;
            Ensure                = "Present";
            GroupDisplayName      = "secgroup3";
            GroupId               = "ff49d90d-ce08-4f5f-92a3-db35816c3ad0";
            PolicyName            = "FirstLineWorker";
            PolicyType            = "TeamsAppSetupPolicy";
            Priority              = 1;
            TenantId              = $ConfigurationData.NonNodeData.TenantId;
        }
        TeamsGroupPolicyAssignment 13fbf8b0-e132-4e83-a3cf-998655571b2f
        {
            ApplicationId         = $ConfigurationData.NonNodeData.ApplicationId;
            CertificateThumbprint = $ConfigurationData.NonNodeData.CertificateThumbprint;
            Ensure                = "Present";
            GroupDisplayName      = "SecGroup2";
            GroupId               = "6eb76881-f56f-470f-be0d-672145d3dcb1";
            PolicyName            = "AllowCalling";
            PolicyType            = "TeamsCallingPolicy";
            Priority              = 1;
            TenantId              = $ConfigurationData.NonNodeData.TenantId;
        }
        TeamsGroupPolicyAssignment f2d379c6-8078-42d9-a949-0bd735b790a2
        {
            ApplicationId         = $ConfigurationData.NonNodeData.ApplicationId;
            CertificateThumbprint = $ConfigurationData.NonNodeData.CertificateThumbprint;
            Ensure                = "Present";
            GroupDisplayName      = "secgroup";
            GroupId               = "4ecd31fa-5da1-4003-a3c8-da7d86edc674";
            PolicyName            = "AllowCallingPreventForwardingtoPhone";
            PolicyType            = "TeamsCallingPolicy";
            Priority              = 2;
            TenantId              = $ConfigurationData.NonNodeData.TenantId;
        }
        TeamsGroupPolicyAssignment 0ce64565-5c3c-4dd3-8b4e-f938f9406ed4
        {
            ApplicationId         = $ConfigurationData.NonNodeData.ApplicationId;
            CertificateThumbprint = $ConfigurationData.NonNodeData.CertificateThumbprint;
            Ensure                = "Present";
            GroupDisplayName      = "secgroup";
            GroupId               = "4ecd31fa-5da1-4003-a3c8-da7d86edc674";
            PolicyName            = "AllOn";
            PolicyType            = "TeamsMeetingPolicy";
            Priority              = 1;
            TenantId              = $ConfigurationData.NonNodeData.TenantId;
        }
    }

#### The operating system the target node is running
OsName               : Microsoft Windows 10 Pro
OsOperatingSystemSKU : 48
OsArchitecture       : 64-bit
WindowsVersion       : 2009
WindowsBuildLabEx    : 19041.1.amd64fre.vb_release.191206-1406
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

#### Version of the DSC module that was used ('dev' if using current dev branch)
1.23.315.1
@andikrueger
Copy link
Collaborator

This is related to #2006.

@andikrueger andikrueger added Bug Something isn't working Teams labels Mar 21, 2023
@NikCharlebois
Copy link
Collaborator

Thanks Sandro, out of curiosity, why are you folks naming two different policies the same? I am trying to understand what the business need behind having multiple items with the same name is. Thanks

@sandrola
Copy link
Contributor Author

There is no name for this, the only information are Groupid, Policyname, Policytype, Rank. All attributes can appear doubled.

@andikrueger
Copy link
Collaborator

I did a quick test. It is not possible to have more than one policy with the same name. It is also not possible to select a group twice in the group policy admin interface.

image

Looks like the GroupID has to be the key parameter for this resource. Right now Priority (Rank) is the key - this makes no sense at all.

@sandrola
Copy link
Contributor Author

sandrola commented Mar 22, 2023

You are right, but the groupid as key is not the solution, the same group can be used for different policy/policy types.
Problem is DSC that need a unique key attribute to compile the mof file.
See this output (the configuration above is from the same tenant):

Get-CsGroupPolicyAssignment | fl *

CreatedBy : d37cec8b-ad61-4a2f-a424-c6f59428ac88
CreatedTime : 3/15/2023 11:36:58 AM
GroupId : ff49d90d-ce08-4f5f-92a3-db35816c3ad0
PolicyName : FirstLineWorker
PolicyType : TeamsAppSetupPolicy
Priority : 1

CreatedBy : d37cec8b-ad61-4a2f-a424-c6f59428ac88
CreatedTime : 1/6/2023 2:46:54 PM
GroupId : 6eb76881-f56f-470f-be0d-672145d3dcb1
PolicyName : AllowCalling
PolicyType : TeamsCallingPolicy
Priority : 1

CreatedBy : d37cec8b-ad61-4a2f-a424-c6f59428ac88
CreatedTime : 1/3/2023 4:15:39 PM
GroupId : 4ecd31fa-5da1-4003-a3c8-da7d86edc674
PolicyName : AllowCallingPreventForwardingtoPhone
PolicyType : TeamsCallingPolicy
Priority : 2

CreatedBy : d37cec8b-ad61-4a2f-a424-c6f59428ac88
CreatedTime : 1/3/2023 1:11:56 PM
GroupId : 4ecd31fa-5da1-4003-a3c8-da7d86edc674
PolicyName : AllOn
PolicyType : TeamsMeetingPolicy
Priority : 1

@andikrueger
Copy link
Collaborator

andikrueger commented Mar 22, 2023

I see. the difference. We need to make PolicyType a Key too. Then it will work. In this case DSC will have a combined key of two parameters, that needs to be a unique combination.

Following screenshot is a calling policy.

image

@andikrueger
Copy link
Collaborator

andikrueger commented Mar 22, 2023

Just want to start the discussion about the best set of keys for this resource. We need a composite key consisting of two key parameters.

In #3057 i introduced the following combination:

  • GroupID
  • PolicyType

which would guarantee the uniqueness of the resource, but would not work in tenant migrations that would depend on the group display name. Which is on the other hand not a unique element in the tenant.

In #3060 @sandrola introduced the following keys:

  • PolicyType
  • PolicyName

the policy name is unique in one category (PolicyType). The modified implementation of the handling of group is and name would allow for various scenarios in export and import and within a tenant.

What I did not test and see as a challenge right now, it will not be possible to assign the same policy to several groups.

We should search for a solution that would work in all scenarios

@sandrola
Copy link
Contributor Author

What would solve the problem is to introduce an "Id" (numeric incremental generated in export) wich is used only by DSC for compiling. This Id would be the Key.

@andikrueger
Copy link
Collaborator

We do have this discussion in #2006 and are looking for a way to get around it.

@andikrueger
Copy link
Collaborator

Just to be a bit more specific as in my previous answer:

We need a solution that would fit either use case:

  1. Creating a configuration from scratch
  2. Creating a configuration as result from an export

In Export we could easily add any number to the resource. It would still require the resource to have an additional key that would only be used for the compilation and uniqueness of a resource. As soon as someone would create an export, we would not be aware of the original ID to make a meaningful comparison of both configurations. Even in the case of Test-TargetResource, this could cause troubles.

@ykuijs
Copy link
Member

ykuijs commented Apr 3, 2023

The code first checks if the specified GroupId exists and when it does not, uses the DisplayName to retrieve the group based on name. This allowed the export to be applied to a different tenant, where the group has a different ID but the same name.

In the Intune resource, we have the same issue. There we specified the Id as key parameter and the DisplayName as required. That way we always have the needed information at available. I would suggest that we do the same here. Maybe not the ideal solution, but the best we have available now and can implement before Wednesdays release.

@andikrueger Can you please update you PR, update it with Dev and set it to final. Then I can merge it into Dev and include this in our upcoming release.

@andikrueger
Copy link
Collaborator

@ykuijs Just set the PR to active. Looks like there are no conflicts... Will update the code to add the GroupID check.

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