Add Azure endpoint for ClusterQuorum resource #279

hungtran84 · 2022-06-28T17:06:06Z

Pull Request (PR) description

This PR is created for adopting Azure Government cloud where the resource endpoint is required.

This Pull Request (PR) fixes the following issues

Fixes ClusterQuorum: Doesn't support Azure Gov Cloud #278

Task list

Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
Resource documentation updated in the resource's README.md.
Resource parameter descriptions updated in schema.mof.
Comment-based help updated, including parameter descriptions.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

johlju

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hungtran84)

a discussion (no related file):
The tests are failing. Looks like we need to add another mock for the added code in the get-function
https://dev.azure.com/dsccommunity/FailoverClusterDsc/_build/results?buildId=6286&view=logs&j=8af7cf9c-43a1-584d-6f5c-57bad8880974&t=a93ba588-7f3e-5259-6924-3f054a327451&l=881

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 193 at r1 (raw file):

        'NodeAndCloudMajority'
        {
            Set-ClusterQuorum -CloudWitness -AccountName $Resource -AccessKey $StorageAccountAccessKey -Endpoint $Endpoint

I'm not familiar with using cloud witness, so I'm curios - is there no other scenario where Endpoint is not necessary so that we should not add it by default? If not, then this line looks good to me.

Googling it looks like it is necessary, just wanted to confirm if you know. 🙂

Code quote:

-Endpoint $Endpoint

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 215 at r1 (raw file):

    .PARAMETER Endpoint
        Azure service endpoint. This paramter is required if the quorum type is set to NodeAndCloudMajority.
        The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

We should also add this text to the description in the schema mof since the schema MOF is used to generate the wiki documentation.

Code quote:

The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 215 at r1 (raw file):

    .PARAMETER Endpoint
        Azure service endpoint. This paramter is required if the quorum type is set to NodeAndCloudMajority.
        The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

May also say? ...or a region specific endpoint
Saw it mentioned here: https://docs.microsoft.com/en-us/windows-server/failover-clustering/deploy-cloud-witness

Throughout comment-based help and schema MOF.

Code quote:

only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 245 at r1 (raw file):

        [Parameter()]
        [System.String]
        $Endpoint = 'core.windows.net',

If the current state already has a different endpoint, should not Test-function return $false so that Set-function kan set the correct endpoint? When the endpoint is part of the configuration and the value is in desired state should Test-function return $true?

Code quote:

$Endpoint = 'core.windows.net'

hungtran84 · 2022-07-01T09:02:43Z

We should also add this text to the description in the schema mof since the schema MOF is used to generate the wiki documentation.

Already added to schema MOF file (if I'm not wrong)

If the current state already has a different endpoint, should not Test-function return $false so that Set-function kan set the correct endpoint? When the endpoint is part of the configuration and the value is in desired state should Test-function return $true?

I tried to remove the default value of Endpoint and set the mock value but no luck. Actually, I don't know much about DSC Unit test and struggle on trousbleshooting/resolving the failed test case.

@johlju could you help to solve it? Thank you in advanced

stale · 2022-07-30T21:21:49Z

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

johlju

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @hungtran84 and @johlju)

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 215 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should also add this text to the description in the schema mof since the schema MOF is used to generate the wiki documentation.

I mean ad the text The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net) to the parameter description in the schema.mof.

Code quote:

The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

johlju · 2022-07-31T13:29:08Z

/AzurePipelines run

azure-pipelines · 2022-07-31T13:29:18Z

Azure Pipelines successfully started running 1 pipeline(s).

johlju

Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @hungtran84 and @johlju)

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 256 at r2 (raw file):

    $getGetTargetResourceResult = Get-TargetResource -IsSingleInstance $IsSingleInstance

    $testTargetResourceReturnValue = $false

This should only do the evaluation if the parameters was assigned. So maybe we have tu reverse it to something like this (this will also fix Resource):

    $testTargetResourceReturnValue = $true

    if ($getGetTargetResourceResult.Type -ne $Type)
    {
        $testTargetResourceReturnValue = $false
    }
    
    if ($PSBoundParameters.ContainsKey('Resource') -and $getGetTargetResourceResult.Resource -ne $Resource)
    {
        $testTargetResourceReturnValue = $false
    }
    
    if ($PSBoundParameters.ContainsKey('Endpoint') -and $getGetTargetResourceResult.Endpoint -ne $Endpoint)
    {
        $testTargetResourceReturnValue = $false
    }

