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

Fix validation for firstConsecutiveStaticIP #4209

Closed
wants to merge 2 commits into from
Closed

Fix validation for firstConsecutiveStaticIP #4209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a088cb
Fix validation for firstConsecutiveStaticIP
ritazh Nov 8, 2018
29ad616
Add unit test
ritazh Nov 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/deploy.go
View file
Open in desktop
Expand Up @@ -383,6 +383,9 @@ func (dc *deployCmd) validateApimodel() (*api.ContainerService, string, error) {
return nil, "", errors.New("when using the kubernetes orchestrator, must either set useManagedIdentity in the kubernetes config or set --client-id and --client-secret or KeyvaultSecretRef of secret (also available in the API model)")
}
}
if p.MasterProfile != nil && p.MasterProfile.IsVirtualMachineScaleSets() && p.MasterProfile.VnetSubnetID != "" && p.MasterProfile.FirstConsecutiveStaticIP != "" {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go in the validate.go area, as this validation is only a convenience for acs-engine deploy CLI usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

This logic was originally in validate.go. But as we have seen in e2e tests, what we want to do here is to validate the user-provided apimodel, not the generated apimodel. Since we want to notify the user that we are generating this firstConsecutiveStaticIP for them if they are using custom vnet and VMSS master profile, instead of asking them to provide a value.

When we run e2e tests, cli_provisioner.go calls engine.ParseOutput in template.go which calls apiloader.LoadContainerServiceFromFile(path, true, false, nil) that triggers validate.goon the generated apimodel.

acs-engine/test/e2e/runner/cli_provisioner.go

Line 223 in 29ad616

csGenerated, err := engine.ParseOutput(engCfg.GeneratedDefinitionPath + "/apimodel.json")

This causes the e2e tests to fail since the generated apimodel now includes thefirstConsecutiveStaticIP we generated based on the vnetCidr provided by the user.

acs-engine/test/e2e/engine/template.go

Lines 283 to 294 in 29ad616

// ParseOutput takes the generated api model and will parse that into a api.ContainerService
func ParseOutput(path string) (*api.ContainerService, error) {
locale, err := i18n.LoadTranslations()
if err != nil {
return nil, errors.Errorf(fmt.Sprintf("error loading translation files: %s", err.Error()))
}
apiloader := &api.Apiloader{
Translator: &i18n.Translator{
Locale: locale,
},
}
containerService, _, err := apiloader.LoadContainerServiceFromFile(path, true, false, nil)

When we run acs-engine deploy, validateApimodel in deploy.go validates the user-provided apimodel before dc.containerService.SetPropertiesDefaults is called.

acs-engine/cmd/deploy.go

Lines 88 to 91 in 29ad616

if _, _, err := dc.validateApimodel(); err != nil {
log.Fatalf("Failed to validate the apimodel after populating values: %s", err.Error())
}
return dc.run()

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, vlabs/validate.go should only validate user input, not the generated data...

return nil, "", errors.Errorf("when masterProfile's availabilityProfile is VirtualMachineScaleSets and a vnetSubnetID is specified, the firstConsecutiveStaticIP should be empty and will be determined by an offset from the first IP in the vnetCidr %s", p.MasterProfile.FirstConsecutiveStaticIP)
}
}

