Add a new "Endpoint URL" field in AWS endpoints

[AparnaKarve]

Added a new Endpoint URL optional field in AWS endpoints as shown in the screenshot.

Depends on ManageIQ/manageiq-providers-amazon#444

Fixes #3907

[djberg96]

👍

[AparnaKarve]

@djberg96 There is an unrelated JS spec failure on travis currently - should be resolved soon hopefully.
( also seeing the same failure on a few other open PRs)

[AlexanderZagaynov]

incoming params should be escaped for safety
http://devdocs.io/ruby~2.3/uri#method-c-parse:
It's recommended to first ::escape the provided uri_str if there are any invalid URI characters.
http://devdocs.io/ruby~2.3/uri/escape#method-i-escape

[AlexanderZagaynov]

Related comment here: ManageIQ/manageiq-providers-amazon#444 (comment) (need to decide where escaping should be made)

[AlexanderZagaynov]

see above about escaping

[AparnaKarve]

@AlexanderZagaynov URI.escape seems to be obsolete (per above rubocop warnings)

@djberg96 and I were discussing if we need to escape at all...

Keeping URI.escape for now, but would need to address this by removing the escape or replacing it with CGI.escape

[AlexanderZagaynov]

I believe WEBrick::HTTPUtils.escape can be the best option:

URI.escape('http://example.com/abc|def?xyz 123?#$')
=> "http://example.com/abc%7Cdef?xyz%20123?%23$"

CGI.escape('http://example.com/abc|def?xyz 123?#$')
=> "http%3A%2F%2Fexample.com%2Fabc%7Cdef%3Fxyz+123%3F%23%24"

URI.encode_www_form(['http://example.com/abc|def?xyz 123?#$'])
=> "http%3A%2F%2Fexample.com%2Fabc%7Cdef%3Fxyz+123%3F%23%24"

URI.encode_www_form_component('http://example.com/abc|def?xyz 123?#$')
=> "http%3A%2F%2Fexample.com%2Fabc%7Cdef%3Fxyz+123%3F%23%24"

WEBrick::HTTPUtils.escape('http://example.com/abc|def?xyz 123?#$')
=> "http://example.com/abc%7Cdef?xyz%20123?%23$"

[AlexanderZagaynov]

Also:

URI.parse('http://example.com/abc|def?xyz 123?#$')
URI::InvalidURIError: bad URI(is not URI?): http://example.com/abc|def?xyz 123?#$

URI.parse(WEBrick::HTTPUtils.escape 'http://example.com/abc|def?xyz 123?#$').tap do |uri|
  ap uri.component.map { |component| [component, uri.public_send(component)] }.to_h
end

=>

{
  :scheme   => "http",
  :userinfo => nil,
  :host     => "example.com",
  :port     => 80,
  :path     => "/abc%7Cdef",
  :query    => "xyz%20123?%23$",
  :fragment => nil
}

[djberg96]

@alexander-demichev @AparnaKarve Let's use Addressable::URI.escape if we're going to escape the field.

On second thought, let's just stick with the standard URI library since Addressable isn't in our Gemfile. I realize there are some edge cases that might break it, but I doubt you'll ever see them in practice. If someone does break it (other than QE), then we'll deal with it later.

[AparnaKarve]

@djberg96 Should I revert 0e81618?

[djberg96]

@AparnaKarve nope, actually, that's fine.

[djberg96]

@AparnaKarve I vote to ignore the rubocop warning for now. Any of the options that @AlexanderZagaynov mentioned will require an explicit require statement anyway, so let's see if I can get the addressable gem accepted into the core repo.

[AparnaKarve]

@djberg96 I have no issues ignoring rubocop for now.

Just one question to you and @AlexanderZagaynov - what do you think about CGI.escape -- what rubocop suggested?

[AlexanderZagaynov]

@AparnaKarve check examples above, you'll see the result of each option.
CGI.escape will not do what you need.

[AlexanderZagaynov]

WEBrick::HTTPUtils only demands for require 'webrick/httputils',
which is part of the Ruby Std-lib (no additional gems needed): https://ruby-doc.org/stdlib-2.3.6/libdoc/webrick/rdoc/WEBrick/HTTPUtils.html
However I'm not sure will it cause any conflict with production web server (puma), but it should not, especially with requiring as I wrote here, that file contains just a module, and don't patch anything.

[AparnaKarve]

@AlexanderZagaynov Thanks.
Sorry I completely missed looking at CGI.escape output.

[miq-bot]

Checked commits AparnaKarve/manageiq-ui-classic@862788a~...9852eb2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

**

• 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

[AparnaKarve]

Changed the escape implementation per @AlexanderZagaynov 's suggestion.
Please review 9852eb2

[AlexanderZagaynov]

LGTM 👍

[djberg96]

👍