-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Tests] Add http integration test setup #19261
Conversation
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
retest |
💚 Build Succeeded |
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 like the direction this is going, but I think we should find a way to validate the certificate that the server is using rather than disabling SSL validation and I'm also curious about a couple seemingly unrelated code removals.
tasks/config/run.js
Outdated
@@ -185,7 +185,6 @@ module.exports = function (grunt) { | |||
args: [ | |||
'scripts/functional_tests_server', | |||
'--config', 'test/functional/config.js', | |||
'--esFrom', 'source', |
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.
Why did this need to go?
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 think this was a bad merge...
@@ -43,7 +43,6 @@ export default async function ({ readConfigFile }) { | |||
junit: { | |||
reportName: 'API Integration Tests' | |||
}, | |||
env: commonConfig.get('env'), |
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.
Why remove this and the one in test/functional/config
?
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 think this is a bad merge. The esFrom source
removal disappeared once I rebased over master again. Let me check.
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 have the answer. The env
value was removed from test/common/config.js
because it used to have the kibana/es server specific stuff. These are left over in the test/functional/config.js
and test/api_integration/config.js
. It's part of the cleanup that didn't get removed earlier.
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 we remove this env
from x-pack test configs (4 places) and schema (src/functional_test_runner/lib/config/schema.js
) as well then?
P.S. it seems we have transform_deprecations
for deprecated values in test config, wondering who cares about that 🙈
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.
Neither of these is part of this PR, but I did go ahead and remove env
. Hopefully that isn't too confusing for future reviewers. :) Can you point to what you're talking about with transform_deprecations
?
test/integration/http/ssl/index.js
Outdated
* When testing https server, set this to false. | ||
* https://github.com/visionmedia/supertest/issues/396 | ||
*/ | ||
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; |
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.
In general I think we should avoid disabling "reject unauthorized" behavior if possible, and since we are using a specific ssl.certificate
when we start Kibana we should be able to use that certificate to indicate that servers using that certificate are "authorized". Probably with something like:
export function KibanaSupertestProvider({ getService }) {
const config = getService('config');
const kibanaServerUrl = formatUrl(config.get('servers.kibana'));
const agent = supertestAsPromised.agent();
// maybe we should expose a separate config option for this?
const kibanaServerCert = config.get('kbnTestServer.serverArgs')
.filter(arg => arg.startsWith('--server.ssl.certificate'))
.map(arg => arg.split('=').pop())
.map(path => readFileSync(path))
.shift()
if (kibanaServerCert) {
agent.ca(kibanaServerCert)
}
return agent(kibanaServerUrl)
}
That way we will also be ensuring that the server is using SSL and that it's using the right certificates. That's probably better, don't you think?
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.
Looks good. I agree. I didn't know about this option on supertest. TY!
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.
// maybe we should expose a separate config option for this?
+1 for config option. It's not very convenient to pass one with this provider approach though, I was thinking about something like this:
supertest: (options) => KibanaSupertestProvider({
...options,
ca: require.resolve('../../../dev_certs/server.crt'),
}),
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.
@spalger The problem is, the dev cert is self signed, and for some reason there's an error when using it:
│ proc [ftr] └- ✖ fail: "kibana server with ssl handles requests using ssl"
│ proc [ftr] │ Error: self signed certificate
│ proc [ftr] │ at TLSSocket.<anonymous> (_tls_wrap.js:1105:38)
│ proc [ftr] │ at TLSSocket._finishInit (_tls_wrap.js:639:8)
│ proc [ftr] │ at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:469:38)
This might be bad advice, but it references the self signed cert issue. Any other ideas? Maybe we can try making a valid cert that isn't self-signed?
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.
Here's more context for what node is doing:
nodejs/node-v0.x-archive#4023
tls, https: validate server certificates by default
This commit changes the default value of the rejectUnauthorized option from
false to true.
What that means is that tls.connect(), https.get() and https.request() will
reject invalid server certificates from now on, including self-signed
certificates.
There is an escape hatch: if you set the NODE_TLS_REJECT_UNAUTHORIZED
environment variable to the literal string "0", node.js reverts to its
old behavior.
Could it be that when setting process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
, self-signed certs are still being accepted, as this comment implies?
https://github.com/nodejs/node-v0.x-archive/pull/4023/files#diff-cbfd1300d9c62cf7f325159582f45d66R27
// disable strict server certificate validation by the client
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
@@ -106,8 +106,6 @@ export default async function ({ readConfigFile }) { | |||
}, | |||
servers: commonConfig.get('servers'), | |||
|
|||
env: commonConfig.get('env'), |
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.
Cleanup only.
💚 Build Succeeded |
@@ -43,7 +43,6 @@ export default async function ({ readConfigFile }) { | |||
junit: { | |||
reportName: 'API Integration Tests' | |||
}, | |||
env: commonConfig.get('env'), |
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 we remove this env
from x-pack test configs (4 places) and schema (src/functional_test_runner/lib/config/schema.js
) as well then?
P.S. it seems we have transform_deprecations
for deprecated values in test config, wondering who cares about that 🙈
return supertestAsPromised(kibanaServerUrl); | ||
} | ||
|
||
export function KibanaSupertestWithoutAuthProvider({ getService }) { |
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.
note: I don't think we need to separate KibanaSupertestProvider
and KibanaSupertestWithoutAuthProvider
in OSS kibana since there is no authentication supported, it can probably be just KibanaSupertestProvider
since withoutAuth
is implicitly expected. What do you think?
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.
Currently these tests are using supertest
which is the version with auth. Should they be using without auth? Hm. Doesn't OSS Kibana offer Basic Auth? The default superagent without any auth setting specified includes Basic Auth.
serverArgs: [ | ||
...httpConfig.get('kbnTestServer.serverArgs'), | ||
'--server.basePath=/abc/xyz', | ||
'--server.rewriteBasePath=true', |
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.
note: it's kind of confusing that for no_rewrite
tests we use --server.rewriteBasePath=true
for rewrite
tests we use --server.rewriteBasePath=false
, what was the idea behind that?
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 think I swapped the naming.
describe('Kibana server with basePath and with rewriteBasePath', () => { | ||
const basePath = '/abc/xyz'; | ||
|
||
it('requires root requests to contain basePath', async () => { |
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.
note: would be great if you can add simple test to test that behavior when Kibana notices "old" base path and redirects to new one.
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 agree!
test/integration/http/ssl/index.js
Outdated
* When testing https server, set this to false. | ||
* https://github.com/visionmedia/supertest/issues/396 | ||
*/ | ||
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; |
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.
// maybe we should expose a separate config option for this?
+1 for config option. It's not very convenient to pass one with this provider approach though, I was thinking about something like this:
supertest: (options) => KibanaSupertestProvider({
...options,
ca: require.resolve('../../../dev_certs/server.crt'),
}),
It's likely not related to this PR, but just curious. I see we run Kibana for tests with the following command: node scripts/kibana --env.name=development --logging.json=false --server.port=5620 \
--optimize.watchPort=5630 --optimize.watchPrebuild=true --status.allowAnonymous=true \
--optimize.enabled=true --elasticsearch.url=http://elastic:changeme@localhost:9200 \
--elasticsearch.username=elastic --elasticsearch.password=changeme --oss --optimize.enabled=false \
--elasticsearch.healthCheck.delay=3600000 --server.xsrf.disableProtection=true \
--server.ssl.enabled=true --server.ssl.key=/projects/elastic/master/kibana/test/dev_certs/server.key \
--server.ssl.certificate=/projects/elastic/master/kibana/test/dev_certs/server.crt --no-base-path \
--optimize.bundleDir=/projects/elastic/master/kibana/optimize/bundles If it's |
💔 Build Failed |
💔 Build Failed |
@@ -59,12 +59,35 @@ function collectCliArgs(config, { installDir, extraKbnOpts }) { | |||
serverArgs, | |||
args => (installDir ? args.filter(a => a !== '--oss') : args), | |||
args => { | |||
return installDir ? [...args, ...buildArgs] : [KIBANA_EXEC_PATH, ...args, ...sourceArgs]; | |||
return installDir ? [...buildArgs, ...args] : [KIBANA_EXEC_PATH, ...sourceArgs, ...args]; |
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.
args
refers to all the options that the user passes in via command line and/or the particular test config.js they're using for starting the Kibana test server.
Previously we were appending sourceArgs
(or buildArgs
if they passed in --kibana-install-dir
) to the end of their passed-in args
. But this doesn't work when they're using --server.basePath
because sourceArgs
was always overriding it with --no-base-path
option. In fact for base path settings, the ordering doesn't matter because no-base-path
and server.basePath
are different keys; just the existence of no-base-path
disables the base path proxy server.
Now, I've flipped the ordering so that the user's args
always override the default build/source args. And, I've made sure that any base path setting that comes later, whether or not the keys match, will override previous base path settings.
💚 Build Succeeded |
Would you mind pulling in master? This needs to include the removal of the |
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.
Works great, diff'd the arguments passed to Kibana in this PR with master and the only things changed are the order of the --no-base-path
and --optimize.bundleDir
arguments passed in most configs, and the removal of some instances of --server.port
, --elasticsearch.url
and --optimize.enabled
which were being overridden by subsequent flags anyway. I do have a couple notes on the code, but LGTM after you pull master and update the panel_actions config.
* overridden options | ||
*/ | ||
function filterCliArgs(args) { | ||
const argv = [...args].slice(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.
Out of curiosity, why ignore the first arg? When running from dev it will be the path to scripts/kibana
, but that shouldn't be a problem, and otherwise the first argument is a meaningful argument that should be considered, right?
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.
It's true. You're right, except that in the case that it's running from an install-path, that first argument isn't the bin/kibana
command. That gets tacked on when we check whether we're on Windows (append the .bat
) or not. So.. it's a bit confusing. Ultimately I think you're right, I got to simplify this method by removing that slice. Thanks for noticing!
function filterCliArgs(args) { | ||
const argv = [...args].slice(1); | ||
|
||
const filteredArgv = argv.reduce((acc, val, ind) => { |
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 think this would be a little easier to follow if it was using an object where the keys are unique representations of each arg (unifying the basePath args), and the values were the last version found for each arg. Then this could just return the Object.values()
of that object rather than doing so much traversal of the args, and having to keep track of things like index.
💚 Build Succeeded |
retest |
💔 Build Failed |
failed on |
retest |
💚 Build Succeeded |
You have one r+ already and it should generally be enough. Feel free to request review if you still need additional review though. |
@azasypkin can you just check that the ssl tests are sufficient? |
Sure, looking..... |
Okay, I think it's a good start, thanks for adding these tests! If we need more tests in the future we'll add them in separate PRs. |
6.x/6.5: 8e696b7 |
* [Tests] Add http integration test setup * Base path tests * SSL tests * Eslint fixes * Remove env from config schema * Rename folders so no_rewrite and rewrite match configs/tests * wip * Use self-signed cert for SSL test * Improve basepath tests * Run base path proxy server in dev mode for now * Remove env from x-pack reporting config * Remove redundant base-path tests * Test SSL with redirectHttpFromPort set * Test SSL with redirectHttpFromPort set * Flesh out comments * Remove some cruft * Add SSL tests to CI run
* [Tests] Add http integration test setup * Base path tests * SSL tests * Eslint fixes * Remove env from config schema * Rename folders so no_rewrite and rewrite match configs/tests * wip * Use self-signed cert for SSL test * Improve basepath tests * Run base path proxy server in dev mode for now * Remove env from x-pack reporting config * Remove redundant base-path tests * Test SSL with redirectHttpFromPort set * Test SSL with redirectHttpFromPort set * Flesh out comments * Remove some cruft * Add SSL tests to CI run
* [Tests] Add http integration test setup * Base path tests * SSL tests * Eslint fixes * Remove env from config schema * Rename folders so no_rewrite and rewrite match configs/tests * wip * Use self-signed cert for SSL test * Improve basepath tests * Run base path proxy server in dev mode for now * Remove env from x-pack reporting config * Remove redundant base-path tests * Test SSL with redirectHttpFromPort set * Test SSL with redirectHttpFromPort set * Flesh out comments * Remove some cruft * Add SSL tests to CI run
Notes:
This PR also attempts to improve how we start the Kibana test server. We currently allow all command line options to get passed. This was okay because options that show up later in the list would override the previous options. It doesn't work for the test server when using
basePath
because--no-base-path
was overriding any--server.basePath
setting previously set by the user. Now we try to remove overridden options before starting the test server. See comment. 😃Run
Todo: