-
Notifications
You must be signed in to change notification settings - Fork 47
feat(proxy): synthetics cli proxy support #1044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Working on unit test rn |
@mcapell @vigneshshanmugam This is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good.
src/options.ts
Outdated
try { | ||
return readFileSync(filePath); | ||
} catch (e) { | ||
throw new Error(`could not be read provided path ${filePath}: ${e}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we keep the error specific to proxy settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the flag the error applies to, this is generic to any file-based option so I don't think it makes sense to restrict this only to proxy settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested it locally and it works
Hi Team, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with few nits
) | ||
.option( | ||
'--proxy-no-verify', | ||
'disable TLS verification for the proxy connection', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does overriding NODE_TLS_UNAUTHORIZED disable this? can you callout this in the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioning NODE_TLS_REJECT_UNAUTHORIZED in our docs, as a featured that is strongly discouraged in nodejs docs, seems to me like would have the opposite effect and it's also inconsistent, we don't include it in our docs for Kibana TLS verification. It's not really overriding anything either, both are on by default. I'm inclined to merge it as it is and we can extend the docs if we get any corner case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I've tested with version 1.19.0 and seems the new code in the main is not tagged yet!!! Can someone take a look into this please? |
Summary
push
command #949.Add global proxy to push/locations command with:
HTTP_PROXY
,HTTPS_PROXY
,NO_PROXY
.--proxy-uri
:--proxy-token
: Based in auth header format, egBasic Asaaas==
--proxy-ca
: Custom CA chain file or string for self-signed certificate validation. Based on nodejs TLS secure context.--proxy-cert
: Custom cert chain file or string for self-signed certificate validation. Based on nodejs TLS secure context--proxy-no-verify
: Disable proxy TLS verification.