-
Notifications
You must be signed in to change notification settings - Fork 431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for provisioning single master clusters on Azure through clusterctl #2
Conversation
The wrappers have been moved into their own directory so it's more clear that they're for testing purposes and do not have to do with the actual functionality of the components they used share files with. More unit tests have been added for VMs/deployments, and existing unit tests have been refactored to use the wrappers.
this is to make upstreaming the azure provider easier, as it better follows the skeleton's structure
…racked a vendored file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Michael,
Sorry it took me a while to look at this PR. I have several comments and you may not be able to address all of them by tomorrow. Please create issues for them (if necessary).
@@ -81,6 +77,10 @@ | |||
"startup_script": { | |||
"defaultValue": null, | |||
"type": "string" | |||
}, | |||
"sshKeyData": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this to sshPublicKey
? Or is it ssh_public_key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to sshPublicKey
"provisioningState": "Succeeded", | ||
"protocol": "TCP", | ||
"sourcePortRange": "*", | ||
"destinationPortRange": "6443", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this port for? Can you add a comment? Would this be something that a user might want to configure?
machine.ObjectMeta.Annotations = make(map[string]string) | ||
} | ||
if azure.v1Alpha1Client != nil { | ||
machine.ObjectMeta.Annotations["dummy"] = "dummy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary? I thought this was for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Cluster API currently determines that a machine's been created/updated by whether an annotation has been set, so rather than "dummy", some useful information could be put there instead, like the machine name. Changing to include some relevant info.
return "", fmt.Errorf("Error setting sftp client: %s", err) | ||
} | ||
|
||
remoteFile := "/home/ClusterAPI/.kube/config" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this parameterized with the SSHUser
constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"value": *base64EncodeCommand("echo 'Hello world!'"), | ||
"value": *base64EncodeCommand(startupScript), | ||
}, | ||
"sshKeyData": map[string]interface{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sshPublicKey
or ssh_public_key
apt-get update | ||
apt-get install -y kubelet kubeadm kubectl | ||
|
||
CLUSTER_DNS_SERVER=$(prips "10.96.0.0/12" | head -n 11 | tail -n 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that can be taken from cluster.yaml
?
apt-get install -y kubelet kubeadm kubectl | ||
|
||
CLUSTER_DNS_SERVER=$(prips "10.96.0.0/12" | head -n 11 | tail -n 1) | ||
CLUSTER_DNS_DOMAIN="cluster.local" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this, can we get this from cluster.yaml
?
serviceSubnet: "10.96.0.0/12" | ||
token: "testtoken" | ||
controllerManagerExtraArgs: | ||
cluster-cidr: "192.168.0.0/16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that can be taken from cluster.yaml
?
@@ -153,7 +218,62 @@ func TestBase64Encoding(t *testing.T) { | |||
} | |||
|
|||
func TestGetStartupScript(t *testing.T) { | |||
expectedStartupScript := "echo 'Hello world!'" | |||
expectedStartupScript := `( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can place the startup script to a file and just read it from here? That way we don't have to do a copy and paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried making this change but was running into some issues where creating the azure deployment would silently fail. Don't think I'll have time to work through it today.
cmd/cluster-controller/Dockerfile
Outdated
# limitations under the License. | ||
|
||
# Reproducible builder image | ||
FROM golang:1.10.0 as builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are using 1.10.2. Should this be built with 1.10.2 as well? Or can the test use 1.10.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to just set the tests to use 1.10.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like travis doesn't support 1.10.0, but it does support 1.10.2. So I switched the build to 1.10.2.
…generation of different machines.yaml
Rename module to machine-api-provider-azure
# This is the 1st commit message: adding *.yaml to .gitattributes for easier WSL development with autocrlf # The commit message kubernetes-sigs#2 will be skipped: # adding *.yaml to .gitattributes for easier WSL development with autocrlf
Sync latest repo
� This is the 1st commit message: add helm chart � The commit message #2 will be skipped: � Expose AKS Preview Features � � Co-authored-by: Jon Huhn <[email protected]>
� This is the 1st commit message: add helm chart � The commit message #2 will be skipped: � Expose AKS Preview Features � � Co-authored-by: Jon Huhn <[email protected]>
No description provided.