-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Actions add proxy support #74289
Actions add proxy support #74289
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
…-support # Conflicts: # x-pack/plugins/actions/server/builtin_action_types/slack.ts # x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts # x-pack/plugins/actions/server/types.ts
…-support # Conflicts: # x-pack/package.json
…-support # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
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'm just about half way through the code, I figured I'd send what I have so far as I won't have time until tomorrow to finish the rest.
I think there should also be a mention of proxy in the developing new action types doc.
x-pack/plugins/actions/server/builtin_action_types/resilient/index.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/server/builtin_action_types/lib/get_proxy_agent.ts
Outdated
Show resolved
Hide resolved
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 made a bunch of comments, but many are just comments or nits, and I don't think there are any blockers. I'm guessing we may need to tweak the proxy bits over time - my experience is that I've had to do that whenever I've added proxy support like this to a product - so anything I mentioned that would be "nice to have", we can do later as needed.
@@ -6,10 +6,10 @@ | |||
|
|||
// info on nodemailer: https://nodemailer.com/about/ | |||
import nodemailer from 'nodemailer'; | |||
|
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 was confused by the references to Nodemailer and using it with an http proxy - I hadn't realized that was even possible! Quick check yields thread - 2nd entry there is very good talking about CONNECT, etc - http://squid-web-proxy-cache.1019090.n4.nabble.com/Can-squid-be-configured-as-SMTP-SMTPS-proxy-td2727188.html
@@ -72,8 +72,9 @@ | |||
"@types/gulp": "^4.0.6", | |||
"@types/hapi__wreck": "^15.0.1", | |||
"@types/he": "^1.1.1", | |||
"@types/hoist-non-react-statics": "^3.3.1", |
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.
seems slightly odd that the package names got resorted here, though it looks like they sorted CORRECTLY now, and weren't before. Like someone previously edited the file manually. It's just @types/
packages anyway though, so not a big deal, and it looks fine to me.
x-pack/plugins/actions/server/builtin_action_types/lib/get_proxy_agent.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/server/builtin_action_types/lib/get_proxy_agent.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/common/lib/get_proxy_server.ts
Outdated
Show resolved
Hide resolved
...test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/jira.ts
Outdated
Show resolved
Hide resolved
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.
Changes LGTM, great work! I've finished reviewing the remaining files.
createExternalService: ( | ||
credentials: ExternalServiceCredentials, | ||
logger: Logger, | ||
proxySettings?: any |
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.
Can proxySettings
use ProxySettings
type instead of using any?
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 a got in this case node plugins/security_solution/scripts/check_circular_deps
failing. I made it 'any', because when the Case team change their Jira, IBM resilient according to the ServiceNow example this code will be removed.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* Added proxy support for action types * Fixed tests * added rejectUnauthorizedCertificates config setting * removed slack not used code * Fixed Slack proxy * fixed typecheck errors * Cleanup code * Fixed slack * Added unit tests * added proxy server for test * Fixed build * Added functional tests * fixed due to comments * Fixed tests and some changes due to comments * Fixed functional tests * fixed circular deps * Added proxy unit test to action type
* master: (24 commits) [ML] Functional tests - skip regression and classification tests [Ingest Manager] fix removing ingest pipelines from elasticsearch (elastic#75092) move tests for placeholder indices to setup (elastic#75096) [jest] temporarily extend default test timeout (elastic#75118) [cli] remove reference to removed --optimize flag (elastic#75083) skip flaky suite (elastic#75044) Adding /etc/rc.d/init.d/functions to the init script when present to … (elastic#22985) [jenkins] add pipeline for hourly security solution cypress tests (elastic#75087) [Reporting/Flaky Test] Skip test for paging list of reports (elastic#75075) remove .kbn-optimizer-cache upload (elastic#75086) skip flaky suite (elastic#74814) Actions add proxy support (elastic#74289) [ILM] TS conversion of Edit policy components (elastic#74747) [Resolver] simulator tests select elements directly instead of using descendant selectors. (elastic#75058) [Enterprise Search] Add Workplace Search side navigation (elastic#74894) [Security solution] Sourcerer: Kibana index pattern selector for security views (elastic#74706) [Logs UI] Remove apollo deps from log link-to routes (elastic#74502) [Maps] add map configurations to docker list (elastic#75035) [functional test][saved objects] update tests for additional copy saved objects to space (elastic#74907) Make the alerts plugin support generics (elastic#72716) ...
* Added proxy support for action types * Fixed tests * added rejectUnauthorizedCertificates config setting * removed slack not used code * Fixed Slack proxy * fixed typecheck errors * Cleanup code * Fixed slack * Added unit tests * added proxy server for test * Fixed build * Added functional tests * fixed due to comments * Fixed tests and some changes due to comments * Fixed functional tests * fixed circular deps * Added proxy unit test to action type
Added proxy settings for defining proxyUrl and proxyHeaders under kibana.yml
xpack.actions
configuration.The decision was to use HTTP/HTTPS proxy, not SOCKS, because it is the most commonly used and all action types libraries support it.
For enabling self-signed certificates added
rejectUnauthorized: false
for all external requests (axios, nodemailer, slack).node-slack-sdk/webhook
implemented by using axios library. So approach here is really the same - used HttpsProxyAgent and HttpProxyAgent.rejectUnauthorized: false
which is configurable only under the agent options. So implemented proxy support usinghttpsAgent
for HTTPS proxy and HttpProxyAgent for the HTTP proxy.Resolve #50267