bjowes / cypress-ntlm-auth

Windows authentication plugin for Cypress
MIT License
55 stars 10 forks source link

Is there a way to specify a port for the proxy to use? #129

Closed gabi-dobritescu closed 3 years ago

gabi-dobritescu commented 4 years ago

Hi,

I'm wondering if there is a way to specify a port for the proxy to use.

I use the ntlm proxy on our CI boxes. Each CI box hosts 3 build agents. At the moment each build agent starts a ntml proxy instance. This is causing us issues when multiple build agents are being used on the same CI box. The second agent that gets engaged will start a new ntml proxy instance and that will result in overriding the config file which results in breaking the test run already happening on the build agent that was engaged first.

What I want to do is to start a ntlm proxy instance as part of the CI box initialisation process and keep it long running to be shared by the 3 agents running on each box. Because we share the CI boxes across many teams I need to have a way to specify which port the proxy needs to use, otherwise I run into the risk of having other build processes trying to gain access to the same port.

bjowes commented 3 years ago

Hi @gabi-dobritescu - the current ntlm-proxy does not really support scenarios where you run multiple cypress instances in parallel, and there is no way to set the port manually. I have seen similar feature requests in the past and I am considering implementing this. Likely it would then not be one proxy instance for all cypress instances, but rather that start/stop and port settings for the proxy is more tightly integrated into the cypress startup, hence removing the "ports" file completely.

If you are interested to help out that would be welcome.

gabi-dobritescu commented 3 years ago

Hi Björn,

Thanks for the quick reply.

I'm interested in helping out - let me know what you have in mind.

Not sure what you mean when you say "ntlm-proxy does not really support scenarios where you run multiple cypress instances in parallel". I've done an experiment where I had 2 running Cypress instances using 1 ntlm-proxy instance and it worked just fine.

bjowes commented 3 years ago

I have reconsidered the startup process for cypress-ntlm. To support multiple instances, I think the best approach is to use one instance of ntlm-proxy with each instance of cypress. Otherwise there will be issues if each instance of cypress sends different configurations to the proxy.

The key is to start the proxy inside the cypress-ntlm command, and pass the resulting ports directly to cypress. This way the ports file is no longer needed. I have avoided this path in the past since I was concerned that there might be side effects of launching both the proxy and cypress from the same node instance. But the upside is that setup of the plugin and launching is much simpler than before, so I think it is worth to evaluate it. This is where I would need your assistance.

I have done a quick and dirty beta version which you can install with `npm i cypress-ntlm-auth@3.0.0-beta.0``

Once installed, comment out anything related to the plugin from cypress/plugins/index.js, it isn't needed anymore. Also update your scripts in package.json. Referring the the examples from the docs, the "ntlm-proxy"script isn't needed anymore. And replace the "cypress-ntlm"script with: "cypress-ntlm": "cypress-ntlm open" It should work with arguments just like before, and the "run" variant is also still supported.

bjowes commented 3 years ago

Or just do npx cypress-ntlm open from the command line directly. No scripts needed.

bjowes commented 3 years ago

Made some more improvements, released in cypress-ntlm-auth@3.0.0-beta.1

bjowes commented 3 years ago

Tested on Windows, works with release cypress-ntlm-auth@3.0.0-beta.2

bjowes commented 3 years ago

Updated again to terminate the process after cypress-ntlm run, there is a known issue with cypress hanging at this stage on Windows. cypress-ntlm-auth@3.0.0-beta.3

gabi-dobritescu commented 3 years ago

Thanks for all the updates Björn! 👍

I'm going to pick up the latest beta and give it a try. I'll let you know how it goes.

gabi-dobritescu commented 3 years ago

Hi Björn,

I gave the beta a try.

I can confirm that creating one ntlm-proxy instance for each cypress runner and passing in the proxy configuration to the runner works fine (for now I've just with tried multiple cypress open commands).

I did run into an issue though. Passing a config file argument in the command line doesn't get parsed properly.

To reproduce it just run a command like: npx cypress-ntlm run --config-file=myConfigFile.json. The error message reads:

The "path" argument must be of type string. Received type boolean
TypeError [ERR_INVALID_ARG_TYPE] [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type boolean

By enabling debug logging I can see these messages:

  cypress:cli exporting Cypress module interface +0ms
  cypress:cli:cli creating program parser +0ms
  cypress:cli:cli parsing args: [ null, null, 'run', '--config-file=myConfigFile.json' ] +3ms
  ...
  cypress:cli parsed cli options { configFile: 'myConfigFile.json' } +0ms
  cypress:cli:cli parsed options { configFile: 'myConfigFile.json' } +46ms
  cypress:cli:cli casted options { configFile: true } +1ms

From these messages it looks like something goes wrong internally within Cypress. But since I didn't run into this issue with the previous version of the ntlm-proxy, it stands to reason that it could be related to the latest changes made for this beta version.

I'm using Cypress v5.1.0 and cypress-ntlm-auth@3.0.0-beta.3.

bjowes commented 3 years ago

Great that it works for you! In the past, the launcher called an internal init method within cypress to launch it, and then cypress parsed the arguments. In the beta, I am using the more official way with a cypress API method for parsing arguments. Apparently this method does not process the config-file argument properly, those logs look quite strange. configFile does accept either a string or a boolean (false), so I guess there is a mixup in the parsing.

I raised an issue with cypress for this.

I would really prefer to stick to the parseRunArguments method since it is an official API, and thereby less likely to be suddenly changed. So for now I don't think you can pass the configFile argument, unless you use the module API of the plugin. But I think that generally any adaptions you make in the custom config file can be passed as arguments instead. If you don't make lots of changes compared to the defaults in cypress.json, maybe that is the easiest way forward.

gabi-dobritescu commented 3 years ago

Hi Björn,

Thanks for raising the issue with the Cypress team. Seems like your suggested fix and PR will be accepted. Looking forward to having the fix released - hopefully soon 😃.

In the mean time, are you going to promote the beta to a main release? The new approach makes starting/closing the proxy so much simpler and eliminates the need to update the plugins index.js file. Overall, a more streamlined way to use the plugin.

I bet all current and future users of the plugin will appreciate it! 👍

bjowes commented 3 years ago

I agree that the beta looks quite promising, but I will need to review the docs and tests again before going stable.

gabi-dobritescu commented 3 years ago

Completely understand. Looking forward to the stable release! 👍

bjowes commented 3 years ago

@gabi-dobritescu - the cli argument fix is now released in cypress 5.3.0

gabi-dobritescu commented 3 years ago

@bjowes - Got the updated version and the fix is working fine 🎉 🎉 🎉

bjowes commented 3 years ago

Implemented in 3.0.0