feat(#8591): adds option for enabling server name indication #8833

fardarter · 2024-01-30T11:07:35Z

Description

Have designed a unified wrapper for request-promise-native to centralise global options and to add a servername by default. It's overridable in the case that you ever want to have a different servername or an empty one.

When proxying to HTTPS from HTTP (for example where an ingress does TLS termination), not including a 'servername' for a request to the HTTPS server (eg, def.org) the request produces the following error (where abc.com is the calling server):

'ERR_TLS_CERT_ALTNAME_INVALID'
"RequestError: Error [ERR_TLS_CERT_ALTNAME_INVALID]: Hostname/IP does not match certificate's altnames:
Host: abc.com. is not in the cert's altnames: DNS:def.org"

The addition of servername as an option passed to the HTTP agent resolves this error.

See docs for tls.connect(options[, callback]) (https://nodejs.org/api/tls.html): "Server name for the SNI (Server Name Indication) TLS extension. It is the name of the host being connected to, and must be a host name, and not an IP address.".

Have written unit tests and have run other tests with the changes included. Relying on your CI for E2E and other tests.

Note: Have not changed the use of request-promise-native outside of calls to Couch.

Some prior discussions under this PR (branch name was throwing an error): #8579

Closes this issue: #8591

Readable: Concise, well named, follows the style guide, documented if necessary.
Tested: Unit and/or e2e where appropriate
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

fardarter · 2024-01-31T06:27:20Z

@garethbowen @dianabarsan We seem to have got most of the tests passing, but just wondering if you might not have a quick solve for the issue I'm seeing in testing.

Were you to look at one of the more recent runs on the sentinel integration tests (for example https://github.com/medic/cht-core/actions/runs/7713278685/job/21023413474?pr=8833), in the test with the string (for search): should create reminders in reminders.spec.js, the test times out but I've logged the result from:

return utils.sentinelDb.allDocs(opts).then(result => {

inside the getReminderLogs function, and I'm consistently seeing total_rows have a positive value but no rows actually returned in the rows array. Struggling to see how my changes have touched this.

This is what I'm seeing:

{ result: { total_rows: 40, offset: 37, rows: [] } }

Thoughts welcomed and appreciated.

garethbowen · 2024-02-23T08:57:17Z

@fardarter Following up here, the build issue you hit previous has been fixed in master so if you rebase you should be able to continue. There's now a conflict which will need to be fixed to get back up to date.

After that I'll see if I can figure out why the test is failing.

fardarter · 2024-02-23T10:28:28Z

@fardarter Following up here, the build issue you hit previous has been fixed in master so if you rebase you should be able to continue. There's now a conflict which will need to be fixed to get back up to date.

After that I'll see if I can figure out why the test is failing.

Run from the rebased version: https://github.com/medic/cht-core/actions/runs/8017517012

Not many tests still failing.

Branch has a bunch of extra commits for debugging but that stuff would come out for a non-draft PR.

Help is very much appreciated.

fardarter · 2024-03-18T11:57:41Z

@fardarter Following up here, the build issue you hit previous has been fixed in master so if you rebase you should be able to continue. There's now a conflict which will need to be fixed to get back up to date.

After that I'll see if I can figure out why the test is failing.

@garethbowen Happy to resync this if you will take a look?

garethbowen · 2024-04-01T05:37:24Z

@fardarter Yes let's get this going.

Steps I think are needed before review...

Resync and fix the conflicts.
Raise or link this PR to an issue to clearly outline the problem you're fixing.
Reduce the number of lines changed. I can see why the import change is necessary but there are also some unrelated changes in there. For example, let's not change the eslint config - that warrants some discussion on its own. Also it looks like you've added some debug logging that should be removed.

fardarter · 2024-04-02T06:17:03Z

@fardarter Yes let's get this going.

Steps I think are needed before review...

Resync and fix the conflicts.

Raise or link this PR to an issue to clearly outline the problem you're fixing.

Reduce the number of lines changed. I can see why the import change is necessary but there are also some unrelated changes in there. For example, let's not change the eslint config - that warrants some discussion on its own. Also it looks like you've added some debug logging that should be removed.

Hi @garethbowen. This PR is in a working state right now -- ie, I know there things in there that are not intended for merge. The eslint change is to support some of the code written, but I can split out some of the other things into other PRs. The tooling changes shouldn't be affecting the tests though.

Here is the linked issue: #8591

I have another PR on a branch name that's never going to work, so swapped to this just as a working branch. Happy to clean it all up once I have something that passes tests.

I'll sync and tidy this up this week. Note that this log here does not appear without the debugging code: #8833 (comment)

garethbowen · 2024-04-02T06:52:54Z

have a positive value but no rows actually returned in the rows array.

That's a valid response... the total_rows is the count of rows in the view before filtering and skipping is done: https://stackoverflow.com/questions/34805659/couchdb-views-total-rows-vs-offset-vs-rows

Does the test fail locally? If so I suggest debugging in there so you can chop the code and work out exactly which line is timing out.

fardarter · 2024-04-02T07:03:40Z

have a positive value but no rows actually returned in the rows array.

That's a valid response... the total_rows is the count of rows in the view before filtering and skipping is done: https://stackoverflow.com/questions/34805659/couchdb-views-total-rows-vs-offset-vs-rows

Does the test fail locally? If so I suggest debugging in there so you can chop the code and work out exactly which line is timing out.

That's a valid response

Sure, but the test is expecting values, and we're not sure why there are no values.

Does the test fail locally?

It does fail locally (on a VM), yes, but we're lacking context and experience on what it's testing and how it ought to be behaving, so we're finding it difficult to isolate what input might be responsible.

Locally, what's timing out is this (in tests/integration/sentinel/schedules/reminders.spec.js):

if (result.rows.length >= expectedLogs) {

Because it is polling for rows to have some length.

garethbowen · 2024-04-02T07:09:42Z

The test that's timing out is "should create reminders". It calls the getReminderLogs function multiple times. The first step is to figure out which call is the one that's timing out because that'll indicate which block of code is no longer working.

fardarter · 2024-04-02T07:21:28Z

The test that's timing out is "should create reminders". It calls the getReminderLogs function multiple times. The first step is to figure out which call is the one that's timing out because that'll indicate which block of code is no longer working.

It fails in the first call.

See the raw logs here: https://pipelinesghubeus2.actions.githubusercontent.com/Z1TT1tHFtNSb9nWma0rEOKax7paM32FuO78wdpig7IuFd2ZpHF/_apis/pipelines/1/runs/31561/signedlogcontent/24?urlExpires=2024-04-02T07%3A16%3A38.7727271Z&urlSigningMethod=HMACV1&urlSignature=psqzhl1bZ5wslpv9l52zvdYrzxLWdqGDE%2F%2BRaijSTs4%3D

Ctrf-f ----------------------------1 to find.

I have a number of other log points not being hit. The length of ---------------------------- is always the same in the code. We never get past 1.

garethbowen · 2024-04-02T07:40:22Z

@fardarter I think I've found it... If you go to the most recent build in github you can download an artifact for the build which contains the logs of the run. In the sentinel log there's this exception:

2024-02-23T10:14:44.506 DEBUG: Running reminder {
  form: 'FORM1',
  text_expression: 'at 09:11 on Fri',
  message: '{{name}} should do something'
} 
2024-02-23T10:14:44.509 ERROR: Task reminders completed with error: Error: "options" must be a plain object
    at Object.get (/service/shared-libs/couch-request/src/couch-request.js:54:29)
    at getPlaceIds (/service/sentinel/src/schedule/reminders.js:111:6)
    at getValidPlacesBatch (/service/sentinel/src/schedule/reminders.js:197:10)
    at sendReminders (/service/sentinel/src/schedule/reminders.js:249:10)
    at /service/sentinel/src/schedule/reminders.js:293:12
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  [stack]: 'Error: "options" must be a plain object\n' +
    '    at Object.get (/service/shared-libs/couch-request/src/couch-request.js:54:29)\n' +
    '    at getPlaceIds (/service/sentinel/src/schedule/reminders.js:111:6)\n' +
    '    at getValidPlacesBatch (/service/sentinel/src/schedule/reminders.js:197:10)\n' +
    '    at sendReminders (/service/sentinel/src/schedule/reminders.js:249:10)\n' +
    '    at /service/sentinel/src/schedule/reminders.js:293:12\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)',
  [message]: '"options" must be a plain object'
}

The configuration of this reminder matches the configuration in that test. It looks like the new couch-request lib is not a drop in replacement for request, in this case, the reminders function calls .get with a URL as the first param, which is valid in the request library but not in the new get function.

I think you need to update the couch-request library to be api compatible with the request library.

fardarter · 2024-04-02T07:46:22Z

@fardarter I think I've found it... If you go to the most recent build in github you can download an artifact for the build which contains the logs of the run. In the sentinel log there's this exception:
2024-02-23T10:14:44.506 DEBUG: Running reminder {
  form: 'FORM1',
  text_expression: 'at 09:11 on Fri',
  message: '{{name}} should do something'
} 
2024-02-23T10:14:44.509 ERROR: Task reminders completed with error: Error: "options" must be a plain object
    at Object.get (/service/shared-libs/couch-request/src/couch-request.js:54:29)
    at getPlaceIds (/service/sentinel/src/schedule/reminders.js:111:6)
    at getValidPlacesBatch (/service/sentinel/src/schedule/reminders.js:197:10)
    at sendReminders (/service/sentinel/src/schedule/reminders.js:249:10)
    at /service/sentinel/src/schedule/reminders.js:293:12
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  [stack]: 'Error: "options" must be a plain object\n' +
    '    at Object.get (/service/shared-libs/couch-request/src/couch-request.js:54:29)\n' +
    '    at getPlaceIds (/service/sentinel/src/schedule/reminders.js:111:6)\n' +
    '    at getValidPlacesBatch (/service/sentinel/src/schedule/reminders.js:197:10)\n' +
    '    at sendReminders (/service/sentinel/src/schedule/reminders.js:249:10)\n' +
    '    at /service/sentinel/src/schedule/reminders.js:293:12\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)',
  [message]: '"options" must be a plain object'
} 
The configuration of this reminder matches the configuration in that test. It looks like the new couch-request lib is not a drop in replacement for request, in this case, the reminders function calls .get with a URL as the first param, which is valid in the request library but not in the new get function.

I think you need to update the couch-request library to be api compatible with the request library.

Brilliant, thank you. I'll have a look a bit later.

An alternative is to enforce the options pattern in this codebase. I made some changes to that effect, but must have missed one.

This error is actually the one I wrote to fail noisily if the wrong pattern came in, so I guess that's a success, I just didn't know where to look for it.

fardarter · 2024-04-05T09:43:25Z

@garethbowen I have all the tests passing. Would be grateful for some attention when you can spare it.

garethbowen

Thanks for this!

I've suggested a range of tweaks, but nothing too major I hope!

garethbowen · 2024-04-05T09:59:31Z

.eslintrc

+    "unicode-bom": ["error", "never"],
+    "eqeqeq": ["error", "always", { "null": "ignore" }] // foo != null is a convenient way to check for null or undefined.


Revert this - it should be done as a separate commit.

Will put in a new commit.

New commit or new PR?

Sorry, new PR. Because we generally "squash and merge" each PR ends up in a single commit to the main branch.

Have PRed to the eslint-config repo. Would it be possible to get that merged if acceptable and then I can version bump here? medic/eslint-config#4

It looks like there's some debate about the right approach on that. I suggest making this PR pass lint without that change so it's not blocked.

.gitattributes

garethbowen · 2024-04-05T10:02:01Z

api/src/routing.js

+const { 
+  PROXY_CHANGE_ORIGIN = false,
+  PROXY_CHANGE_ORIGIN_AUTH = false,
+  PROXY_CHANGE_ORIGIN_CHANGES = false
+} = process.env;


To make it easier to reuse these, or at least see what options are available, I think these should go in the environment.js

Also can you add more info about what these three are for? Surely if you want to changeOrigin for one, you want to do it for all 3?

Will look into it.

Surely if you want to changeOrigin for one, you want to do it for all 3?

I didn't want to foreclose on optionality here, so I made each client individually configurable.

Would you prefer collapsing to one variable or having an override that sets them all while retaining the granularity?

Yeah let's not add complexity that we don't know there's a requirement for it. Stick with 1 if that works for your use case, and it'll be easy to add the other two if we have a good reason to later.

garethbowen · 2024-04-05T10:02:58Z

api/src/routing.js

+const isString = value => typeof value === 'string' || value instanceof String;
+const isTrue = value => isString(value) ? value.toLowerCase() === 'true' : value === true;


Move these to environment.js and then call them when reading the process.env so we just store the boolean in memory rather than executing this function every time we want to check.

I'll look at it.

By these do you mean the ultimate value for the process.env? Not sure I follow if you're referencing the functions as they're initialised in the file scope and initialised once at server.js. Happy to move the variables to environment.js though.

Ahh yes I see what you mean. Now that it's in environment I think this is clearer.

garethbowen · 2024-04-05T10:05:07Z

api/tests/mocha/services/setup/utils.spec.js


-      await expect(utils.makeUpgradeRequest(payload)).to.be.rejectedWith('No containers were updated');
+      await expect(utils.makeUpgradeRequest(payload)).to.eventually.be.rejectedWith('No containers were updated');


Why is this change necessary? I expect rpn and @medic/request to behave the same here.

This resolved some flakiness on the testing. It's a fix for the testing not the library.

That's what I'm worried about - unit test should not be flake because they control their environment. Let's see if we can fix this reliably...

I'll revisit. It was intermittent enough of a failure that I might just revert the change.

shared-libs/couch-request/test/couch-request.js

garethbowen · 2024-04-05T10:29:29Z

shared-libs/couch-request/test/couch-request.js

+  const options = [{ foo: 'bar' }];
+
+
+  options.forEach(option => {


No need to loop over an array of 1...

I generally do this so that new people can extend instead of having to restructure.

shared-libs/couch-request/test/couch-request.js

garethbowen · 2024-04-05T10:30:52Z

shared-libs/couch-request/test/couch-request.js

+  afterEach(() => {
+    requestGet.restore();
+    requestPost.restore();
+    requestPut.restore();
+    requestDelete.restore();
+    requestHead.restore();
+    sinon.restore();
+  });


I don't think this is necessary - sinon should restore automatically.

garethbowen · 2024-04-05T10:31:56Z

tests/e2e/upgrade/wdio.conf.js

@@ -8,7 +8,7 @@ const os = require('os');
 const chai = require('chai');
 const { spawn } = require('child_process');
 chai.use(require('chai-exclude'));
-const rpn = require('request-promise-native');
+const request = require('request-promise-native');


As above - don't rename the variable if nothing actually changed.

garethbowen · 2024-04-08T14:49:55Z

@fardarter Let me know when this is ready for another round.

fardarter · 2024-04-09T06:20:42Z

@fardarter Let me know when this is ready for another round.

@garethbowen I'll tag you directly.

fardarter · 2024-04-11T10:13:49Z

@fardarter Let me know when this is ready for another round.

@garethbowen I think this is ready for another round. Don't think there's a good reason for the tests failing, so please if someone could rerun. The same code ran successfully before squashing here: https://github.com/medic/cht-core/actions/runs/8644276073

fardarter · 2024-04-11T10:15:06Z

shared-libs/couch-request/src/couch-request.js

+//  name of the host being connected to, and must be a host name, and not an IP address.".
+// 
+
+const testReqInputs = (method, first, second = {}) => {


@garethbowen Refactoring this into its own function was purely to please SonarQube. Personally think the code is more readable and less complex in a single function.

garethbowen

Nice, thanks for simplifying this PR to just the necessary bits!

There are a couple of suggestions to reduce complexity, both for developers and implementers but nothing too complex.

garethbowen · 2024-04-19T07:19:50Z

api/src/environment.js

+  // If set to true, PROXY_CHANGE_ORIGIN_ALL will override the values of PROXY_CHANGE_ORIGIN, 
+  // PROXY_CHANGE_ORIGIN_AUTH and PROXY_CHANGE_ORIGIN_CHANGES and set them to true.
+  PROXY_CHANGE_ORIGIN_ALL = false, 
+  PROXY_CHANGE_ORIGIN = false,
+  PROXY_CHANGE_ORIGIN_AUTH = false,
+  PROXY_CHANGE_ORIGIN_CHANGES = false


Can you think of any time when you would want one proxy to have change origin but not the others? Because they're all going to proxy to the same couch instance the sni will either be required by all or by none. If you set one to true and the others to false you would end up with a really hard to debug setup. This complexity makes life much harder for implementers and unless there's a really good reason to include it we should aim to make their job as easy as possible.

Well, the existence of the multiple clients means that conceptually they could be pointed at differently configured entities, but if you don't know of any use cases, I'm OK with collapsing them.

My general bent is to expose configurability/apis where possible and to try not predict usage too much.

Will change.

garethbowen · 2024-04-19T07:21:01Z

api/src/routing.js

+const port = typeof environment.port !== 'undefined' && environment.port !== null ? `:${environment.port}` : '';
+const target = `${environment.protocol}//${environment.host}${port}`;
+const proxy = require('http-proxy').createProxyServer({
+  target: target, changeOrigin: environment?.proxies?.default?.changeOrigin ?? false 


Nit: For formatting let's put each value on its own line just like the two proxies below...

Suggested change

target: target, changeOrigin: environment?.proxies?.default?.changeOrigin ?? false

target,

changeOrigin: environment?.proxies?.default?.changeOrigin ?? false

@garethbowen have you ever considered adding prettier to the project?

Yes. It's very challenging to add it to an existing project because every time you modify a file it makes a bunch of unrelated changes that make the review difficult. Furthermore the developers agreed it actually made the code less readable so we removed it.

It only makes unrelated changes if it isn't enforced in CI?

I've moved projects over to it. You have a single format change and then there's no noise on PRs.

In fact, the main reasons I use it is to a) reduce noise on PRs b) avoid formatting issues coming up/being discussed in review.

If IDE integrated on save, it does have another advantage, which is that you get a snap to format on code that is structurally valid, so it's really great for hunting for closing brackets quickly.

I agree it's not always the best defaults (the ternary formatting is bad, for example), but there are few things I love less than debating formatting on review.

Speaking of which, going to make the requested change.

api/src/routing.js

garethbowen · 2024-04-19T07:38:10Z

shared-libs/couch-request/src/couch-request.js

+const req = (method, first, second = {}) => {
+
+  const { shouldReject, errorMessage, firstIsString = false } = testReqInputs(method, first, second);


One of the reasons this function is complicated is you're doing two things - one for working out if the first param is a string, and one to validate inputs. This would be cleaner if you cleaned up the parameters (and renamed them from first, and second before calling the testReqInputs function.

Then you can rename testReqInputs to validate which is actually what you're doing.

Finally you can have validate throw the exception directly which helps get a more accurate stack trace as well as cleaning up the code a lot.

I haven't tested this and it has wider implications but I think you can see what I mean below...

Suggested change

const req = (method, first, second = {}) => {

const { shouldReject, errorMessage, firstIsString = false } = testReqInputs(method, first, second);

const req = (method, first, options = {}) => {

if (typeof first === 'string' || first instanceof String) {

options.url = first;

}

try {

validate(method, options);

} catch(e) {

return Promise.reject(e);

}

I'll look at the code, but I don't see a better rename for the inputs.

first can either be the url or the options object, and I don't have a unifying name for that. second can be the options object or nothing, but if first is the options, then it doesn't make sense for second to be named options.

would be nice to have some decent overloading in js.

I also suspect that I can't add another logic branch to the function without triggering sonarqube, but will test that if that's true if the changes make sense.

shared-libs/transitions/package.json

fardarter · 2024-04-23T07:23:16Z

@garethbowen Please won't you rerun the tests? I've made the requested changes. Hope this passes and is ready.

garethbowen

Thank you for sticking with this!

There's a small batch of simple changes, and we'll need to pass the sonar check, which I think my suggestions will resolve.

Let me know when these are done and I'll make sure the builds pass.

api/src/environment.js

garethbowen · 2024-04-23T07:46:02Z

shared-libs/couch-request/src/couch-request.js

+  try {
+    validate(firstIsString, method, first, second);
+  } catch (e) {
+    return Promise.reject(Error(e));


There is no need to wrap the error in another error.

Suggested change

return Promise.reject(Error(e));

return Promise.reject(e);

shared-libs/couch-request/src/couch-request.js

garethbowen · 2024-04-23T07:47:37Z

shared-libs/couch-request/src/couch-request.js

+
+const req = (method, first, second = {}) => {
+
+  const firstIsString = typeof first === 'string' || first instanceof String;


You created the isString function which does this too - call that for reuse. I think this will also reduce the cognitive complexity enough to get sonar to pass.

Suggested change

const firstIsString = typeof first === 'string' || first instanceof String;

const firstIsString = isString(first);

Sigh. Can't agree with SQ here. Indirection is a major source of cognitive complexity. I've never liked Uncle Bob's approach, which seems to be driving the ruleset.

Abstraction IMO is both a cognitive and design overhead and should only be introduced with clear benefit. Use case in the other instance was a consistent way to normalise env inputs.

Some posts on the topic:

https://dev.to/wuz/stop-trying-to-be-so-dry-instead-write-everything-twice-wet-5g33

https://kentcdodds.com/blog/aha-programming

But I'll make the change. Just offering thoughts for future consideration.

fardarter · 2024-04-23T09:36:33Z

Thank you for sticking with this!

There's a small batch of simple changes, and we'll need to pass the sonar check, which I think my suggestions will resolve.

Let me know when these are done and I'll make sure the builds pass.

We need it, so have to. But I've been around long enough to know one has to listen to the rules of the house. No worries.

While I have your attention, would you be open to my backporting this to 4.3.x once it is in?

…sensitive environments; refactors request promise native to shared lib for central management

fardarter · 2024-04-23T09:39:57Z

@garethbowen Have amended as requested.

garethbowen · 2024-04-23T09:49:37Z

While I have your attention, would you be open to my backporting this to 4.3.x once it is in?

The problem with backporting to 4.3.x is you'd also have to backport to every release branch between that and master (4.4.x, 4.5.x, and 4.6.x) because otherwise someone updating from 4.3 to 4.4 would have a breaking change. Also because it touches so many files it's going to be difficult and risky to do. I don't see the value in that as as far as I know you're the only team who need it so I personally would not approve the backport.

Given the number of changes your team has made it looks like you've got your own fork already so I recommend you backport it to your fork.

Would you be amenable to also providing documentation in the cht-docs repo for this change?

fardarter · 2024-04-23T09:53:21Z

While I have your attention, would you be open to my backporting this to 4.3.x once it is in?

The problem with backporting to 4.3.x is you'd also have to backport to every release branch between that and master (4.4.x, 4.5.x, and 4.6.x) because otherwise someone updating from 4.3 to 4.4 would have a breaking change. Also because it touches so many files it's going to be difficult and risky to do. I don't see the value in that as as far as I know you're the only team who need it so I personally would not approve the backport.

Given the number of changes your team has made it looks like you've got your own fork already so I recommend you backport it to your fork.

Would you be amenable to also providing documentation in the cht-docs repo for this change?

We have the repo as a submodule with an explicit patch folder, so trying to reduce the overhead/complexity of the patch folder. I take your point, though. We'll just have to catch up before removing the patch.

Would you be amenable to also providing documentation in the cht-docs repo for this change?

Sure, point me in the right direction.

garethbowen · 2024-04-23T10:49:05Z

I guess add a section on this page: https://docs.communityhealthtoolkit.org/apps/guides/hosting/4.x/adding-tls-certificates/

If you go there you can click the "edit this page" link at the top right which will take you to the right place in the repo. You can either edit through GH or check out the repo as per usual.

garethbowen · 2024-04-23T10:51:25Z

Merged! Thank you @fardarter !!

fardarter · 2024-04-23T10:51:53Z

I guess add a section on this page: https://docs.communityhealthtoolkit.org/apps/guides/hosting/4.x/adding-tls-certificates/

If you go there you can click the "edit this page" link at the top right which will take you to the right place in the repo. You can either edit through GH or check out the repo as per usual.

Do you have a page for environment variables?

garethbowen · 2024-04-23T10:53:28Z

There's some info here: https://docs.communityhealthtoolkit.org/apps/guides/hosting/4.x/self-hosting/multiple-nodes/#prepare-environment-variables-file

fardarter force-pushed the adds-sni-environment-compat branch from f61426f to 5bff15b Compare February 23, 2024 09:59

fardarter force-pushed the adds-sni-environment-compat branch 6 times, most recently from 7dc850b to 3c05a66 Compare April 5, 2024 08:03

fardarter marked this pull request as ready for review April 5, 2024 09:42

garethbowen requested changes Apr 5, 2024

View reviewed changes

fardarter force-pushed the adds-sni-environment-compat branch 5 times, most recently from 8382f95 to 32e80b1 Compare April 11, 2024 09:33

fardarter commented Apr 11, 2024

View reviewed changes

garethbowen requested changes Apr 19, 2024

View reviewed changes

fardarter force-pushed the adds-sni-environment-compat branch from f5977c5 to 754ab88 Compare April 23, 2024 06:29

garethbowen requested changes Apr 23, 2024

View reviewed changes

feat(api, sentinal): adds servername and proxy origin switch for sni …

98df8ee

…sensitive environments; refactors request promise native to shared lib for central management

fardarter force-pushed the adds-sni-environment-compat branch from 754ab88 to 98df8ee Compare April 23, 2024 09:38

garethbowen approved these changes Apr 23, 2024

View reviewed changes

garethbowen changed the title ~~Adds sni environment compat~~ feat(#8591): adds option for enabling server name indication Apr 23, 2024

garethbowen merged commit 2402acd into medic:master Apr 23, 2024
16 checks passed

fardarter deleted the adds-sni-environment-compat branch April 23, 2024 10:52

		"unicode-bom": ["error", "never"],
		"eqeqeq": ["error", "always", { "null": "ignore" }] // foo != null is a convenient way to check for null or undefined.

		const isString = value => typeof value === 'string' \|\| value instanceof String;
		const isTrue = value => isString(value) ? value.toLowerCase() === 'true' : value === true;


		await expect(utils.makeUpgradeRequest(payload)).to.be.rejectedWith('No containers were updated');
		await expect(utils.makeUpgradeRequest(payload)).to.eventually.be.rejectedWith('No containers were updated');

		const options = [{ foo: 'bar' }];


		options.forEach(option => {

	target: target, changeOrigin: environment?.proxies?.default?.changeOrigin ?? false
	target,
	changeOrigin: environment?.proxies?.default?.changeOrigin ?? false

		const req = (method, first, second = {}) => {

		const { shouldReject, errorMessage, firstIsString = false } = testReqInputs(method, first, second);

-const req = (method, first, second = {}) => {
-  const { shouldReject, errorMessage, firstIsString = false } = testReqInputs(method, first, second);
+const req = (method, first, options = {}) => {
+  if (typeof first === 'string' || first instanceof String) {
+    options.url = first;
+  }
+  try {
+    validate(method, options);
+  } catch(e) {
+    return Promise.reject(e);
+  }


		const req = (method, first, second = {}) => {

		const firstIsString = typeof first === 'string' \|\| first instanceof String;

	const firstIsString = typeof first === 'string' \|\| first instanceof String;
	const firstIsString = isString(first);

feat(#8591): adds option for enabling server name indication #8833

feat(#8591): adds option for enabling server name indication #8833

Conversation

fardarter commented Jan 30, 2024 • edited Loading

Description

fardarter commented Jan 31, 2024

garethbowen commented Feb 23, 2024

fardarter commented Feb 23, 2024

fardarter commented Mar 18, 2024

garethbowen commented Apr 1, 2024

fardarter commented Apr 2, 2024 • edited Loading

garethbowen commented Apr 2, 2024

fardarter commented Apr 2, 2024

garethbowen commented Apr 2, 2024

fardarter commented Apr 2, 2024

garethbowen commented Apr 2, 2024

fardarter commented Apr 2, 2024 • edited Loading

fardarter commented Apr 5, 2024

garethbowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fardarter Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

garethbowen Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethbowen commented Apr 8, 2024

fardarter commented Apr 9, 2024

fardarter commented Apr 11, 2024

Choose a reason for hiding this comment

garethbowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fardarter Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

fardarter commented Apr 23, 2024

garethbowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fardarter commented Apr 23, 2024

fardarter commented Apr 23, 2024

garethbowen commented Apr 23, 2024

fardarter commented Apr 23, 2024

garethbowen commented Apr 23, 2024

garethbowen commented Apr 23, 2024

fardarter commented Apr 23, 2024

garethbowen commented Apr 23, 2024

fardarter commented Jan 30, 2024 •

edited

Loading

fardarter commented Apr 2, 2024 •

edited

Loading

fardarter commented Apr 2, 2024 •

edited

Loading

fardarter Apr 10, 2024 •

edited

Loading

garethbowen Apr 17, 2024 •

edited

Loading

fardarter Apr 19, 2024 •

edited

Loading