Disable use of service ticket for datastore HTTP access by default #635
Conversation
Can still be enabled by setting GOVMOMI_USE_SERVICE_TICKET=true as a GOVC_URL query param or environment variable. - Only use service tickets when connected to VC - Add Datastore.HostContext helper for cases where datastore files can only be read by the host where the VM is running - Prefer host management IP to host inventory name - Add -host option to govc datastore.download and datastore.tail to optionally specify the host owning the lock for a VM file open for writing (e.g. vmware.log) - Collapse some common code code for constructing the URL - Use the full inventory path for the dcPath parameter (Fixes vmware#594)
| @@ -73,6 +81,18 @@ func (cmd *download) Run(ctx context.Context, f *flag.FlagSet) error { | |||
| return err | |||
| } | |||
|
|
|||
| var via string | |||
There was a problem hiding this comment.
Small readability note: can we move via closer to where it's used?
| @@ -39,12 +41,18 @@ func init() { | |||
| func (cmd *download) Register(ctx context.Context, f *flag.FlagSet) { | |||
| cmd.DatastoreFlag, ctx = flags.NewDatastoreFlag(ctx) | |||
| cmd.DatastoreFlag.Register(ctx, f) | |||
|
|
|||
| cmd.HostSystemFlag, ctx = flags.NewHostSystemFlag(ctx) | |||
There was a problem hiding this comment.
I wonder why do we pass context and get it back?
There was a problem hiding this comment.
I didn't implement that pattern, but I believe its because some flags attach context.Value(s)
| if cmd.OutputFlag.TTY { | ||
| logger := cmd.ProgressLogger("Downloading... ") | ||
| if cmd.DatastoreFlag.OutputFlag.TTY { | ||
| logger := cmd.DatastoreFlag.ProgressLogger(fmt.Sprintf("Downloading%s... ", via)) |
There was a problem hiding this comment.
Shouldn't it be something like ": " before %s?
There was a problem hiding this comment.
Default message is just: "Downloading..."
If host is specified, looks like: "Downloading via esx1-host.example.com..."
| } | ||
|
|
||
| // NewURL constructs a url.URL with the given file path for datastore access over HTTP. | ||
| func (d Datastore) NewURL(path string) *url.URL { |
There was a problem hiding this comment.
In general my preference/habit is to use pointer receivers, but the existing wrappers use value.
| } | ||
| var host *HostSystem | ||
|
|
||
| h := ctx.Value(datastoreServiceTicketHostKey) |
There was a problem hiding this comment.
you could just cast h in place without checking for nil: https://play.golang.org/p/T6cKj0hHM3
There was a problem hiding this comment.
yes that'll clean this up a bit, I'll do that.
|
I have no concerns about the correctness of this code. Things looks good to me. There are some cosmetic improvements could be made, but that is it. |
|
lgtm |
1 similar comment
|
lgtm |
| @@ -312,6 +318,7 @@ func (f *Finder) DatastoreList(ctx context.Context, path string) ([]*object.Data | |||
| if ref.Type == "Datastore" { | |||
| ds := object.NewDatastore(f.client, ref) | |||
| ds.InventoryPath = e.Path | |||
| ds.DatacenterPath = f.dc.InventoryPath | |||
There was a problem hiding this comment.
is dc guaranteed to be non-nil at this point?
There was a problem hiding this comment.
The f.datastoreFolder func above takes care of checking f.dc != nil. You'd have to be using Finder without having called SetDatacenter and all inputs as absolute paths without wildcards, in which case there's no point to using Finder :)
There was a problem hiding this comment.
@dougm In one of my projects I'm using absolute path to the datastore, e.g. /MyDatacenter/datastore/vm-storage01 and use a Finder.Datastore() to get the datastore object, but now I get panics because of this.
What would be the recommended way without having to call SetDatacenter(), because that would be another parameter I need to add to my project and that's already lots of changes I need to do to adjust the code to the recent change?
Thanks,
Marin
There was a problem hiding this comment.
@dnaeon in that case you don't need Finder, as SearchIndex.FindByInventoryPath does the job with absolute paths. But I'd be fine with adding the nil check for now. Are you using any of the Datastore upload/download methods? Those need the Datacenter ref against VC, unless you set GOVMOMI_USE_SERVICE_TICKET=true
| @@ -58,6 +58,8 @@ func (e DatastoreNoSuchFileError) Error() string { | |||
|
|
|||
| type Datastore struct { | |||
| Common | |||
|
|
|||
| DatacenterPath string | |||
There was a problem hiding this comment.
Given everything lives in the context of a datacenter, should this move into Common eventually?
There was a problem hiding this comment.
Started on that route initially, but opted to keep the diff a bit smaller and ideally there won't be other types that need this path. Datastore is the only type currently that uses HTTP without the vmodl.
| hosts, err := d.AttachedHosts(ctx) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| if len(hosts) == 0 { | ||
| return nil, nil, fmt.Errorf("no hosts attached to datastore %#v", d.Reference()) | ||
| return u, nil, nil |
|
LGTM |
PR vmware#635 assumed that Finder would not be used without having called SetDatacenter. However, when the Datastore methods are used with absolute paths and without SetDatacenter, would cause a panic.
PR #635 assumed that Finder would not be used without having called SetDatacenter. However, when the Datastore methods are used with absolute paths and without SetDatacenter, would cause a panic.
Can still be enabled by setting GOVMOMI_USE_SERVICE_TICKET=true
as a GOVC_URL query param or environment variable.
Only use service tickets when connected to VC
Add Datastore.HostContext helper for cases where datastore files
can only be read by the host where the VM is running
Prefer host management IP to host inventory name
Add -host option to govc datastore.download and datastore.tail to optionally
specify the host owning the lock for a VM file open for writing (e.g. vmware.log)
Collapse some common code code for constructing the URL
Use the full inventory path for the dcPath parameter (Fixes Datastore.URL(...) returns incorrect url when Datacenter is in a folder #594)