Azure support #1939
Azure support #1939
Conversation
Signed-off-by: radu-matei <matei.radu94@gmail.com>
Signed-off-by: radu-matei <matei.radu94@gmail.com>
| ## Build an image | ||
|
|
||
| > This is a preliminary example image with SSHD and Docker services. In the future, there will be an `azure.yml` file in the `examples` directory | ||
|
|
| - /var/lib/docker:/var/lib/docker | ||
| - /lib/modules:/lib/modules | ||
| - name: dev | ||
| image: "radumatei/linuxkit-dev:dev" |
There was a problem hiding this comment.
can we make the examples so that they only require LinuxKit images or standard images from the library?
| @@ -0,0 +1,101 @@ | |||
| kernel: | |||
There was a problem hiding this comment.
could this YAML file be update to ressemble the other ones. The hashes don't match the latest and neither do the configs for the containers.
| "os/exec" | ||
| "time" | ||
|
|
||
| "path/filepath" |
There was a problem hiding this comment.
could we include the standard imports in line and not as a separate sections. None of the other code does this.
| flags := flag.NewFlagSet("azure", flag.ExitOnError) | ||
| invoked := filepath.Base(os.Args[0]) | ||
| flags.Usage = func() { | ||
| fmt.Printf("USAGE: %s push azure [options] [name]\n\n", invoked) |
There was a problem hiding this comment.
[name] seems to indicate that the name is optional. Should also be path instead of name, like in the qemu runner. Just noticed the GCP backend has the same, need to fix that
| } | ||
|
|
||
| func createPublicIPAddress(resourceGroup resources.Group, ipName, location string) *network.PublicIPAddress { | ||
| fmt.Printf("\nCreating public IP Address in resource group %s, with name %s", *resourceGroup.Name, ipName) |
| publicIPAddress, err := publicIPAddressesClient.Get(*resourceGroup.Name, ipName, "") | ||
| if err != nil { | ||
| fmt.Println(err.Error()) | ||
| log.Fatalf("Unable to retrieve public IP address") |
|
|
||
| networkInterface, err := interfacesClient.Get(*resourceGroup.Name, networkInterfaceName, "") | ||
| if err != nil { | ||
| log.Fatalf("Unable to retrieve network interface") |
There was a problem hiding this comment.
would it be worth logging the error?
| } | ||
|
|
||
| func createVirtualMachine(resourceGroup resources.Group, storageAccountName string, virtualMachineName string, networkInterface network.Interface, publicIPAddress network.PublicIPAddress, location string) { | ||
| fmt.Printf("\nCreating virtual machine in resource group %s, with name %s, in location %s", *resourceGroup.Name, virtualMachineName, location) |
There was a problem hiding this comment.
please remove the \n from the start.
| func getEnvVarOrExit(varName string) string { | ||
| value := os.Getenv(varName) | ||
| if value == "" { | ||
| log.Fatalf(fmt.Sprintf("Missing environment variable %s\n", varName)) |
There was a problem hiding this comment.
the fmt.Sprintf() should not be necessary
| func initializeAzureClients(subscriptionID, tenantID, clientID, clientSecret string) { | ||
| oAuthConfig, err := adal.NewOAuthConfig(azure.PublicCloud.ActiveDirectoryEndpoint, tenantID) | ||
| if err != nil { | ||
| fmt.Printf(err.Error()) |
There was a problem hiding this comment.
this should probably be only in the log, without any fmt.Printf, since the fatal prints to stdout:
ex: log.Fatalf("Cannot get oAuth configuration: %v", err)
|
|
||
| token, err := adal.NewServicePrincipalToken(*oAuthConfig, clientID, clientSecret, azure.PublicCloud.ResourceManagerEndpoint) | ||
| if err != nil { | ||
| fmt.Printf(err.Error()) |
| } | ||
| case _, ok := <-errorChannel: | ||
| if !ok { | ||
| errorChannel = nil |
There was a problem hiding this comment.
I'm not sure I understand this logic - what do we do in the case of a non-nil errorChannel? It seems like we aren't handling the error?
|
|
||
| subnetParameters := network.Subnet{ | ||
| SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ | ||
| AddressPrefix: to.StringPtr("10.0.0.0/24"), |
There was a problem hiding this comment.
Additionally, if it is hardcoded - can we make it a const?
| Location: &location, | ||
| VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ | ||
| AddressSpace: &network.AddressSpace{ | ||
| AddressPrefixes: &[]string{"10.0.0.0/16"}, |
There was a problem hiding this comment.
same comment here re: what 10.0.0.0 is and if we can const-ify it?
| } | ||
|
|
||
| func createSubnet(resourceGroup resources.Group, virtualNetworkName, subnetName string) *network.Subnet { | ||
| fmt.Printf("\nCreating subnet %s in resource group %s, within virtual network %s", subnetName, *resourceGroup.Name, virtualNetworkName) |
There was a problem hiding this comment.
same as @rneugeba's comment re: removing \n
| break | ||
| } | ||
| } | ||
| time.Sleep(time.Second * 5) |
There was a problem hiding this comment.
It got lost through the commit messages. Basically, just after I submit the creation of the public IP address I need to retrieve it through the publicIPAddressesClient. Most of the times, there isn't enough time so that it gets registered, so I added a little delay before retrieving it.
Maybe should try to await the creation.
| }, | ||
| OsProfile: &compute.OSProfile{ | ||
| ComputerName: to.StringPtr("linuxkit"), | ||
| AdminUsername: to.StringPtr("dummyusername"), |
There was a problem hiding this comment.
this username and password shouldn't be in code, nor should we ever hardcode a password for users
There was a problem hiding this comment.
Maybe make the OS Profile take in a SSH public key instead of username/password? And bubble up the SSH public key as a parameter?
There was a problem hiding this comment.
Will try to add Disable Password Auth
There was a problem hiding this comment.
The thing is unless you manually add the SSHD service when building the image, you cannot SSH into the machine.
There was a problem hiding this comment.
Thats fine, if you don't configure login in the image there should be no way to login.
There was a problem hiding this comment.
Just to be clear: those parameters are only specified so that the deployment gets validated. Since there is no Azure Linux Agent, those values will not be enforced and users can only connect if they included the SSHD service.
What do you think should be the way to go here?
| networkInterface := createNetworkInterface(*group, networkInterfaceName, *publicIPAddress, *subnet, *location) | ||
| go createVirtualMachine(*group, *accountName, virtualMachineName, *networkInterface, *publicIPAddress, *location) | ||
|
|
||
| fmt.Printf("\nStarted deployment of virtual machine %s in resource group %s", virtualMachineName, *group.Name) |
There was a problem hiding this comment.
same comment re: leading \n
| time.Sleep(time.Second * 5) | ||
|
|
||
| fmt.Printf("\nNOTE: Since you created a minimal VM without the Azure Linux Agent, the portal will notify you that the deployment failed. After around 50 seconds try connecting to the VM") | ||
| fmt.Printf("\nssh -i path-to-key root@%s\n", *publicIPAddress.DNSSettings.Fqdn) |
There was a problem hiding this comment.
same comment re: leading \n for these two lines
| log.Fatalf("Cannot get oAuth configuration") | ||
| } | ||
|
|
||
| token, err := adal.NewServicePrincipalToken(*oAuthConfig, clientID, clientSecret, azure.PublicCloud.ResourceManagerEndpoint) |
There was a problem hiding this comment.
Similar in line with above comment on Azure API end points - instead of hardcoding PublicCloud.ResourceManagerEndpoint, please make the resource manager endpoint configurable and PublicCloud the default.
| ) | ||
|
|
||
| func initializeAzureClients(subscriptionID, tenantID, clientID, clientSecret string) { | ||
| oAuthConfig, err := adal.NewOAuthConfig(azure.PublicCloud.ActiveDirectoryEndpoint, tenantID) |
There was a problem hiding this comment.
Instead of hardcoding PublicCloud. ActiveDirectoryEndpoint, please make the active directory endpoint configurable and PublicCloud the default. That way this can easily be used to target any other Azure environment like Germany, China, Government as well as Private Azure Stack setups.
|
|
||
| - you can [get the Azure tenant ID following the instructions here](https://docs.microsoft.com/en-us/azure/azure-resource-manager/resource-group-create-service-principal-portal#get-tenant-id) | ||
| - to get the subscription ID, log in to the Azure portal, then go to Subscriptions | ||
| - then, you need to [create an Azure Active Directory application and retrieve its ID and secret following the instructions here](https://docs.microsoft.com/en-us/azure/azure-resource-manager/resource-group-create-service-principal-portal#create-an-azure-active-directory-application) |
There was a problem hiding this comment.
FYI ... docker4x/create-sp-azure is a quick and easy way to generate a SP. https://docs.docker.com/docker-for-azure/#configuration has usage details under SP.
|
|
||
| subnetParameters := network.Subnet{ | ||
| SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ | ||
| AddressPrefix: to.StringPtr("10.0.0.0/24"), |
There was a problem hiding this comment.
Instead of creating all the resources (network, public IP, VM, etc) along with parameters through Go code, how about creating a JSON ARM template to create the VM and associated resources? You can instantiate the ARM template with parameters specified in JSON using the Go APIs to create a resource group and get the same result. Please see example here: https://github.com/Azure/azure-quickstart-templates/tree/master/101-vm-from-user-image
I think the ARM template will be much easier to maintain than the Go code.
| }, | ||
| OsProfile: &compute.OSProfile{ | ||
| ComputerName: to.StringPtr("linuxkit"), | ||
| AdminUsername: to.StringPtr("dummyusername"), |
There was a problem hiding this comment.
Maybe make the OS Profile take in a SSH public key instead of username/password? And bubble up the SSH public key as a parameter?
|
Instead of creating all the resources (network, public IP, VM, etc) along with parameters through Go code, how about creating a JSON ARM template to create the VM and associated resources? You can instantiate the ARM template with parameters specified in JSON using the Go APIs to create a resource group and get the same result. Please see example here: https://github.com/Azure/azure-quickstart-templates/tree/master/101-vm-from-user-image I think the ARM template will be much easier to maintain than the Go code. |
|
Closed in favour of #1933 the original one. |
| storageAccountKeyArg := fmt.Sprintf("STORAGE_ACCOUNT_KEY=%s", *keys[0].Value) | ||
| vhdPath := fmt.Sprintf("VHD_PATH=/vhds/%s", image) | ||
|
|
||
| output, err := exec.Command("docker", "run", "-v", dockerMount, "-e", vhdPath, "-e", storageAccountNameArg, "-e", storageAccountKeyArg, "radumatei/azure-vhd-upload:alpine").CombinedOutput() |
There was a problem hiding this comment.
Here is the thing I would like you to review: there is already a very competent tool that uploads VHDs to Azure. Because the Go SDK changed in the meantime, it meant I should have had to add two versions of it. Didn't want to do it, so I think this is the easiest way for the moment.
For the first version, until the SDK stabilizes, do you think we can have this way of uploading the image?
There was a problem hiding this comment.
There was a problem hiding this comment.
Not really happy with that. We are trying to remove the docker dependencies in LinuxKit (now mostly gone) as it is a pain to have to have Docker installed in CI for example.
|
Updated now in #1933 |
cc @radu-matei