// This isn't terribly elegant, but it's the easiest way to go for now w/o duplicating a bunch of code
Expand Down
182 changes: 182 additions & 0 deletions cmd/deploy_test.go
View file
Open in desktop
Expand Up @@ -54,6 +54,82 @@ const ExampleAPIModelWithoutServicePrincipalProfile = `{
}
}
`
const ExampleAPIModelWithCustomVnetVMSSAndFirstConsecutiveStaticIP = `{
"apiVersion": "vlabs",
"properties": {
"orchestratorProfile": { "orchestratorType": "Kubernetes", "kubernetesConfig": { "useManagedIdentity": %s, "etcdVersion" : "2.3.8" } },
"masterProfile": { "count": 1, "dnsPrefix": "mytestcluster", "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/master",
"agentVnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"vnetCidr": "10.240.192.0/21",
"firstConsecutiveStaticIP": "10.240.192.4",
"availabilityProfile": "VirtualMachineScaleSets" },
"agentPoolProfiles": [ { "name": "linuxpool1", "count": 2, "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"availabilityProfile": "VirtualMachineScaleSets" } ],
"windowsProfile": { "adminUsername": "azureuser", "adminPassword": "replacepassword1234$" },
"linuxProfile": { "adminUsername": "azureuser", "ssh": { "publicKeys": [ { "keyData": "" } ] }
},
"servicePrincipalProfile": { "clientId": "%s", "secret": "%s" }
}
}
`
const ExampleAPIModelWithCustomVnetVMSSAndNoFirstConsecutiveStaticIP = `{
"apiVersion": "vlabs",
"properties": {
"orchestratorProfile": { "orchestratorType": "Kubernetes", "kubernetesConfig": { "useManagedIdentity": %s, "etcdVersion" : "2.3.8" } },
"masterProfile": { "count": 1, "dnsPrefix": "mytestcluster", "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/master",
"agentVnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"vnetCidr": "10.240.192.0/21",
"availabilityProfile": "VirtualMachineScaleSets" },
"agentPoolProfiles": [ { "name": "linuxpool1", "count": 2, "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"availabilityProfile": "VirtualMachineScaleSets" } ],
"windowsProfile": { "adminUsername": "azureuser", "adminPassword": "replacepassword1234$" },
"linuxProfile": { "adminUsername": "azureuser", "ssh": { "publicKeys": [ { "keyData": "" } ] }
},
"servicePrincipalProfile": { "clientId": "%s", "secret": "%s" }
}
}
`
const ExampleAPIModelWithCustomVnetVMASAndFirstConsecutiveStaticIP = `{
"apiVersion": "vlabs",
"properties": {
"orchestratorProfile": { "orchestratorType": "Kubernetes", "kubernetesConfig": { "useManagedIdentity": %s, "etcdVersion" : "2.3.8" } },
"masterProfile": { "count": 1, "dnsPrefix": "mytestcluster", "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/master",
"agentVnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"vnetCidr": "10.240.192.0/21",
"firstConsecutiveStaticIP": "10.240.192.4" },
"agentPoolProfiles": [ { "name": "linuxpool1", "count": 2, "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"availabilityProfile": "VirtualMachineScaleSets" } ],
"windowsProfile": { "adminUsername": "azureuser", "adminPassword": "replacepassword1234$" },
"linuxProfile": { "adminUsername": "azureuser", "ssh": { "publicKeys": [ { "keyData": "" } ] }
},
"servicePrincipalProfile": { "clientId": "%s", "secret": "%s" }
}
}
`
const ExampleAPIModelWithCustomVnetVMASAndNoFirstConsecutiveStaticIP = `{
"apiVersion": "vlabs",
"properties": {
"orchestratorProfile": { "orchestratorType": "Kubernetes", "kubernetesConfig": { "useManagedIdentity": %s, "etcdVersion" : "2.3.8" } },
"masterProfile": { "count": 1, "dnsPrefix": "mytestcluster", "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/master",
"agentVnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"vnetCidr": "10.240.192.0/21" },
"agentPoolProfiles": [ { "name": "linuxpool1", "count": 2, "vmSize": "Standard_D2_v2",
"vnetSubnetId": "/subscriptions/SUB/resourceGroups/RG/providers/Microsoft.Network/virtualNetworks/VN/subnets/agent",
"availabilityProfile": "VirtualMachineScaleSets" } ],
"windowsProfile": { "adminUsername": "azureuser", "adminPassword": "replacepassword1234$" },
"linuxProfile": { "adminUsername": "azureuser", "ssh": { "publicKeys": [ { "keyData": "" } ] }
},
"servicePrincipalProfile": { "clientId": "%s", "secret": "%s" }
}
}
`

//mockAuthProvider implements AuthProvider and allows in particular to stub out getClient()
type mockAuthProvider struct {
Expand Down Expand Up @@ -502,6 +578,112 @@ func testAutodeployCredentialHandling(t *testing.T, useManagedIdentity bool, cli
}
}

func TestAPIModelWithFirstConsecutiveStaticIP(t *testing.T) {
apiloader := &api.Apiloader{
Translator: nil,
}

apimodel := getAPIModel(ExampleAPIModelWithCustomVnetVMSSAndFirstConsecutiveStaticIP, false, "", "")
cs, ver, err := apiloader.DeserializeContainerService([]byte(apimodel), false, false, nil)
if err != nil {
t.Fatalf("unexpected error deserializing the example apimodel: %s", err)
}

// deserialization happens in validate(), but we are testing just the default
// setting that occurs in autofillApimodel (which is called from validate)
// Thus, it assumes that containerService/apiVersion are already populated
deployCmd := &deployCmd{
apimodelPath: "./this/is/unused.json",
outputDirectory: "_test_output",
forceOverwrite: true,
location: "westus",

containerService: cs,
apiVersion: ver,

client: &armhelpers.MockACSEngineClient{},
authProvider: &mockAuthProvider{
authArgs: &authArgs{},
},
}

err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
defer os.RemoveAll(deployCmd.outputDirectory)

_, _, err = deployCmd.validateApimodel()
if err == nil {
t.Fatalf("expected error validating apimodel with custom vnet, vmss masters, and firstConsecutiveStaticIP: %s", err)
}

apimodel = getAPIModel(ExampleAPIModelWithCustomVnetVMSSAndNoFirstConsecutiveStaticIP, false, "", "")
cs, _, err = apiloader.DeserializeContainerService([]byte(apimodel), false, false, nil)
if err != nil {
t.Fatalf("unexpected error deserializing the example apimodel: %s", err)
}

deployCmd.containerService = cs

err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
defer os.RemoveAll(deployCmd.outputDirectory)

_, _, err = deployCmd.validateApimodel()
if err != nil {
t.Fatalf("unexpected error validating apimodel with custom vnet, vmss masters, and no firstConsecutiveStaticIP: %s", err)
}

apimodel = getAPIModel(ExampleAPIModelWithCustomVnetVMASAndNoFirstConsecutiveStaticIP, false, "", "")
cs, _, err = apiloader.DeserializeContainerService([]byte(apimodel), false, false, nil)
if err != nil {
t.Fatalf("unexpected error deserializing the example apimodel: %s", err)
}

deployCmd.containerService = cs

err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
defer os.RemoveAll(deployCmd.outputDirectory)

_, _, err = deployCmd.validateApimodel()
if err == nil {
t.Fatalf("expected error validating apimodel with custom vnet, vmas masters, and no firstConsecutiveStaticIP: %s", err)
}

apimodel = getAPIModel(ExampleAPIModelWithCustomVnetVMASAndFirstConsecutiveStaticIP, false, "", "")
cs, _, err = apiloader.DeserializeContainerService([]byte(apimodel), false, false, nil)
if err != nil {
t.Fatalf("unexpected error deserializing the example apimodel: %s", err)
}

deployCmd.containerService = cs

err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
defer os.RemoveAll(deployCmd.outputDirectory)

_, _, err = deployCmd.validateApimodel()
if err != nil {
t.Fatalf("unexpected error validating apimodel with custom vnet, vmas masters, and firstConsecutiveStaticIP: %s", err)
}
}

