Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

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

Merged
merged 5 commits into from
Jun 14, 2018
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a2c517
adding nil check for managed cluster and its properties object
Jun 6, 2018
32315d9
adding check for update scenario
Jun 6, 2018
0f0bc77
adding merge code and UTs
Jun 12, 2018
58a0838
correcting UTs
Jun 12, 2018
90eaffa
correcting lint errors
Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/api/apiloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.
-- | -

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@amanohar amanohar Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
// verify orchestrator version
if len(managedCluster.Properties.KubernetesVersion) > 0 && !common.AllKubernetesSupportedVersions[managedCluster.Properties.KubernetesVersion] {
return nil, IsSSHAutoGenerated, a.Translator.Errorf("The selected orchestrator version '%s' is not supported", managedCluster.Properties.KubernetesVersion)
Expand Down