Escape characters in SSH URI for virt-v2v #18451

ghost · 2019-02-12T20:56:02Z

Migrating to OSP over SSH transformation method with names containing international chars, such as �Ää, fails with :

[   3.5] Opening the source -i vmx ssh://root@10.8.58.29/vmfs/volumes/env-esxi67-ims-h01_localdisk/�Ää/�Ää.vmx
virt-v2v: error: remote vmx 
‘ssh://root@10.8.58.29/vmfs/volumes/env-esxi67-ims-h01_localdisk/�Ää/�Ää.vmx’ 
could not be parsed as a URI

This PR escapes the characters for the URI bits that are not yet escaped.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1669240

ghost · 2019-02-12T20:56:33Z

@miq-bot add-label transformation, bug, hammer/yes, wip

ghost · 2019-02-13T12:44:06Z

@miq-bot remove-label wip
@miq-bot add-reviewer @djberg96

ghost · 2019-02-13T12:51:59Z

@miq-bot add-reviewer @jerryk55
@miq-bot add-reviewer @jameswnl

jerryk55

Appropriate fix. Looks good!

djberg96 · 2019-02-13T13:56:50Z

Looks ok to me though a spec would be good.

Edit: oops, I should have said this as part of the "review". See below.

djberg96

This method needs specs. Doesn't have to be anything fancy, but something would be good.

ghost · 2019-02-13T14:00:15Z

@djberg96 how a spec would look like for this kind of escaping ? Do you have an example ?
It's a private method, so I test the public method that calls it.

jameswnl · 2019-02-13T14:12:43Z

@fdupont-redhat how about a spec testing the output of conversion_options_source_provider_vmwarews_ssh?

agrare · 2019-02-13T14:22:11Z

app/models/service_template_transformation_plan_task.rb