func TestDeployCmdMergeAPIModel(t *testing.T) {
d := &deployCmd{}
d.apimodelPath = "../pkg/acsengine/testdata/simple/kubernetes.json"
Expand Down
6 changes: 0 additions & 6 deletions pkg/api/vlabs/validate.go
View file
Open in desktop
Expand Up @@ -363,12 +363,6 @@ func (a *Properties) validateMasterProfile() error {
}
}

if a.OrchestratorProfile.OrchestratorType == Kubernetes {
if m.IsVirtualMachineScaleSets() && m.VnetSubnetID != "" && m.FirstConsecutiveStaticIP != "" {
return errors.New("when masterProfile's availabilityProfile is VirtualMachineScaleSets and a vnetSubnetID is specified, the firstConsecutiveStaticIP should be empty and will be determined by an offset from the first IP in the vnetCidr")
}
}

if m.ImageRef != nil {
if err := m.ImageRef.validateImageNameAndGroup(); err != nil {
return err
Expand Down
21 changes: 0 additions & 21 deletions pkg/api/vlabs/validate_test.go
View file
Open in desktop
Expand Up @@ -2147,27 +2147,6 @@ func TestProperties_ValidateVNET(t *testing.T) {
},
expectedMsg: "when master profile is using VirtualMachineScaleSets and is custom vnet, set \"vnetsubnetid\" and \"agentVnetSubnetID\" for master profile",
},
{
name: "User-provided MasterProfile FirstConsecutiveStaticIP when master is VMSS",
masterProfile: &MasterProfile{
VnetSubnetID: validVNetSubnetID,
Count: 1,
DNSPrefix: "foo",
VMSize: "Standard_DS2_v2",
AvailabilityProfile: VirtualMachineScaleSets,
FirstConsecutiveStaticIP: "10.0.0.4",
},
agentPoolProfiles: []*AgentPoolProfile{
{
Name: "agentpool",
VMSize: "Standard_D2_v2",
Count: 1,
AvailabilityProfile: VirtualMachineScaleSets,
VnetSubnetID: validVNetSubnetID,
},
},
expectedMsg: "when masterProfile's availabilityProfile is VirtualMachineScaleSets and a vnetSubnetID is specified, the firstConsecutiveStaticIP should be empty and will be determined by an offset from the first IP in the vnetCidr",
},
{
name: "Invalid vnetcidr",
masterProfile: &MasterProfile{
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/engine/template.go
View file
Open in desktop
Expand Up @@ -169,6 +169,9 @@ func Build(cfg *config.Config, masterSubnetID string, agentSubnetID string, isVM

if config.CreateVNET {
if isVMSS {
if cs.Properties.MasterProfile.FirstConsecutiveStaticIP != "" {
return nil, errors.Errorf("when masterProfile's availabilityProfile is VirtualMachineScaleSets and a vnetSubnetID is specified, the firstConsecutiveStaticIP should be empty and will be determined by an offset from the first IP in the vnetCidr %s", cs.Properties.MasterProfile.FirstConsecutiveStaticIP)
}
cs.ContainerService.Properties.MasterProfile.VnetSubnetID = masterSubnetID
cs.ContainerService.Properties.MasterProfile.AgentVnetSubnetID = agentSubnetID
for _, p := range cs.ContainerService.Properties.AgentPoolProfiles {
Expand Down