fix: respect NODE_TLS_REJECT_UNAUTHORIZED environment variable #2602
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
size/L
|
/lgtm Thanks for updating the test. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0x5457, brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Tests are failing with some error related to generating the certificate. |
| @@ -83,6 +83,7 @@ | |||
| "nock": "^14.0.5", | |||
| "prettier": "^3.0.0", | |||
| "pretty-quick": "^4.0.0", | |||
| "selfsigned": "^3.0.1", | |||
There was a problem hiding this comment.
So, selfsigned looks to be about 1.7MB, and it is only used in one test. It might be simpler to use a fixture certificate.
| @@ -202,7 +202,9 @@ export class KubeConfig implements SecurityAuthentication { | |||
| agentOptions.key = opts.key; | |||
| agentOptions.pfx = opts.pfx; | |||
| agentOptions.passphrase = opts.passphrase; | |||
| agentOptions.rejectUnauthorized = opts.rejectUnauthorized; | |||
| if (opts.rejectUnauthorized !== undefined) { | |||
There was a problem hiding this comment.
The changes in this file are very likely to be refactored in the future by someone missing the context of this PR.
Can you either add comments to the two code blocks in this file, or rewrite as something along the lines of agentOptions.rejectUnauthorized = opts.rejectUnauthorized ?? process.env.NODE_TLS_REJECT_UNAUTHORIZED !== '0';. If you go with the second option, it probably makes sense to put the logic in a function, similar to what Node core does.
| strictEqual(res2.status, 200); | ||
| strictEqual(await res2.text(), 'ok'); | ||
|
|
||
| delete process.env.NODE_TLS_REJECT_UNAUTHORIZED; |
There was a problem hiding this comment.
Is this line necessary since the after() hook is assigning a value to process.env.NODE_TLS_REJECT_UNAUTHORIZED?
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; | ||
| after(() => { | ||
| server.close(); | ||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = originalValue; |
There was a problem hiding this comment.
I would reverse the order of these two lines in case server.close() throws for some reason.
fix: #2601
After testing, I found that the fetch behavior is as follows:
Also should I can create test certificates in the test-data directory to avoid introducing the selfsigned library