Don't NullRef during Get-AzureDeployment when the cloud service has extended configuration #4788

spaoliello · 2017-10-13T23:27:32Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

…ice has extended configuration

azuresdkci · 2017-10-13T23:30:17Z

Can one of the admins verify this patch?

cormacpayne · 2017-10-16T20:25:48Z

@azuresdkci add to whitelist

vanbasten2323 · 2017-10-18T20:58:28Z

src/ServiceManagement/Compute/Commands.ServiceManagement/Model/RoleConfiguration.cs

@@ -104,8 +105,18 @@ internal XElement Serialize()
            {
                XElement certificateElement = new XElement(this.ns + "Certificate");
                certificateElement.SetAttributeValue("name", certificate.Key);
-                certificateElement.SetAttributeValue("thumbprint", certificate.Value.Thumbprint);
-                certificateElement.SetAttributeValue("thumbprintAlgorithm", certificate.Value.ThumbprintAlgorithm);
+                if (certificate.Value.Thumbprint != null)


Could certificate.Value be null?

No. It's a value driven by the foreach loop over existing elements.

I meant this.Certificates is of type Dictionary<string, CertificateConfiguration>();. and the for loop make sure that variable "certificate" is not null... I wonder if "certificate.Value' can be null.

Oh, I see what you're asking.
Certificates.value won't be null because in the deserializer where it's set up, an element to the Dictionary is only added after the CertificateConfiguration object is created using an Object Initializer (so it can't be null).
See lines 44-53.

vanbasten2323 · 2017-10-18T21:02:27Z

src/ServiceManagement/Compute/Commands.ServiceManagement/Model/RoleConfiguration.cs

+                }
+                if (certificate.Value.ThumbprintAlgorithm != null)
+                {
+                    certificateElement.SetAttributeValue("thumbprintAlgorithm", certificate.Value.ThumbprintAlgorithm);


Would it be better to define a constant string instead of using hard coded string literals?

I was following the existing pattern of the file. There are no constants at all.

markcowl · 2017-10-23T17:40:17Z

@spaoliello @vanbasten2323 The PR is failing for CredScan issues. Go to http://aka.ms/credscan for information on suppressing any false positives. The report is here: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/70/artifact/src/Package/credscan-matches.tsv

spaoliello · 2017-10-23T21:21:27Z

Tests were added, and are passing.

cormacpayne · 2017-10-25T16:45:08Z

@hyonholee @vanbasten2323 would one of you be able to sign off on the changes made by Simon?

vanbasten2323 · 2017-10-25T17:53:50Z

src/ServiceManagement/Common/Commands.ScenarioTest/Commands.ScenarioTest.csproj

@@ -224,6 +224,13 @@
    <None Include="Resources\DiagnosticsExtension\Files\diagnostics.wadcfgx">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </None>
+    <None Include="Resources\ServiceManagement\Files\dSMSTest.cscfg">
+      <SubType>Designer</SubType>


you don't need this line ..., do you?

No, I think Visual Studio added it automatically.
I can remove it.

vanbasten2323 · 2017-10-25T17:59:16Z

...viceManagement/Common/Commands.ScenarioTest/Resources/ServiceManagement/Files/dSMSTest.cscfg

+    </ConfigurationSettings>
+    <Certificates>
+      <Certificate name="ThumbprintCert" thumbprint="{{PLACEHOLDER}}" thumbprintAlgorithm="sha1" />
+      <Certificate name="dSMSCert" sourceLocation="/genevacustomers/monitoring/prod/warmpath/test/auxweb/certificates/chained/portaldogfoodmdscert.pfx" />


Is it the correct source location format? A sample source location would be like sourceLocation="https://uswest-dsms.dsms.core.windows.net/dsms/AzureTeam/ABC/certificates/chained/myPFXName.pfx" as shown here https://microsoft.sharepoint.com/teams/AzureSecurityCompliance/Security/_layouts/OneNote.aspx?id=%2Fteams%2FAzureSecurityCompliance%2FSecurity%2FSpecs%20and%20Design%20Docs%2FdSMS%2FOnboarding%2FdSMS%20Support&wd=target%28dSMS%20Onboarding%2FCertificates.one%7C4D38427D-2849-456A-8AED-B754A7AAF6A2%2FOK%20%5C%2F%20Express%20v2%7CB17594EC-657B-4B59-A4E3-F5E510E716C7%2F%29
onenote:https://microsoft.sharepoint.com/teams/AzureSecurityCompliance/Security/Specs%20and%20Design%20Docs/dSMS/Onboarding/dSMS%20Support/dSMS%20Onboarding/Certificates.one#OK%20/%20Express%20v2&section-id={4D38427D-2849-456A-8AED-B754A7AAF6A2}&page-id={B17594EC-657B-4B59-A4E3-F5E510E716C7}&end

The relative sourceLocation works if you are using RDFE to perform deployments. It will automatically use the closest dSMS server location.
I tested this deployment with that format and it does work.

vanbasten2323 · 2017-10-25T19:12:25Z

...nagement/Common/Commands.ScenarioTest/Resources/ServiceManagement/ServiceManagementTests.ps1

+        $result = New-AzureDeployment -ServiceName $svcName -Package $cspkg -Configuration $cscfgChanged -Label $svcName -Slot Staging;
+
+		# Get Deployment
+		$deploy = Get-AzureDeployment -ServiceName $svcName -Slot Staging;


So before the change, if the you have a line in cscfg like: , then you would get null ref error when you create a new deployment. Is my understanding correct?

Yes. If you didn't supply a thumbprint attribute, it would NullRef when you perform Get-AzureDeployment

Then shouldn't the NullRef exception be thrown when you do New-AzureDeployment? The PR topic says the exception happens when you do Get-AzureDeployment

The NullRef was only thrown when I ran Get-AzureDeployment. The test goes through CRUD to ensure it's not occurring anywhere else. I'd rather have extra coverage :)

vanbasten2323 · 2017-10-25T20:14:19Z

...nagement/Common/Commands.ScenarioTest/Resources/ServiceManagement/ServiceManagementTests.ps1

+		$newConfig | Set-Content $cscfgChanged;
+
+		# Update configuration
+		$result = Set-AzureDeployment -Config -ServiceName $svcName -Configuration $cscfgChanged -Slot Staging;


Why do you need to get a new config and do a Set-AzureDeployment on the new config? I don't quite understand.

It's to test round-trip. Deploy (tests New) Get (tests Get) Update (tests Set).
I wanted to make sure all parts could handle config without thumbprints.

vanbasten2323 · 2017-10-25T20:52:58Z

Looks good to me.

markcowl · 2017-11-01T22:23:10Z

@vanbasten2323 Can you sign off based on the responses to your comments?

vanbasten2323 · 2017-11-01T23:10:21Z

@markcowl Approved. Sorry for the delay. It's my first time to approve the PR on GitHub. Was not familiar with the approve procedure before.

markcowl · 2017-11-02T00:27:41Z

@vanbasten2323 Thanks for the review.

LGTM to me as well.

On demand pr here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/114/

Don't NullRef when performing Get-AzureDeployment when the cloud serv…

2bb4cb3

…ice has extended configuration

azuresdkci added the needs-review label Oct 13, 2017

azuresdkci assigned cormacpayne Oct 13, 2017

vanbasten2323 reviewed Oct 18, 2017

View reviewed changes

cormacpayne removed their assignment Oct 18, 2017

spaoliello added 2 commits October 20, 2017 17:43

Add tests

7fc96a1

Remove summy secrets from test

4a7653f

markcowl added needs-revision and removed needs-review labels Oct 23, 2017

markcowl assigned spaoliello Oct 23, 2017

Updated test session record

027ec89

cormacpayne assigned hyonholee and vanbasten2323 and unassigned spaoliello Oct 25, 2017

cormacpayne added needs-review and removed needs-revision labels Oct 25, 2017

vanbasten2323 reviewed Oct 25, 2017

View reviewed changes

Remove spurious metadata

f9a7cf5

vanbasten2323 reviewed Oct 25, 2017

View reviewed changes

vanbasten2323 approved these changes Nov 1, 2017

View reviewed changes

vanbasten2323 closed this Nov 1, 2017

vanbasten2323 reopened this Nov 1, 2017

markcowl removed the needs-review label Nov 2, 2017

markcowl added the Waiting for On Demand label Nov 2, 2017

cormacpayne merged commit 8f54a4b into Azure:preview Nov 2, 2017

Don't NullRef during Get-AzureDeployment when the cloud service has extended configuration #4788

Don't NullRef during Get-AzureDeployment when the cloud service has extended configuration #4788

Uh oh!

Conversation

spaoliello commented Oct 13, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azuresdkci commented Oct 13, 2017

Uh oh!

cormacpayne commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spaoliello commented Oct 23, 2017

Uh oh!

cormacpayne commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanbasten2323 Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanbasten2323 commented Oct 25, 2017

Uh oh!

markcowl commented Nov 1, 2017

Uh oh!

vanbasten2323 commented Nov 1, 2017

Uh oh!

markcowl commented Nov 2, 2017

Uh oh!

Uh oh!

spaoliello commented Oct 13, 2017 •

edited by azuresdkci

Loading

markcowl commented Oct 23, 2017 •

edited

Loading

vanbasten2323 Oct 25, 2017 •

edited

Loading