Skip to content
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

Ensure Remote Console tickets aren't VimStrings #779

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

agrare
Copy link
Member

@agrare agrare commented Jan 14, 2022

When openings a VMRC Console a MiqTask is created and vm.remote_console_vmrc_acquire_ticket is called on the queue.
The ticket string returned by vim.acquireCloneTicket is a VimString type. This then is saved in the task_results which throws an exception when loaded due to VMwareWebService/VimTypes not being loaded in the webserver.

=> #<MiqTask id: 78, name: "acquiring Vm ag_/test_\\ren%amed VMRC remote consol...", state: "Finished", status: "Ok", message: "Task completed successfully", userid: "admin", created_on: "2022-01-14 15:25:10", updated_on: "2022-01-14 15:25:10", pct_complete: nil, context_data: nil, results: nil, miq_server_id: 1, identifier: nil, started_on: "2022-01-14 15:25:10", zone: nil>
>> vmrc_task.task_results
   (0.5ms)  SELECT "binary_blob_parts"."data" FROM "binary_blob_parts" WHERE "binary_blob_parts"."binary_blob_id" = $1 ORDER BY "binary_blob_parts"."id" ASC  [["binary_blob_id", 55]]
=> {:ticket=>"cst-VCT-5202d576-98d3-d347-39b5-eb798442a3dd--tp-85-88-00-9D-4C-D2-52-9B-10-B1-33-00-40-56-DA-6D-06-F5-C9-87", :remote_url=>"vmrc://clone:cst-VCT-5202d576-98d3-d347-39b5-eb798442a3dd--tp-85-88-00-9D-4C-D2-52-9B-10-B1-33-00-40-56-DA-6D-06-F5-C9-87@icovcpc65.rtp.raleigh.ibm.com:443/?moid=vm-53550", :proto=>"remote"}
>> vmrc_task.task_results[:ticket].class
   (0.4ms)  SELECT "binary_blob_parts"."data" FROM "binary_blob_parts" WHERE "binary_blob_parts"."binary_blob_id" = $1 ORDER BY "binary_blob_parts"."id" ASC  [["binary_blob_id", 55]]
=> VimString

The raw data looks like:

>> BinaryBlob.find(55).binary_blob_parts.first.data
  BinaryBlob Load (0.6ms)  SELECT "binary_blobs".* FROM "binary_blobs" WHERE "binary_blobs"."id" = $1 LIMIT $2  [["id", 55], ["LIMIT", 1]]
  BinaryBlob Inst Including Associations (0.1ms - 1rows)
  BinaryBlobPart Load (0.4ms)  SELECT "binary_blob_parts".* FROM "binary_blob_parts" WHERE "binary_blob_parts"."binary_blob_id" = $1 ORDER BY "binary_blob_parts"."id" ASC LIMIT $2  [["binary_blob_id", 55], ["LIMIT", 1]]
  BinaryBlobPart Inst Including Associations (0.1ms - 1rows)
=> "---\n:ticket: !ruby/string:VimString\n  str: cst-TICKET\n  xsiType: :SOAP::SOAPString\n  vimType:\n:remote_url: vmrc://clone:cst-TICKET@vcenter.local:443/?moid=vm-53550\n:proto: remote\n"

Data migration: ManageIQ/manageiq-schema#631
Fixes #778

@agrare agrare requested a review from Fryguy as a code owner January 14, 2022 15:53
@agrare agrare added the bug label Jan 14, 2022
@agrare
Copy link
Member Author

agrare commented Jan 14, 2022

Working on a schema migration for this but binary_blob_parts#data is a binary not a string so my usual REGEXP_REPLACE is failing

@Fryguy
Copy link
Member

Fryguy commented Jan 14, 2022

Can you add a spec for this or is it too complicated?

@Fryguy
Copy link
Member

Fryguy commented Jan 14, 2022

Strange that travis didn't run on this PR.

@agrare
Copy link
Member Author

agrare commented Jan 14, 2022

Can you add a spec for this or is it too complicated?

👍 yeah should be able to

@agrare
Copy link
Member Author

agrare commented Jan 14, 2022

Oh yeah you're right

@agrare agrare force-pushed the remote_console_ticket_vim_string branch from efcdf57 to b3a46d4 Compare January 14, 2022 20:39
expect(ticket).to have_key(:ticket)
expect(ticket[:ticket]).to match(/^[0-9\-A-Z]{40}$/)
expect(ticket[:ticket].class.name).to eq("String") # Ensure that VimStrings aren't returned
Copy link
Member

@Fryguy Fryguy Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly clearer

Suggested change
expect(ticket[:ticket].class.name).to eq("String") # Ensure that VimStrings aren't returned
expect(ticket[:ticket]).to be_instance_of(String) # Ensure that VimStrings aren't returned

Can't use be_a or be_kind_of because that would account for subclassing, but I believe be_instance_of ignores subclassing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah didn't do that because I thought VimString (subclass of String) might return true but looks like that isn't the case

@agrare agrare force-pushed the remote_console_ticket_vim_string branch from b3a46d4 to fc37e55 Compare January 14, 2022 20:44
@Fryguy Fryguy merged commit c031f88 into ManageIQ:master Jan 14, 2022
@Fryguy Fryguy self-assigned this Jan 14, 2022
@agrare agrare deleted the remote_console_ticket_vim_string branch January 14, 2022 21:17
@Fryguy
Copy link
Member

Fryguy commented Jan 24, 2022

Backported to morphy in commit af3e8ad.

commit af3e8adb2e35f1533edac84b32bffc7e3b0b33e0
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Jan 14 16:15:25 2022 -0500

    Merge pull request #779 from agrare/remote_console_ticket_vim_string
    
    Ensure Remote Console tickets aren't VimStrings
    
    (cherry picked from commit c031f88b918eb1b8b35ebb3d8df1e233471c6140)

Fryguy added a commit that referenced this pull request Jan 24, 2022
Ensure Remote Console tickets aren't VimStrings

(cherry picked from commit c031f88)
@Fryguy
Copy link
Member

Fryguy commented Feb 2, 2022

Backported to lasker in commit 6f0aaa5.

commit 6f0aaa59492646dbc1b04e95e17dfa4ed87ec416
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Jan 14 16:15:25 2022 -0500

    Merge pull request #779 from agrare/remote_console_ticket_vim_string
    
    Ensure Remote Console tickets aren't VimStrings
    
    (cherry picked from commit c031f88b918eb1b8b35ebb3d8df1e233471c6140)

Fryguy added a commit that referenced this pull request Feb 2, 2022
Ensure Remote Console tickets aren't VimStrings

(cherry picked from commit c031f88)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lasker-1 vmrc not working
3 participants