Suggestion:

    $testTargetResourceReturnValue = $true

    if ($getGetTargetResourceResult.Type -ne $Type)
    {
        $testTargetResourceReturnValue = $false
    }
    
    if ($PSBoundParameters.ContainsKey('Resource') -and $getGetTargetResourceResult.Resource -ne $Resource)
    {
        $testTargetResourceReturnValue = $false
    }
    
    if ($PSBoundParameters.ContainsKey('Endpoint') -and $getGetTargetResourceResult.Endpoint -ne $Endpoint)
    {
        $testTargetResourceReturnValue = $false
    }

hungtran84 · 2022-07-31T14:00:53Z

Thank you for your suggestion. I'll be working on it soonest.

…

On Sun, Jul 31, 2022, 20:49 Johan Ljunggren ***@***.***> wrote: ***@***.**** requested changes on this pull request. *Reviewable <https://reviewable.io/reviews/dsccommunity/failoverclusterdsc/279>* status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @hungtran84 <https://github.com/hungtran84> and @johlju <https://github.com/johlju>) ------------------------------ *source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 256 at r2 <https://reviewable.io/reviews/dsccommunity/failoverclusterdsc/279#-N8JVVpu8C69Be0MkFj1:-N8JVVpu8C69Be0MkFj2:b5r8pkw> (raw file <https://github.com/dsccommunity/failoverclusterdsc/blob/2147a57d86ea8a7d7d441a117dadb249f1eba50b/source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1#L256>):* $getGetTargetResourceResult = Get-TargetResource -IsSingleInstance $IsSingleInstance $testTargetResourceReturnValue = $false This should only do the evaluation if the parameters was assigned. So maybe we have tu reverse it to something like this (this will also fix Resource): $testTargetResourceReturnValue = $true if ($getGetTargetResourceResult.Type -ne $Type) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Resource') -and $getGetTargetResourceResult.Resource -ne $Resource) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Endpoint') -and $getGetTargetResourceResult.Endpoint -ne $Endpoint) { $testTargetResourceReturnValue = $false } *Suggestion:* $testTargetResourceReturnValue = $true if ($getGetTargetResourceResult.Type -ne $Type) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Resource') -and $getGetTargetResourceResult.Resource -ne $Resource) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Endpoint') -and $getGetTargetResourceResult.Endpoint -ne $Endpoint) { $testTargetResourceReturnValue = $false } — Reply to this email directly, view it on GitHub <#279 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHGGMR7M7QRAQU5MEICRIRDVWZ75PANCNFSM52C4OAEA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

johlju

Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @hungtran84 and @johlju)

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 98 at r2 (raw file):

            Select-Object -ExpandProperty Value
        $clusterQuorumEndpoint = $getClusterQuorumResult.QuorumResource |
            Get-ClusterParameter -Name EndpointInfo |

The tests are failing on this row because the command Get-ClusterParameter is not mocked with that parameter resulting in that the stub command is called (which correctly throws). See next comment in the tests.

Code quote:

Get-ClusterParameter -Name EndpointInfo

tests/Unit/DSC_ClusterQuorum.Tests.ps1 line 177 at r2 (raw file):

                    Mock -CommandName 'Get-ClusterQuorum' -MockWith $mockGetClusterQuorum
                    Mock -CommandName 'Get-ClusterParameter' -MockWith $mockGetClusterParameter_SharePath -ParameterFilter $mockGetClusterParameter_SharePath_ParameterFilter
                    Mock -CommandName 'Get-ClusterParameter' -MockWith $mockGetClusterParameter_AccountName -ParameterFilter $mockGetClusterParameter_AccountName_ParameterFilter

Se previous comment in the code regarding Get-ClusterParameter. Here we need to add a new line to mock the command Get-CLusterParameter correctly.

After line 177, add:

Mock -CommandName 'Get-ClusterParameter' -MockWith {
    return @(
        # MAKE SURE TO RETURN THE CORRECT PROPERTIES OF EndpointInfo, I don't know the properties...This was just a guess copied from previous tests.
        [PSCustomObject] @{
            ClusterObject = '?'
            Name          = 'EndpointInfo'
            IsReadOnly    = '?'
            ParameterType = '?'
            Value         = 'endpoint.azure.dummy'
        }
    )
} -ParameterFilter {
    $Name -eq 'EndpointInfo'
}

Code quote:

Mock -CommandName 'Get-ClusterParameter' -MockWith $mockGetClusterParameter_AccountName -ParameterFilter $mockGetClusterParameter_AccountName_ParameterFilter

johlju · 2022-07-31T14:09:41Z

@hungtran84 let me know when you done above changes and I take a look again.

johlju

Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions (waiting on @hungtran84 and @johlju)

tests/Unit/DSC_ClusterQuorum.Tests.ps1 line 383 at r3 (raw file):

                    Mock -CommandName 'Get-ClusterParameter' -MockWith $mockGetClusterParameter_SharePath -ParameterFilter $mockGetClusterParameter_SharePath_ParameterFilter
                    Mock -CommandName 'Get-ClusterParameter' -MockWith $mockGetClusterParameter_AccountName -ParameterFilter $mockGetClusterParameter_AccountName_ParameterFilter

Need to add the mock here too.

                    Mock -CommandName 'Get-ClusterParameter' -MockWith {
                        return @(
                            [PSCustomObject] @{
                                Object = 'Cloud Witness'
                                Name   = 'EndpointInfo'
                                Type   = 'String'
                                Value  = 'endpoint.azure.dummy'
                            }
                        )
                    } -ParameterFilter {
                        $Name -eq 'EndpointInfo'
                    }

codecov · 2022-08-02T05:17:12Z

Codecov Report

Merging #279 (7ec21f6) into main (faa9aa3) will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #279   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         8      8           
  Lines       611    619    +8     
===================================
+ Hits        611    619    +8

Impacted Files	Coverage Δ
...Resources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1	`100% <100%> (ø)`

hungtran84 · 2022-08-02T05:27:02Z

@johlju It seems that all the tests passed, could you help to review my PR again?

johlju · 2022-08-02T10:19:28Z

Great work! I will review as soon as I have a chance.

johlju

Great work! A few more comments.

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hungtran84)

CHANGELOG.md line 11 at r4 (raw file):

- ClusterQuorum
  - New parameter Endpoint to support Azure Government cloud. Default value now is only for the commercial one.

This text is no longer accurate as there is no default value.

Code quote:

Default value now is only for the commercial one.

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 98 at r4 (raw file):

            Select-Object -ExpandProperty Value
        $clusterQuorumEndpoint = $getClusterQuorumResult.QuorumResource |
            Get-ClusterParameter -Name EndpointInfo |

Just want to verify, I assume this pipeline returns $null if there is no endpoint info to return, is that correct? Just want to verify that the command Get-ClusterParameter does not throw if there is no endpoint info.

Code quote:

        $clusterQuorumEndpoint = $getClusterQuorumResult.QuorumResource |
            Get-ClusterParameter -Name EndpointInfo |
            Select-Object -ExpandProperty Value

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 131 at r4 (raw file):

    .PARAMETER Endpoint
        Azure service endpoint. This paramter is required if the quorum type is set to NodeAndCloudMajority.

This is no longer accurate as it is now optional, we no longer have a default value. We should correct this text.