@@ -283,7 +283,7 @@ def conversion_options_source_provider_vmwarews_vddk(_storage)

  def conversion_options_source_provider_vmwarews_ssh(storage)
    {
-      :vm_name          => URI::Generic.build(:scheme => 'ssh', :userinfo => 'root', :host => source.host.ipaddress, :path => "/vmfs/volumes").to_s + "/#{storage.name}/#{source.location}",
+      :vm_name          => URI::Generic.build(:scheme => 'ssh', :userinfo => 'root', :host => source.host.ipaddress, :path => "/vmfs/volumes").to_s + "/#{CGI.escape(storage.name)}/#{CGI.escape(source.location)}",


.to_s + "/#{CGI.escape(storage.name)}/#{CGI.escape(source.location)}"

This should be part of the path passed to URI::Generic.build not tacked on the end, also recommend building a local variable path to keep the line length down.

djberg96 · 2019-02-13T15:01:31Z

@fdupont-redhat Looks like it's a private method, so we don't test it directly. Instead I think you could mostly copy https://github.com/ManageIQ/manageiq/blob/master/spec/models/service_template_transformation_plan_task_spec.rb#L488-L501 and just create a storage factory that contains non-ascii characters.

miq-bot · 2019-02-15T18:45:52Z

Checked commits fabiendupont/manageiq@cb677f9~...62b00ef with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

djberg96 · 2019-02-15T20:47:30Z

Addressable::URI, eh? Huh.

ghost · 2019-02-15T22:18:56Z

Addressable::URI, eh? Huh.

Didn't have much choice as CGI.escape converts space to +, instead of %20. Found it on stackoverflow.

djberg96 · 2019-02-16T12:05:46Z

@fdupont-redhat #17502

…wrapper Escape characters in SSH URI for virt-v2v (cherry picked from commit b652050) https://bugzilla.redhat.com/show_bug.cgi?id=1678385

simaishi · 2019-02-18T16:47:39Z

Hammer backport details:

$ git log -1
commit 9a6cc992fe53263d5faed3f4ccb1e157d423d2c1
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Feb 15 14:14:07 2019 -0500

    Merge pull request #18451 from fdupont-redhat/v2v_escape_unicode_for_wrapper
    
    Escape characters in SSH URI for virt-v2v
    
    (cherry picked from commit b652050c9e9c37e50672978aa0a2f939cc7510a8)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1678385

simaishi · 2019-02-18T17:53:56Z

@fdupont-redhat please take a look at Travis failure in Hammer branch:
https://travis-ci.org/ManageIQ/manageiq/builds/494991766

ghost · 2019-02-18T18:06:29Z

@simaishi I don't see how my code can generate that kind of error. The error message talk about LDAP authentication failure, tag assignment error, job aborting... Looks like the test environment is wrong. Are there other PRs failing with the same kind of errors ?

agrare · 2019-02-18T18:08:51Z

@fdupont-redhat there were two failures and they both look related to service_template_transformation_plan_task: https://travis-ci.org/ManageIQ/manageiq/builds/494991766#L9645-L9697

The specs passed before this one PR was backported.

djberg96 · 2019-02-18T20:34:51Z

Seeing two failures locally on the Hammer branch where it's converting [] characters. But why this is happening on Hammer and not master I haven't figured out yet, or even where those brackets are coming from.

>irb
irb(main):001:0> URI.escape("[storage]")
=> "[storage]"
irb(main):002:0> require 'addressable'
=> true
irb(main):003:0> Addressable::URI.escape("[storage]")
=> "%5Bstorage%5D"

djberg96 · 2019-02-19T13:43:15Z

Something changed in the underlying vm factory. On master, I show src_vm_1.location as "unknown", whereas on hammer I see "[storage] vm_0000000000072/vm_0000000000072.vmx". I's those brackets around the word 'storage' that are causing the issue. There's also the space between '[storage]' and 'vm_'.

Update: looks like we switched from vm_vmware to vm_openstack on line 304 of the spec file.

https://github.com/ManageIQ/manageiq/blob/hammer/spec/models/service_template_transformation_plan_task_spec.rb#L304

https://github.com/ManageIQ/manageiq/blob/master/spec/models/service_template_transformation_plan_task_spec.rb#L304

miq-bot changed the title ~~Escape characters to avoid unicode issues with virt-v2v~~ [WIP] Escape characters to avoid unicode issues with virt-v2v Feb 12, 2019

miq-bot added bug hammer/yes v2v wip labels Feb 12, 2019

ghost changed the title ~~[WIP] Escape characters to avoid unicode issues with virt-v2v~~ [WIP] Escape characters in SSH URI for virt-v2v Feb 13, 2019

miq-bot changed the title ~~[WIP] Escape characters in SSH URI for virt-v2v~~ Escape characters in SSH URI for virt-v2v Feb 13, 2019

miq-bot removed the wip label Feb 13, 2019

miq-bot requested a review from djberg96 February 13, 2019 12:44

miq-bot requested review from jerryk55 and jameswnl February 13, 2019 12:52

jerryk55 approved these changes Feb 13, 2019

View reviewed changes

djberg96 suggested changes Feb 13, 2019

View reviewed changes

agrare reviewed Feb 13, 2019

View reviewed changes

agrare self-assigned this Feb 13, 2019

fabiendupont added 3 commits February 15, 2019 17:41

Escape characters to avoid unicode issues with virt-v2v

cb677f9

Escape VM and storage separately

3b56faa

Move escape inside 'path' and add specs

9a39485

ghost force-pushed the v2v_escape_unicode_for_wrapper branch from cc2cb67 to 9a39485 Compare February 15, 2019 16:45

Fix rubocop errors

62b00ef

agrare approved these changes Feb 15, 2019

View reviewed changes

agrare merged commit b652050 into ManageIQ:master Feb 15, 2019

agrare added this to the Sprint 105 Ending Feb 18, 2019 milestone Feb 15, 2019

ghost deleted the v2v_escape_unicode_for_wrapper branch February 15, 2019 20:00

simaishi added hammer/backported and removed hammer/yes labels Feb 18, 2019

djberg96 mentioned this pull request Feb 19, 2019

[HAMMER] Change factories from vm_vmware to vm_openstack. #18466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape characters in SSH URI for virt-v2v #18451

Escape characters in SSH URI for virt-v2v #18451

ghost commented Feb 12, 2019

ghost commented Feb 12, 2019

ghost commented Feb 13, 2019

ghost commented Feb 13, 2019

jerryk55 left a comment

djberg96 commented Feb 13, 2019 •

edited

Loading

djberg96 left a comment

ghost commented Feb 13, 2019 •

edited by ghost

Loading

jameswnl commented Feb 13, 2019

agrare Feb 13, 2019 •

edited

Loading

djberg96 commented Feb 13, 2019

miq-bot commented Feb 15, 2019

djberg96 commented Feb 15, 2019

ghost commented Feb 15, 2019

djberg96 commented Feb 16, 2019

simaishi commented Feb 18, 2019

simaishi commented Feb 18, 2019

ghost commented Feb 18, 2019

agrare commented Feb 18, 2019

djberg96 commented Feb 18, 2019 •

edited

Loading

djberg96 commented Feb 19, 2019 •

edited

Loading

Escape characters in SSH URI for virt-v2v #18451

Escape characters in SSH URI for virt-v2v #18451

Conversation

ghost commented Feb 12, 2019

ghost commented Feb 12, 2019

ghost commented Feb 13, 2019

ghost commented Feb 13, 2019

jerryk55 left a comment

Choose a reason for hiding this comment

djberg96 commented Feb 13, 2019 • edited Loading

djberg96 left a comment

Choose a reason for hiding this comment

ghost commented Feb 13, 2019 • edited by ghost Loading

jameswnl commented Feb 13, 2019

agrare Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

djberg96 commented Feb 13, 2019

miq-bot commented Feb 15, 2019

djberg96 commented Feb 15, 2019

ghost commented Feb 15, 2019

djberg96 commented Feb 16, 2019

simaishi commented Feb 18, 2019

simaishi commented Feb 18, 2019

ghost commented Feb 18, 2019

agrare commented Feb 18, 2019

djberg96 commented Feb 18, 2019 • edited Loading

djberg96 commented Feb 19, 2019 • edited Loading

djberg96 commented Feb 13, 2019 •

edited

Loading

ghost commented Feb 13, 2019 •

edited by ghost

Loading

agrare Feb 13, 2019 •

edited

Loading

djberg96 commented Feb 18, 2019 •

edited

Loading

djberg96 commented Feb 19, 2019 •

edited

Loading