adding nil check for managed cluster and its properties object #3197

shrutir25 · 2018-06-06T17:49:23Z

What this PR does / why we need it:
Checks for managed cluster object and properties object to make sure it is not nil

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Fixes nil panics seen here https://jarvis-west.dc.ad.msft.net/C7CCDED7
Special notes for your reviewer:

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

shrutir25 · 2018-06-06T17:50:19Z

/assign @CecileRobertMichon

CecileRobertMichon · 2018-06-06T18:21:50Z

/lgtm

acs-bot · 2018-06-06T18:21:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, shrutir25

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

amanohar · 2018-06-06T18:59:11Z

pkg/api/apiloader.go

@@ -200,6 +200,10 @@ func (a *Apiloader) LoadContainerServiceForAgentPoolOnlyCluster(
 		if e := json.Unmarshal(contents, &managedCluster); e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}
+		// verify managedCluster
+		if managedCluster == nil || managedCluster.Properties == nil {
+			return nil, false, a.Translator.Errorf("properties object in managed cluster should not be nil")


@shrutir25 actually the properties can be nil and is a supported ARM scenario. RPs need to be able to handle this.

Here is the documentation: https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/resource-api-reference.md#put-resource -

properties | Optional. Format not defined by ARM. Settings used to provision or configure the resource. All Resource Provider specific properties must be placed within this. The order of parameters in the request is unspecified. RPs should not rely on any particular ordering. -- | -

@amanohar - so then you are suggesting that we make this check in RP before calling this function in acs-engine right ?

@shrutir25 I meant RP has to support a payload with nil properties and PUT should not fail.

I talked to Gaurav to get clarification. So long as the fields inside Properties are required, we can return 400 if it's not supplied.

In case of update correct behavior is to accept properties=nil as user is not required to supply "required" properties during update.

amanohar · 2018-06-06T19:02:04Z

pkg/api/apiloader.go

@@ -200,6 +200,10 @@ func (a *Apiloader) LoadContainerServiceForAgentPoolOnlyCluster(
 		if e := json.Unmarshal(contents, &managedCluster); e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}
+		// verify managedCluster
+		if managedCluster == nil || managedCluster.Properties == nil {
+			return nil, false, a.Translator.Errorf("properties object in managed cluster should not be nil")


Here is the documentation: https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/resource-api-reference.md#put-resource -

properties | Optional. Format not defined by ARM. Settings used to provision or configure the resource. All Resource Provider specific properties must be placed within this. The order of parameters in the request is unspecified. RPs should not rely on any particular ordering. -- | -

jackfrancis · 2018-06-06T22:57:36Z

/hold

acs-bot · 2018-06-06T23:02:25Z

New changes are detected. LGTM label has been removed.

codecov · 2018-06-06T23:02:26Z

Codecov Report

Merging #3197 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3197      +/-   ##
==========================================
+ Coverage   52.31%   52.32%   +<.01%     
==========================================
  Files         103      103              
  Lines       15458    15454       -4     
==========================================
- Hits         8087     8086       -1     
+ Misses       6643     6640       -3     
  Partials      728      728

amanohar · 2018-06-06T23:02:45Z

@shrutir25 Couple of suggestions:

I recommend using isUpdate param to make a determination of what kind of error to return. If it's an existing MC and nil then create managedCluster.Properties and then merge should happen.
Fix this for both (all) MC versions and not just 2018-03-31.

amanohar · 2018-06-06T23:12:52Z

pkg/api/apiloader.go

@@ -200,6 +200,10 @@ func (a *Apiloader) LoadContainerServiceForAgentPoolOnlyCluster(
 		if e := json.Unmarshal(contents, &managedCluster); e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}
+		// verify managedCluster.Properties is not nil for creating case
+		if (managedCluster == nil || managedCluster.Properties == nil) && !hasExistingCS {


Properties is a pointer. In update case if customer didn't set Properties would it be nil? Will that pointer need initialization in update case?

Also add a test for this in: apiloader_test.go

in update case, wouldn't nil managedCluster.Properties cause panic in line 208?

weinong · 2018-06-13T22:24:32Z

pkg/api/agentPoolOnlyApi/v20170831/merge.go

+)
+
+// Merge existing ManagedCluster attribute into mc
+func (mc *ManagedCluster) Merge(emc *ManagedCluster) error {


@amanohar @JunSun17 I think we should also merge KubernetesConfig

oh nvm this is versioned managed cluster..

* adding nil check for managed cluster and its properties object * adding check for update scenario * adding merge code and UTs * correcting UTs * correcting lint errors

adding nil check for managed cluster and its properties object

0a2c517

ghost assigned shrutir25 Jun 6, 2018

ghost added the in progress label Jun 6, 2018

acs-bot added the size/XS label Jun 6, 2018

acs-bot assigned CecileRobertMichon Jun 6, 2018

acs-bot added the lgtm label Jun 6, 2018

acs-bot added the approved label Jun 6, 2018

CecileRobertMichon requested a review from amanohar June 6, 2018 18:21

amanohar reviewed Jun 6, 2018

View reviewed changes

amanohar suggested changes Jun 6, 2018

View reviewed changes

weinong approved these changes Jun 6, 2018

View reviewed changes

acs-bot added the do-not-merge/hold label Jun 6, 2018

adding check for update scenario

32315d9

acs-bot removed the lgtm label Jun 6, 2018

amanohar reviewed Jun 6, 2018

View reviewed changes

CecileRobertMichon mentioned this pull request Jun 11, 2018

Fixing panic issue in Validate when properties are nil #3242

Merged

3 tasks

Shruti Ramesh added 2 commits June 12, 2018 11:52

adding merge code and UTs

0f0bc77

correcting UTs

58a0838

acs-bot added size/L and removed size/XS labels Jun 13, 2018

correcting lint errors

90eaffa

weinong reviewed Jun 13, 2018

View reviewed changes

CecileRobertMichon added the awaiting review label Jun 13, 2018

weinong approved these changes Jun 14, 2018

View reviewed changes

CecileRobertMichon merged commit 1dc55c0 into master Jun 14, 2018

ghost removed the in progress label Jun 14, 2018

mboersma deleted the fix-nil-panic branch October 5, 2018 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding nil check for managed cluster and its properties object #3197

adding nil check for managed cluster and its properties object #3197

shrutir25 commented Jun 6, 2018

shrutir25 commented Jun 6, 2018

CecileRobertMichon commented Jun 6, 2018

acs-bot commented Jun 6, 2018

amanohar Jun 6, 2018

amanohar Jun 6, 2018

shrutir25 Jun 6, 2018

amanohar Jun 6, 2018

weinong Jun 6, 2018

amanohar Jun 6, 2018 •

edited

Loading

amanohar Jun 6, 2018

jackfrancis commented Jun 6, 2018

acs-bot commented Jun 6, 2018

codecov bot commented Jun 6, 2018 •

edited

Loading

amanohar commented Jun 6, 2018

amanohar Jun 6, 2018

amanohar Jun 6, 2018

weinong Jun 6, 2018

weinong Jun 13, 2018

weinong Jun 13, 2018

adding nil check for managed cluster and its properties object #3197

adding nil check for managed cluster and its properties object #3197

Conversation

shrutir25 commented Jun 6, 2018

shrutir25 commented Jun 6, 2018

CecileRobertMichon commented Jun 6, 2018

acs-bot commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanohar Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Jun 6, 2018

acs-bot commented Jun 6, 2018

codecov bot commented Jun 6, 2018 • edited Loading

Codecov Report

amanohar commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanohar Jun 6, 2018 •

edited

Loading

codecov bot commented Jun 6, 2018 •

edited

Loading