Re: [pgAdmin4][Patch] Feature #1447 SSH Tunnel

On Thu, May 3, 2018 at 10:06 AM, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com
> wrote:

> Hi
>
> On Thu, May 3, 2018 at 2:20 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>>
>>
>> On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi
>>>
>>> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry <vhenry(at)pivotal(dot)io>
>>> wrote:
>>>
>>>> Hi Akshay,
>>>>
>>>> Thanks for sending this updated patch. The linter and tests are all
>>>> passing.
>>>>
>>>>> - utils/driver/psycopg2/server_manager.py
>>>>>> - Do we have Unit Tests around this?
>>>>>
>>>>> No.
>>>>
>>>>
>>>> In our opinion, server_manager.py and connection.py should have tests.
>>>> Are you finding it difficult to add tests to these files?
>>>>
>>>
>>> We will have to write test cases from scratch for both the files and
>>> it will take time, there is no point keeping these important feature(SSH
>>> Tunnel) on hold. We can create a separate task for this as we have for
>>> utility(Backup, Maintenance, Restore) modules.
>>>
>>> @Dave your thoughts on this?
>>>
>>
>> I agree. Please add a ticket to add these tests in the future. General
>> tests for those files should not hold up this feature.
>>
>
> Done. Can you please review and commit this feature.
>

Thanks - patch applied with minor changes to the help text, and to change
the Password/Passphrase label to Password.

