Skip to content
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

Fixed alerting_api_integration/security_and_spaces tests failing if actions proxy set on for parallel process running using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner' #75232

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Aug 17, 2020

Added new environment variable ALERTING_PROXY_PORT with value "61${parallelId}5"
Tested locally by using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner'
Resolve #75386, #75522, #75722

…ctions proxy set on for parallel process running using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner'
@YulNaumenko YulNaumenko added bug Fixes for quality problems that affect the customer experience Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Aug 17, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner August 17, 2020 21:35
@YulNaumenko YulNaumenko self-assigned this Aug 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and works as expected 👍

I would love it if we could use a command line arg instead of commented out config, but it isn't a blocker :D

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned because if I run FTS / FTR without a const proxy port set in the config, all the tests pass for actions - both security and spaces, and spaces only. But I see a lot of warnings and errors in the FTS terminal showing http-related errors running actions.

Is the intention that if you want to run FTS / FTR locally you have to set the proxy port? If so, then we at least need an env var to set it, but then we might as well also have some port that we default to as well.

Another idea - FTS has to be running before you start FTR, so we could add a route to FTS that returned the proxy port, or the whole proxy url, which FTR could call to get it. Could be a lot of work tho.

@YulNaumenko
Copy link
Contributor Author

I'm a little concerned because if I run FTS / FTR without a const proxy port set in the config, all the tests pass for actions - both security and spaces, and spaces only. But I see a lot of warnings and errors in the FTS terminal showing http-related errors running actions.

Is the intention that if you want to run FTS / FTR locally you have to set the proxy port? If so, then we at least need an env var to set it, but then we might as well also have some port that we default to as well.

Another idea - FTS has to be running before you start FTR, so we could add a route to FTS that returned the proxy port, or the whole proxy url, which FTR could call to get it. Could be a lot of work tho.

@pmuellr you have a good catch here about the warnings in the FTS terminal - I realized that I added the fix with random proxy port but didn't do this for Webhook and Slack simulator servers. Why the tests are green I'm surprised - ops :)

@pmuellr
Copy link
Member

pmuellr commented Aug 19, 2020

I noticed that we're doing a logger.info() in the slack webhook when the proxy is used:

if (execOptions.proxySettings) {
proxyAgent = getProxyAgent(execOptions.proxySettings, logger);
logger.info(`IncomingWebhook was called with proxyUrl ${execOptions.proxySettings.proxyUrl}`);
}

and that we have a test that the logger.info() is run:

expect(mockedLogger.info).toHaveBeenCalledWith(
'IncomingWebhook was called with proxyUrl https://someproxyhost'
);

Seems like this should be changed to a logger.debug().

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; I noticed a logger.info() from the prior PR that seems like it should be changed to a debug(), noted in a separate comment.

…-local-tests-nonsingle-process

# 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.
@YulNaumenko YulNaumenko force-pushed the actions-proxy-fix-local-tests-nonsingle-process branch from 06fbc74 to f8560c4 Compare August 23, 2020 13:56
…-local-tests-nonsingle-process

# 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.
@YulNaumenko YulNaumenko requested a review from a team as a code owner August 24, 2020 19:23
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operations: Pipeline changes LGTM

…-local-tests-nonsingle-process

# 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.
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@YulNaumenko YulNaumenko merged commit e31a0c2 into elastic:master Aug 24, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Aug 25, 2020
…ctions proxy set on for parallel process running using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner' (elastic#75232)

* Fixed alerting_api_integration/security_and_spaces tests failing if actions proxy set on for parallel process running using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner'

* -

* Fixed get port from range for Slack and webhook simulators, removed some test warnings

* Added check for listening proxy server

* changed logger to debug removed not useful error

* -

* changed proxy to dynamic target in a single place

* test retry

* -

* -

* -

* -

* test with no cleanup

* -

* -

* -

* -

* Added environment variable ALERTING_PROXY_PORT

* fixed type checks

* fixed clean up proxy server port
YulNaumenko added a commit that referenced this pull request Aug 25, 2020
…ctions proxy set on for parallel process running using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner' (#75232) (#75884)

* Fixed alerting_api_integration/security_and_spaces tests failing if actions proxy set on for parallel process running using commands 'scripts/functional_tests_server' and 'scripts/functional_test_runner'

* -

* Fixed get port from range for Slack and webhook simulators, removed some test warnings

* Added check for listening proxy server

* changed logger to debug removed not useful error

* -

* changed proxy to dynamic target in a single place

* test retry

* -

* -

* -

* -

* test with no cleanup

* -

* -

* -

* -

* Added environment variable ALERTING_PROXY_PORT

* fixed type checks

* fixed clean up proxy server port
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 v8.0.0
Projects
None yet
6 participants