Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Enable manual provisioning for windows machines. #6523
Conversation
|
This patch depends on this patch. |
hoenirvili
referenced this pull request
in juju/utils
Nov 21, 2016
Merged
This patch provides a friendly interface with the masterzen/winrm client #249
|
@hoenirvili can you please check the conflicts? @axw and @natefinch can you please take a peek at this PR? |
|
@mitechie fixed ! |
|
!!build!! |
|
!!build!! |
|
!!build!! |
|
!!build!! |
|
@natefinch what are your thoughts on this patch? |
natefinch
suggested changes
Jan 5, 2017
Mostly ok, but needs some cleanup in the given spots.
Major point is to avoid using JujuConnSuite in the tests. Please test as much as you can without using JujuConnSuite and then write a CI test to actually manually adding a real windows machine.
| + | ||
| + caCert := cert.CACert() | ||
| + if caCert == nil { | ||
| + logger.Infof("Skipping winrm CA validation") |
| + if caCert == nil { | ||
| + logger.Infof("Skipping winrm CA validation") | ||
| + cfg.Insecure = true | ||
| + |
| +package winrmprovisioner | ||
| + | ||
| +var ( | ||
| + WinDetectHardware = detectHardware |
| + }() | ||
| + | ||
| + if err = InitAdministratorUser(&args); err != nil { | ||
| + return machineId, errors.Annotatef(err, |
natefinch
Jan 5, 2017
Contributor
probably clearer to return "" rather than machineId, just so it's clear you're returning an empty string here
axw
Jan 23, 2017
Member
Have you pushed your fixes? I'm pretty sure Nate meant to use a string literal of "" here (i.e. return "", ...).
| + | ||
| + if err = InitAdministratorUser(&args); err != nil { | ||
| + return machineId, errors.Annotatef(err, | ||
| + "Cannot provision machine beacuse no WinRM http/https standard", |
natefinch
Jan 5, 2017
Contributor
this error is not going to turn out the way you expect, and this will likely fail go vet. The first string is the format string, the rest of the strings are the args to the format string. The easiest fix is to replace the comma on this line with a +, so you're just concatenating the two lines (add a space at the end of this line, too)
| + }) | ||
| + | ||
| + if err != nil { | ||
| + logger.Errorf("Cannot obtain provisioning script") |
natefinch
Jan 5, 2017
Contributor
this error will handled at a higher level, so the log message here is probably redundant
| +) | ||
| + | ||
| +type provisionerSuite struct { | ||
| + testing.JujuConnSuite |
natefinch
Jan 5, 2017
Contributor
We're trying to avoid using JujuConnSuite for new tests. It's too slow, and requires too much hackery to make work.
I think we can test the majority of the code via unit tests, mocking out specific pieces, and then write a full CI test to do real provisioning with a real windows machine to test the end to end bits.
| + }, | ||
| + | ||
| + fakeRun: func(cmd string, stdout, stderr io.Writer) error { | ||
| + c.Assert((len(cmd) > 0), gc.Equals, true) |
| +` | ||
| + | ||
| +// detectHardware is a powershell script that determines the following: | ||
| +// - the processor architecture |
natefinch
Jan 5, 2017
Contributor
the formatting on this comment is a little weird. Can you clean it up some?
hoenirvili
Jan 9, 2017
Contributor
Why this is wierd? I think this comes to personal preference. I didn't come with a better way of explaining this powershell script in another manner. Also in my oppinion it's a good comment with main bullet points and very structured(all points explained).
If have a different opinion please elaborate more.
| +// the Administrator user using the secure client | ||
| +// only if this is false then this will make a new attempt with the unsecure http client. | ||
| +func InitAdministratorUser(args *manual.ProvisionMachineArgs) error { | ||
| + logger.Infof("Trying https client as user %s on %s", args.Host, args.User) |
natefinch
Jan 5, 2017
Contributor
I think this is more a debug level thing. If everything works, there's no need to mention anything to the user. Plus, if we keep everything in this function at debug level, then it'll provide a more consistent behavior for the person looking at the logs. Right now it's kind of random which are debug and which are info.
hoenirvili
Jan 9, 2017
Contributor
So what are you suggesting? To replace all logger.Infof with logger.Debuf ?
| +// we are doing this instead of one big script because winrm supports | ||
| +// just 8192 length commands. We know we have an amount of prefixed scripts | ||
| +// that we want to bind for the init process so create an array of scripts | ||
| +func bindInitScripts(pass string, keys *winrm.X509) ([3]string, error) { |
natefinch
Jan 5, 2017
Contributor
There's no real reason to use an array as the return value rather than a slice. If we decide we need to return 4 scripts instead of 3, the calling code shouldn't care, I presume it just ranges through the results anyway.
hoenirvili
Jan 9, 2017
Contributor
Yeah it ranges through the result. But I choosed a simple array over slice for many reasons.
Why should I assume or let the compiler decide if the space is stored on heap or stack?
It's more a memory efficient way to just make a simple array on stack and use it. It's not like the array needs to get bigger or modify it any time soon.
Why should I clutter the code with append ? Or use unnecessary call to make at the start ?(And make it's not 100% it will make the decision to make it on heap)
Using a simple array I inform the caller that he/she will deal with a constant number of scripts.
axw
Jan 23, 2017
Member
This code isn't on any hot path, so how the memory is allocated is not important. Maintainability is more important in general. Please use a slice; the caller should not care how many elements there are. As such, exposing it is creating too tight a dependency from callers to the implementation details.
(Also, FWIW: you can preallocate a slice and assign by index just as you do below. The compiler might even be smart enough to elide bounds checking, but even if it's not, it's going to have no noticeable impact on the runtime of the program.)
| + ) | ||
| + | ||
| + if len(pass) == 0 { | ||
| + return scripts, fmt.Errorf("The password is empty, provide a valid password to enable https interactions") |
| + | ||
| + scripts[0], err = shell.NewPSEncodedCommand(setFiles) | ||
| + if err != nil { | ||
| + return scripts, err |
natefinch
Jan 5, 2017
Contributor
in cases where there's an error, it's more appropriate to return nil, err, so we're not returning potentially partially formed data in the array of scripts.
In addition, you should use errors.Trace(err) so we keep stack info on the error.
| + | ||
| + provisioned, err := checkProvisioned(hostname, cli) | ||
| + if err != nil { | ||
| + err = fmt.Errorf("error checking is provisioned: %v", err) |
| + // if it isn't one that the environment provider knows about. | ||
| + | ||
| + instanceId := instance.Id(manual.ManualInstancePrefix + hostname) | ||
| + nonce := fmt.Sprintf("%s:%s", instanceId, uuid.String()) |
natefinch
Jan 5, 2017
Contributor
just FYI, you don't need the .String() on uuid. fmt.Sprintf will look for a .String() method if you're passing something into a %s format value.
| +// checkProvisioned checks if the machine is already provisioned or not. | ||
| +// if it's already provisioned it will return true | ||
| +func checkProvisioned(host string, cli manual.WinrmClientAPI) (bool, error) { | ||
| + logger.Infof("Checking if %s windows machine is already provisioned", host) |
| + | ||
| + provisioned := strings.Contains(stdout.String(), "Yes") | ||
| + if stderr.Len() != 0 { | ||
| + err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String())) |
| +// - info[2] the series of the machine | ||
| +// - info[3] the number of cores that the machine has | ||
| +// It returns nil if it parsed successfully. | ||
| +func initHC(hc *instance.HardwareCharacteristics, info []string) error { |
natefinch
Jan 5, 2017
Contributor
it's more idiomatic to simply return a new hardware characteristics value, rather than passing in a pointer to be filled out.
hoenirvili
Jan 9, 2017
•
Edited 1 time
-
hoenirvili
Jan 9, 2017
Contributor
-
Jan 9, 2017
I think it's fine this way, the name implies that given *hc it will initialize(modify) after the call.
| + return errors.Annotatef(err, "Can't parse mem number of the windows machine") | ||
| + } | ||
| + hc.Mem = new(uint64) | ||
| + *hc.Mem = mem |
axw
Jan 23, 2017
Member
No, because after we exit the scope the hc.Mem will be nil.
"mem" will be captured by the reference, and will be stored on the heap (assuming hc is also stored on the heap). It's fine (and preferable) to do hc.Mem = &mem.
| + } | ||
| + | ||
| + hc.CpuCores = new(uint64) | ||
| + *hc.CpuCores = cores |
| + | ||
| + udata, err := cloudconfig.NewUserdataConfig(icfg, cloudcfg) | ||
| + if err != nil { | ||
| + return "", errors.Annotate(err, "error generating cloud-config") |
natefinch
Jan 5, 2017
Contributor
ideally you'd make these error messages distinct so it's more obvious which one is causing the problem.
| + | ||
| + // this should return to ioctl device error | ||
| + err = winrmprovisioner.InitAdministratorUser(&args) | ||
| + c.Assert(err, gc.NotNil) |
natefinch
Jan 5, 2017
Contributor
this needs a more specific test that we're returning the right kind of error
hoenirvili
Jan 9, 2017
Contributor
So what are you proposing? should I move this into a new func TestIotlErrReturn or smth ?
| + | ||
| + hc, ser, err := winrmprovisioner.DetectSeriesAndHardwareCharacteristics(winrmListenerAddr, fakeCli) | ||
| + c.Assert(err, gc.IsNil) | ||
| + c.Assert((len(ser) > 0), gc.Equals, true) |
axw
Jan 23, 2017
Member
This has not been addressed.
You can drop this line as Nate mentions, but FYI for future: you can use c.Assert(ser, gc.Not(gc.HasLen), 0). This preserves the value of ser for the error message.
| + c.Assert((len(ser) > 0), gc.Equals, true) | ||
| + c.Assert(hc, gc.NotNil) | ||
| + c.Assert(ser, jc.DeepEquals, series) | ||
| + c.Assert(*hc.Arch, jc.DeepEquals, arch) |
| + var stdin, stderr bytes.Buffer | ||
| + fakeCli := &fakeWinRM{ | ||
| + fakeRun: func(cmd string, stdout, stderr io.Writer) error { | ||
| + c.Assert((len(cmd) > 0), gc.Equals, true) |
| + return errors.Annotatef(err, "connot load/create x509 client certs for winrm connection") | ||
| + } | ||
| + | ||
| + _ = cert.LoadCACert(filepath.Join(base, "winrmcacert.crt")) |
hoenirvili
Jan 9, 2017
Contributor
I'm skipping this error because this InitJujuXDGDataHome runs everytime I launch a juju command. So not all commands deal with provisioning or needs that CA cert to work. This is also very "windows specific" and only the juju add-machine benefits from this. So we don't need to annoy the user that he doesn't have a CACert alongside with the client cert and key.
This check is already done in the winrmprovisioner code when the provisioning process starts.
|
@natefinch Could you please take a look at the code and add more recommendations? |
added some commits
Dec 5, 2016
axw
reviewed
Jan 23, 2017
There's a bunch of comments that haven't been addressed, though you have commented saying "Fixed". Please double-check; maybe you didn't push some commits?
| + "cannot decide which provisioning script to generate based on this series %q", icfg.Series)) | ||
| + } | ||
| + | ||
| + switch osSeries { |
axw
Jan 23, 2017
Member
this could be simplified to:
getProvisioningScript := sshprovisioner.ProvisioningScript
if osSeries == os.Windows {
getProvisioningScript = winrmprovisioner.ProvisioningScript
}
result.Script, err = getProvisioningScript(icfg)
if err != nil {
return result, common.ServerError(errors.Annotate(
err, "getting provisioning script",
))
}| @@ -50,9 +51,22 @@ type ProvisionMachineArgs struct { | ||
| // ubuntu user's ~/.ssh/authorized_keys. | ||
| AuthorizedKeys string | ||
| + // WKeys winrm keys that contains, CACert, ClientCert, ClientKey | ||
| + WKeys *winrm.X509 |
axw
Jan 23, 2017
Member
Can you please expand the W here and below to WinRM. It's not very meaningful out of context
| + }() | ||
| + | ||
| + if err = InitAdministratorUser(&args); err != nil { | ||
| + return machineId, errors.Annotatef(err, |
natefinch
Jan 5, 2017
Contributor
probably clearer to return "" rather than machineId, just so it's clear you're returning an empty string here
axw
Jan 23, 2017
Member
Have you pushed your fixes? I'm pretty sure Nate meant to use a string literal of "" here (i.e. return "", ...).
| +// we are doing this instead of one big script because winrm supports | ||
| +// just 8192 length commands. We know we have an amount of prefixed scripts | ||
| +// that we want to bind for the init process so create an array of scripts | ||
| +func bindInitScripts(pass string, keys *winrm.X509) ([3]string, error) { |
natefinch
Jan 5, 2017
Contributor
There's no real reason to use an array as the return value rather than a slice. If we decide we need to return 4 scripts instead of 3, the calling code shouldn't care, I presume it just ranges through the results anyway.
hoenirvili
Jan 9, 2017
Contributor
Yeah it ranges through the result. But I choosed a simple array over slice for many reasons.
Why should I assume or let the compiler decide if the space is stored on heap or stack?
It's more a memory efficient way to just make a simple array on stack and use it. It's not like the array needs to get bigger or modify it any time soon.
Why should I clutter the code with append ? Or use unnecessary call to make at the start ?(And make it's not 100% it will make the decision to make it on heap)
Using a simple array I inform the caller that he/she will deal with a constant number of scripts.
axw
Jan 23, 2017
Member
This code isn't on any hot path, so how the memory is allocated is not important. Maintainability is more important in general. Please use a slice; the caller should not care how many elements there are. As such, exposing it is creating too tight a dependency from callers to the implementation details.
(Also, FWIW: you can preallocate a slice and assign by index just as you do below. The compiler might even be smart enough to elide bounds checking, but even if it's not, it's going to have no noticeable impact on the runtime of the program.)
| + return errors.Annotatef(err, "Can't parse mem number of the windows machine") | ||
| + } | ||
| + hc.Mem = new(uint64) | ||
| + *hc.Mem = mem |
axw
Jan 23, 2017
Member
No, because after we exit the scope the hc.Mem will be nil.
"mem" will be captured by the reference, and will be stored on the heap (assuming hc is also stored on the heap). It's fine (and preferable) to do hc.Mem = &mem.
| + input := bytes.NewBufferString(script64) // make new buffer out of script | ||
| + // will make sure to buffer the entire script | ||
| + // in a sequential write fashion first into a script | ||
| + // decouple the provisioning script into little 512 bytes chunks |
hoenirvili
Jan 23, 2017
•
Edited 1 time
-
hoenirvili
Jan 23, 2017
Contributor
-
Jan 23, 2017
s/512/1024/?
Updated.
assuming hc is also stored on the heap
It's on stack. (check line 385).
| @@ -23,5 +26,20 @@ func InitJujuXDGDataHome() error { | ||
| if err := ssh.LoadClientKeys(osenv.JujuXDGDataHomePath("ssh")); err != nil { | ||
| return errors.Annotate(err, "cannot load ssh client keys") | ||
| } | ||
| + | ||
| + base := osenv.JujuXDGDataHomePath("x509") |
axw
Jan 23, 2017
Member
are these bits needed any more? certs are loaded in the add-machine command (which is preferable, since it doesn't affect every command) -- does it need to be here too?
|
!!build!! |
axw
requested changes
Jan 23, 2017
Once the issues I've highlighted have been addressed, I will take one final pass. Nothing major jumps out at me.
| + }() | ||
| + | ||
| + if err = InitAdministratorUser(&args); err != nil { | ||
| + return machineId, errors.Annotatef(err, |
natefinch
Jan 5, 2017
Contributor
probably clearer to return "" rather than machineId, just so it's clear you're returning an empty string here
axw
Jan 23, 2017
Member
Have you pushed your fixes? I'm pretty sure Nate meant to use a string literal of "" here (i.e. return "", ...).
| + }) | ||
| + | ||
| + if err != nil { | ||
| + logger.Errorf("Cannot obtain provisioning script") |
natefinch
Jan 5, 2017
Contributor
this error will handled at a higher level, so the log message here is probably redundant
| +) | ||
| + | ||
| +type provisionerSuite struct { | ||
| + testing.JujuConnSuite |
natefinch
Jan 5, 2017
Contributor
We're trying to avoid using JujuConnSuite for new tests. It's too slow, and requires too much hackery to make work.
I think we can test the majority of the code via unit tests, mocking out specific pieces, and then write a full CI test to do real provisioning with a real windows machine to test the end to end bits.
| + | ||
| + scripts[0], err = shell.NewPSEncodedCommand(setFiles) | ||
| + if err != nil { | ||
| + return scripts, err |
natefinch
Jan 5, 2017
Contributor
in cases where there's an error, it's more appropriate to return nil, err, so we're not returning potentially partially formed data in the array of scripts.
In addition, you should use errors.Trace(err) so we keep stack info on the error.
| + | ||
| + provisioned, err := checkProvisioned(hostname, cli) | ||
| + if err != nil { | ||
| + err = fmt.Errorf("error checking is provisioned: %v", err) |
| + // if it isn't one that the environment provider knows about. | ||
| + | ||
| + instanceId := instance.Id(manual.ManualInstancePrefix + hostname) | ||
| + nonce := fmt.Sprintf("%s:%s", instanceId, uuid.String()) |
natefinch
Jan 5, 2017
Contributor
just FYI, you don't need the .String() on uuid. fmt.Sprintf will look for a .String() method if you're passing something into a %s format value.
| + | ||
| + provisioned := strings.Contains(stdout.String(), "Yes") | ||
| + if stderr.Len() != 0 { | ||
| + err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String())) |
| + | ||
| + udata, err := cloudconfig.NewUserdataConfig(icfg, cloudcfg) | ||
| + if err != nil { | ||
| + return "", errors.Annotate(err, "error generating cloud-config") |
natefinch
Jan 5, 2017
Contributor
ideally you'd make these error messages distinct so it's more obvious which one is causing the problem.
| + | ||
| + hc, ser, err := winrmprovisioner.DetectSeriesAndHardwareCharacteristics(winrmListenerAddr, fakeCli) | ||
| + c.Assert(err, gc.IsNil) | ||
| + c.Assert((len(ser) > 0), gc.Equals, true) |
axw
Jan 23, 2017
Member
This has not been addressed.
You can drop this line as Nate mentions, but FYI for future: you can use c.Assert(ser, gc.Not(gc.HasLen), 0). This preserves the value of ser for the error message.
| + c.Assert((len(ser) > 0), gc.Equals, true) | ||
| + c.Assert(hc, gc.NotNil) | ||
| + c.Assert(ser, jc.DeepEquals, series) | ||
| + c.Assert(*hc.Arch, jc.DeepEquals, arch) |
| + var stdin, stderr bytes.Buffer | ||
| + fakeCli := &fakeWinRM{ | ||
| + fakeRun: func(cmd string, stdout, stderr io.Writer) error { | ||
| + c.Assert((len(cmd) > 0), gc.Equals, true) |
|
!!build!! |
axw
approved these changes
Jan 24, 2017
A few little things, otherwise LGTM. Please run tests with -race before merging.
| + t.Skip("Manual provider as client is not supported on windows") | ||
| + } | ||
| + | ||
| + testing.MgoTestPackage(t) |
axw
Jan 24, 2017
Member
Now that you're not using JujuConnSuite, MgoTestPackage isn't needed. Please use gocheck.TestingT(t) instead
| + | ||
| + provisioned := strings.Contains(stdout.String(), "Yes") | ||
| + if stderr.Len() != 0 { | ||
| + err = errors.Annotatef(err, "%v (%v)", err, strings.TrimSpace(stderr.String())) |
axw
Jan 24, 2017
Member
Annotatef already includes the original error in the message. Please change this to:
err = errors.Annotate(err, strings.TrimSpace(stderr.String()))
| + | ||
| + udata, err := cloudconfig.NewUserdataConfig(icfg, cloudcfg) | ||
| + if err != nil { | ||
| + return "", errors.Annotate(err, "error generating cloud-config") |
natefinch
Jan 5, 2017
Contributor
ideally you'd make these error messages distinct so it's more obvious which one is causing the problem.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jujubot
merged commit 3dcb717
into
juju:develop
Jan 24, 2017
1 check failed
hoenirvili
deleted the
hoenirvili:enable-windows-provisioner
branch
Jan 24, 2017
|
This is a big deal. Thank you for your perseverance and congrats on getting it landed @hoenirvili :) |
|
@axw Thanks man, you welcome. I actually enjoyed writing and contribute my code to this wonderful community. |
hoenirvili commentedNov 1, 2016
This patch contains:
machines
cert dosen't exist.
files, etc.
Ignore unit tests for now, I will write them after we all agree on the api.