>
>>
>>>
>>>> Sincerely,
>>>>
>>>> Victoria & Joao
>>>>
>>>>
>>>> On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Joao
>>>>>
>>>>>
>>>>> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>
>>>>>> Hi Akshay,
>>>>>>
>>>>>> Some suggestions:
>>>>>> - browser/server_groups/servers/__init__py
>>>>>> This file could have been split into separate functionalities.
>>>>>> There is a chunk of changes for connect, why not move that out? Same thing
>>>>>> for create. Do we really need to have full integrationy tests that do a
>>>>>> HTTP request and connect to a real database to do make sure the
>>>>>> functionalities are working? If we isolate these into their own actions we
>>>>>> can more easily get more coverage on the code with tests that would be much
>>>>>> faster and directed. The big advantage of this is that by reading the tests
>>>>>> we can understand what the functions do. Self documenting code.
>>>>>>
>>>>>
>>>>> If I understood you correctly, you want separate connect and
>>>>> create function for SSH Tunnel. If yes I don't think so we should move the
>>>>> SSH code and make the rest of the code duplicated in two different
>>>>> functions. For "create" function I have just added logic to pass SSH tunnel
>>>>> parameter while creating server object, encrypt the tunnel password and
>>>>> send it to connect method. For "connect" function encrypt the password and
>>>>> send it connect method as parameter. There is no point for such small
>>>>> changes we should create two separate functions.
>>>>>
>>>>>>
>>>>>> - utils/driver/psycopg2.py same comments has above
>>>>>>
>>>>>
>>>>> I think you are talking about "utils/driver/psycopg2/connection.py".
>>>>> Same comments as above.
>>>>>
>>>>>>
>>>>>> - browser/server_groups/servers/static/js/server.js:
>>>>>> - The patterm of using `m` for `model`? is a bad pattern, so why
>>>>>> not change it?
>>>>>>
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> - We could extract the model creation from this file. This will
>>>>>> allow us to add some tests around disabled methods that are a bit everywhere
>>>>>>
>>>>> Model creation is the main functionality of the "server.js"
>>>>> (or any other module file), code readability wise it should be in the same
>>>>> file. If we will do it for rest of the modules then there are so many java
>>>>> script files where model creation is in separate file.
>>>>>
>>>>> - We could also convert this file to ES6
>>>>>>
>>>>>
>>>>> I am new to this, so will need to learn first. We can create a
>>>>> separate task to do this.
>>>>>
>>>>>>
>>>>>> - utils/driver/psycopg2/server_manager.py
>>>>>> - Do we have Unit Tests around this?
>>>>>>
>>>>>
>>>>> No.
>>>>>
>>>>>> - Maybe this SSH part could be isolated into it's own class, as it
>>>>>> is not 100% related to the class in question. We need to use it but is is
>>>>>> not part of the ServerManager domain
>>>>>>
>>>>>
>>>>> According to me SSH Tunnel parameters is the part of server
>>>>> manager as we do have other parameters of Server dialog. We can
>>>>> isolate the SSH part in other class, but most of the modules (including
>>>>> Server module) have access to the ServerManager. If we will isolate
>>>>> that part then anyways we will have to write wrapper functions in
>>>>> ServerManager which will eventually call functions of new SSH class.
>>>>>
>>>>> As one ServerManager object belongs to one server, similarly
>>>>> one SSH Tunnel belongs to one server. When SSH tunnel gets created it will
>>>>> return local bind port, where rest of the communication should be done on
>>>>> local host and the local bind port return by the "SSHTunnelForwarder"
>>>>> class, so that need to be in the ServerManager.
>>>>>
>>>>> Considering above I have kept that logic in ServerManager.
>>>>>
>>>>>>
>>>>>>
>>>>>> - JS template. Eventually I would like to see if completely removed,
>>>>>> and the information that we are generating using the template can be passed
>>>>>> to the frontend via a Ajax call as an example( Do not think this is the
>>>>>> time to do it.)
>>>>>>
>>>>>> - start_running_query.py
>>>>>> - we could enrich the tests of this functionality
>>>>>>
>>>>>
>>>>> Added one test case for SSHTunnelConnectionLost.
>>>>>
>>>>>>
>>>>>>
>>>>>> And example of naming is for example on psycopg2/connection.py
>>>>>>
>>>>>> mgr = self.manager
>>>>>>
>>>>>> How much to we win by having this variable name versus manager =
>>>>>> self.manager or even using the self.manager?
>>>>>>
>>>>>
>>>>> Fixed. Replace "mgr" with "manager" almost at 69 places in the file.
>>>>>
>>>>> Attached is the modified patch. Please review it.
>>>>>
>>>>>>
>>>>>>
>>>>>> This is not for you in specific, but for @hackers in general:
>>>>>> The book https://www.amazon.com/dp/0132350882/
>>>>>> <https://www.amazon.com/dp/0132350882/?tag=stackoverflow17-20> is a
>>>>>> pretty nice book that gives you an introduction to clean code, that is self
>>>>>> documenting and that is much easily maintained.
>>>>>>
>>>>>> Thanks
>>>>>> Joao
>>>>>>
>>>>>> On Thu, Apr 26, 2018 at 3:44 AM Akshay Joshi <
>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Hackers,
>>>>>>>
>>>>>>> Attached is the updated patch which includes documentation of the
>>>>>>> feature and also updated screenshots of server dialog with new "SSH Tunnel"
>>>>>>> tab.
>>>>>>>
>>>>>>> On Wed, Apr 25, 2018 at 11:45 AM, Akshay Joshi <
>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Joao
>>>>>>>>
>>>>>>>> On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira <
>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>
>>>>>>>>> Hi Akshay,
>>>>>>>>>
>>>>>>>>> After looking through the patch we found some one letter variable
>>>>>>>>> names and this is a regression on what we have been trying to accomplish in
>>>>>>>>> the last year.
>>>>>>>>>
>>>>>>>>> An objective that we have for pgAdmin source code is to increase
>>>>>>>>> the testability of it and make it more readable. If we keep on adding one
>>>>>>>>> letter variables and if we continue adding code to already convoluted
>>>>>>>>> source files it is going to be very hard to achieve this objective.
>>>>>>>>>
>>>>>>>>
>>>>>>>> At my level I have tried not to give one letter variable
>>>>>>>> names. Are you talking about the variable "m" in server.js file
>>>>>>>> which represents the Model? If yes then I have followed the code written
>>>>>>>> for whole schema and I thought we have to maintain the consistency, so use
>>>>>>>> that as it is. Apart from that I haven't seen any other one letter
>>>>>>>> variable, please correct me so that I'll rename it.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Our recommendations for this change are:
>>>>>>>>> - Name the variables with comprehensive names
>>>>>>>>>
>>>>>>>> Can you please suggest from the patch.
>>>>>>>>
>>>>>>>>
>>>>>>>>> - Extract functions where we can and try to wrap some tests around
>>>>>>>>> them (ex: the javascript disabled functions)
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have tried to do that too, if you can see the "server/__init
>>>>>>>> __.py" file I have created "*get_response_for_password" function
>>>>>>>> to remove redundant code. Based on the condition it will return the json
>>>>>>>> response.*
>>>>>>>>
>>>>>>>>
>>>>>>>>> - We really need to find a better pattern than templated
>>>>>>>>> Javascript to pass information from the backend to the frontend
>>>>>>>>>
>>>>>>>> - When changing a piece of code, if we see code that is confusing
>>>>>>>>> or that is hard to read, we should refactor instead of adding to the
>>>>>>>>> pattern.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Please elaborate more with respect to my patch, which part of
>>>>>>>> code should required modification?
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Victoria & Joao
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Apr 24, 2018 at 10:13 AM Akshay Joshi <
>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Hackers
>>>>>>>>>>
>>>>>>>>>> As per suggestion by Dave, I have moved "Advanced" tab at the
>>>>>>>>>> last for Server dialog. Attached is the modified patch.
>>>>>>>>>>
>>>>>>>>>> On Mon, Apr 23, 2018 at 7:32 PM, Anthony Emengo <
>>>>>>>>>> aemengo(at)pivotal(dot)io> wrote:
>>>>>>>>>>
>>>>>>>>>>> For what it is worth, I manually verified that the feature
>>>>>>>>>>> worked, as well as looked through the code.
>>>>>>>>>>>
>>>>>>>>>>> I'd like to see end-to-end testing for regression sake, but it's
>>>>>>>>>>> hard to so at this moment.
>>>>>>>>>>>
>>>>>>>>>>> - Anthony and Joao.
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Apr 23, 2018 at 5:09 AM, Akshay Joshi <
>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo <
>>>>>>>>>>>>> aemengo(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hey Akshay
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This patch passed our test pipelines.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Did you test the feature and//or review the code and tests?
>>>>>>>>>>>>> Passing the tests is great, *if* the whole feature is covered (and the
>>>>>>>>>>>>> nature of this patch will make that quite difficult, maybe impossible to do
>>>>>>>>>>>>> without external infrastructure and config).
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Agreed, it's been difficult to write test case to test the
>>>>>>>>>>>> complete feature.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Anthony and Victoria
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi <
>>>>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Hackers
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I have implemented the SSH Tunnel support using
>>>>>>>>>>>>>>> https://pypi.org/project/sshtunnel/ python package. Added
>>>>>>>>>>>>>>> "SSH Tunnel" Tab in server dialog. This implementation supports user name
>>>>>>>>>>>>>>> /password and private/public key combination with Passphrase to crate SSH
>>>>>>>>>>>>>>> Tunnel. I have added regression test case to add server using SSH Tunnel
>>>>>>>>>>>>>>> options.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The given python package(https://pypi.org/project/sshtunnel
>>>>>>>>>>>>>>> /) support Python version *2.7, 3.4+*.
>>>>>>>>>>>>>>> It uses Paramiko (Python implementation of SSHv2 protocol)
>>>>>>>>>>>>>>> which actually drops support for Python 2.6. So I have added
>>>>>>>>>>>>>>> *SUPPORT_SSH_TUNNEL* parameter in config.py which checks
>>>>>>>>>>>>>>> the python version and set the flag accordingly. In case of Python 2.6,
>>>>>>>>>>>>>>> 3.0, 3.1, 3.2 and 3.3 control on the "SSH Tunnel" tab of server dialog will
>>>>>>>>>>>>>>> be disabled.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please review it, and if looks good please commit the code.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Sr. Software Architect *
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>>>>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Dave Page
>>>>>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>>>
>>>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>>
>>>>>>>>>>>> *Sr. Software Architect *
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>
>>>>>>>>>> *Sr. Software Architect *
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> *Akshay Joshi*
>>>>>>>>
>>>>>>>> *Sr. Software Architect *
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> *Akshay Joshi*
>>>>>>>
>>>>>>> *Sr. Software Architect *
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> *Akshay Joshi*
>>>>>
>>>>> *Sr. Software Architect *
>>>>>
>>>>>
>>>>>
>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> *Akshay Joshi*
>>>
>>> *Sr. Software Architect *
>>>
>>>
>>>
>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
>
> --
> *Akshay Joshi*
>
> *Sr. Software Architect *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company