Allow to pass custom axios config

[m-szyszka]

Value

When library works in Azure cloud, with high load, it's possible to hit SNAT Port Exhaustion issue https://learn.microsoft.com/en-us/azure/load-balancer/load-balancer-outbound-connections#port-exhaustion Suggested method to mitigate it is to add "agentkeepalive" to axios config. Solution is described here https://azureossd.github.io/2022/03/10/NodeJS-with-Keep-Alives-and-Connection-Reuse/

Proposed approach

• add optional argument to ApiClient class constructor, with custom axios properties
• add this argument also to NotifyClient class constructor Client applications will be able to pass own axios configuration with custom defined http/https agents.

[idavidmcdonald]

Hey 👋

Thanks for raising this issue.

Firstly, we are a small team and we will need to prioritise this request alongside our other work. To help us with prioritisation

1. Any one else affected and keen to see this worked on should leave a emoji to indicate you are also interested in this.
2. Any additional information you can provide is useful. For example do you have any workarounds available to you? What is the impact to you of us not doing this?

We are also open to contributions (although we can't guarantee a speedy response always as this takes some time to review). Let us know if you would be interested in making a contribution for this and we can discuss the approach if need be and any other tips.

My initial thoughts are that making our NotifyClient more flexible so you can override almost any of the axios options would be the most flexible (and therefore best) solution. That way, if someone comes wanting to set a proxy, they can do so. If someone comes wanting to set a custom http agent they can do that, if someone comes with any other use case then they are able to set that appropriately (Rather than us needing to add explicit support for each individual axios use case to the client).

We would also look to avoid making a breaking change if we were to go ahead with this.

Thanks!

[m-szyszka]

Hey @idavidmcdonald

More details of my request:

Impact

notifications-node-client is used as a part of asynchronous process. Without custom axios options, when it hit SNAT Port Exhaustion issue in Azure cloud, outgoing requests (from our app to GOV Notify) are frequently timed out. In result, requests must be retried, which also must be provided by our client code, due to lack of options.

Proposed change

• NotificationClient already has setProxy method https://github.com/alphagov/notifications-node-client/blob/ff948ad650dfa14fc40103230618e132b412b2d5/client/notification.js#L345
• additional method setAxiosConfig could be added, proposed syntax:

setAxiosConfig: function(axiosConfig) {
    this.apiClient.setAxiosConfig(axiosConfig);
}

• similar method added to ApiClient:

setAxiosConfig: function(axiosConfig){
    this.axiosConfig = axiosConfig
}

• methods get and post of ApiClient extended to respect axiosConfig:

get: function(path, additionalOptions) {
    var options = {
      method: 'get',
      url: this.urlBase + path,
      headers: {
        'Authorization': 'Bearer ' + createToken('GET', path, this.apiKeyId, this.serviceId),
        'User-Agent': 'NOTIFY-API-NODE-CLIENT/' + version
      }
    };
    Object.assign(options, additionalOptions)

    if(this.axiosConfig !== null) Object.assign(options, this.axiosConfig);   // <---- ADDED LINE

    if(this.proxy !== null) options.proxy = this.proxy;

    return restClient(options);
  },

That way it won't be a breaking change, existing interfaces won't be touched, just one new method will be added.

[opyate]

@m-szyszka wondering if this feature might be useful, so you can provide a pre-configured underlying Axios instance:

https://github.com/alphagov/notifications-node-client/pull/200

[m-szyszka]

Thats exactly what we need, thanks!

[m-szyszka]

Hi @opyate . What else do we need to close the Merge Request and publish a new version? This is important to us as we intend to increase the number of notifications sent and we are concerned about SNAT Port Exhaustion.

[opyate]

Hi @opyate . What else do we need to close the Merge Request and publish a new version? This is important to us as we intend to increase the number of notifications sent and we are concerned about SNAT Port Exhaustion.

Badger the repo owners, I suppose :-P

[m-szyszka]

Hey @idavidmcdonald Could you give us guidance what else needs to be done to complete this merge request?

[idavidmcdonald]

Thank you both (for your patience and contribution) ⭐

We've just released 8.2.0 which is now available. Let us know how you get on with it and if you encounter any issues.

I'll close this issue as I think the release takes care of this, but do correct us if that isn't the case and there is anything remaining.

Thank you again!

[m-szyszka]

Actually it's not fully completed, because new "setClient" is not exposed. It's addressed in this PR https://github.com/alphagov/notifications-node-client/pull/202 (thanks @opyate ) @idavidmcdonald could you take a look?

[CrystalPea]

Hey @m-szyszka , @idavidmcdonald is not on our team anymore unfortunately, but I will merge the bug fix.

[quis]

Version 8.2.1 is now available on npm with the setClient fix included. Thanks again for your work on this