Code quote:

        Azure service endpoint. This paramter is required if the quorum type is set to NodeAndCloudMajority.
        The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 193 at r4 (raw file):

        'NodeAndCloudMajority'
        {
            Set-ClusterQuorum -CloudWitness -AccountName $Resource -AccessKey $StorageAccountAccessKey -Endpoint $Endpoint

This should only use parameter Endpoint if it is set.

Suggestion:

$setClusterQuorumParameters = @{
    CloudWitness = $true
    AccountName  = $Resource
    AccessKey    = $StorageAccountAccessKey
}

if ($PSBoundParameters.ContainsKey('Endpoint'))
{
    $setClusterQuorumParameters.Endpoint = $Endpoint
}

Set-ClusterQuorum @setClusterQuorumParameters

source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 214 at r4 (raw file):

    .PARAMETER Endpoint
        Azure service endpoint. This paramter is required if the quorum type is set to NodeAndCloudMajority.

This is no longer accurate as it is now optional, we no longer have a default value. We should correct this text.

Code quote:

        Azure service endpoint. This paramter is required if the quorum type is set to NodeAndCloudMajority.
        The default value is core.windows.net and only need to override when using Azure Government Cloud (eg: core.usgovcloudapi.net)

tests/Unit/DSC_ClusterQuorum.Tests.ps1 line 166 at r4 (raw file):

                    -and $AccountName -eq $mockQuorumAccountName `
                    -and $AccessKey -eq $mockQuorumAccessKey
                    # -and $Endpoint -eq $mockQuorumAccountEndpoint

If this was not needed we should remove the commented line.

Code quote:

# -and $Endpoint -eq $mockQuorumAccountEndpoint

stale · 2022-09-20T21:27:49Z

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

hungtran84 added 2 commits June 28, 2022 11:08

Add Azure endpoint for ClusterQuorum resource

0da4081

Update changelog

6381b49

hungtran84 mentioned this pull request Jun 28, 2022

ClusterQuorum: Doesn't support Azure Gov Cloud #278

Open

johlju requested changes Jun 28, 2022

View reviewed changes

johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jun 28, 2022

hungts added 4 commits July 1, 2022 11:25

Update test function

67a96ad

Add mock value

fade434

Add mock value

35bf1a6

Add mock value

2147a57

stale bot added the abandoned The pull request has been abandoned. label Jul 30, 2022

johlju self-requested a review July 31, 2022 13:28

johlju requested changes Jul 31, 2022

View reviewed changes

johlju self-requested a review July 31, 2022 13:49

johlju requested changes Jul 31, 2022

View reviewed changes

johlju self-requested a review July 31, 2022 14:08

johlju removed the abandoned The pull request has been abandoned. label Jul 31, 2022

hungts added 3 commits August 1, 2022 09:25

Fix unit test

95b97ad

Fix unit test

91b3549

Fix unit test

a9c955f

johlju requested changes Aug 1, 2022

View reviewed changes

johlju self-requested a review August 1, 2022 17:10

Add the new mock

7ec21f6

johlju requested changes Aug 2, 2022

View reviewed changes

stale bot added the abandoned The pull request has been abandoned. label Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Azure endpoint for ClusterQuorum resource #279

Add Azure endpoint for ClusterQuorum resource #279

hungtran84 commented Jun 28, 2022 •

edited by johlju

Loading

johlju left a comment

hungtran84 commented Jul 1, 2022

stale bot commented Jul 30, 2022

johlju left a comment

johlju commented Jul 31, 2022

azure-pipelines bot commented Jul 31, 2022

johlju left a comment

hungtran84 commented Jul 31, 2022 via email

johlju left a comment

johlju commented Jul 31, 2022

johlju left a comment

codecov bot commented Aug 2, 2022

hungtran84 commented Aug 2, 2022

johlju commented Aug 2, 2022

johlju left a comment

stale bot commented Sep 20, 2022

Add Azure endpoint for ClusterQuorum resource #279

Are you sure you want to change the base?

Add Azure endpoint for ClusterQuorum resource #279

Conversation

hungtran84 commented Jun 28, 2022 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

johlju left a comment

Choose a reason for hiding this comment

hungtran84 commented Jul 1, 2022

stale bot commented Jul 30, 2022

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jul 31, 2022

azure-pipelines bot commented Jul 31, 2022

johlju left a comment

Choose a reason for hiding this comment

hungtran84 commented Jul 31, 2022 via email

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jul 31, 2022

johlju left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2022

Codecov Report

hungtran84 commented Aug 2, 2022

johlju commented Aug 2, 2022

johlju left a comment

Choose a reason for hiding this comment

stale bot commented Sep 20, 2022

hungtran84 commented Jun 28, 2022 •

edited by johlju

Loading