-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[server/rewriteBasePath] Support rewriting basePath requests #16724
Changes from all commits
3c07d08
6afdcd5
9053b5e
0a9a64c
ace596d
49e642c
03c12ae
d845c62
d168119
b93d1ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,12 +22,14 @@ export default class ClusterManager { | |
this.basePathProxy = new BasePathProxy(this, settings); | ||
|
||
optimizerArgv.push( | ||
`--server.basePath=${this.basePathProxy.basePath}` | ||
`--server.basePath=${this.basePathProxy.basePath}`, | ||
'--server.rewriteBasePath=true', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for trailing commas |
||
); | ||
|
||
serverArgv.push( | ||
`--server.port=${this.basePathProxy.targetPort}`, | ||
`--server.basePath=${this.basePathProxy.basePath}` | ||
`--server.basePath=${this.basePathProxy.basePath}`, | ||
'--server.rewriteBasePath=true', | ||
); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,13 +19,15 @@ describe('Config schema', function () { | |
|
||
describe('basePath', function () { | ||
it('accepts empty strings', function () { | ||
const { error } = validate({ server: { basePath: '' } }); | ||
const { error, value } = validate({ server: { basePath: '' } }); | ||
expect(error == null).to.be.ok(); | ||
expect(value.server.basePath).to.be(''); | ||
}); | ||
|
||
it('accepts strings with leading slashes', function () { | ||
const { error } = validate({ server: { basePath: '/path' } }); | ||
const { error, value } = validate({ server: { basePath: '/path' } }); | ||
expect(error == null).to.be.ok(); | ||
expect(value.server.basePath).to.be('/path'); | ||
}); | ||
|
||
it('rejects strings with trailing slashes', function () { | ||
|
@@ -40,6 +42,45 @@ describe('Config schema', function () { | |
expect(error.details[0]).to.have.property('path', 'server.basePath'); | ||
}); | ||
|
||
it('rejects things that are not strings', function () { | ||
for (const value of [1, true, {}, [], /foo/]) { | ||
const { error } = validate({ server: { basePath: value } }); | ||
expect(error).to.have.property('details'); | ||
expect(error.details[0]).to.have.property('path', 'server.basePath'); | ||
} | ||
}); | ||
}); | ||
|
||
describe('rewriteBasePath', function () { | ||
it('defaults to false', () => { | ||
const { error, value } = validate({}); | ||
expect(error).to.be(null); | ||
expect(value.server.rewriteBasePath).to.be(false); | ||
}); | ||
|
||
it('accepts false', function () { | ||
const { error, value } = validate({ server: { rewriteBasePath: false } }); | ||
expect(error).to.be(null); | ||
expect(value.server.rewriteBasePath).to.be(false); | ||
}); | ||
|
||
it('accepts true if basePath set', function () { | ||
const { error, value } = validate({ server: { basePath: '/foo', rewriteBasePath: true } }); | ||
expect(error).to.be(null); | ||
expect(value.server.rewriteBasePath).to.be(true); | ||
}); | ||
|
||
it('rejects true if basePath not set', function () { | ||
const { error } = validate({ server: { rewriteBasePath: true } }); | ||
expect(error).to.have.property('details'); | ||
expect(error.details[0]).to.have.property('path', 'server.rewriteBasePath'); | ||
}); | ||
|
||
it('rejects strings', function () { | ||
const { error } = validate({ server: { rewriteBasePath: 'foo' } }); | ||
expect(error).to.have.property('details'); | ||
expect(error.details[0]).to.have.property('path', 'server.rewriteBasePath'); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should there be a test case rejecting everything but strings for |
||
}); | ||
|
||
describe('ssl', function () { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,17 @@ const savedObjectsIndexCheckTimeout = (settings, log) => { | |
} | ||
}; | ||
|
||
const rewriteBasePath = (settings, log) => { | ||
if (_.has(settings, 'server.basePath') && !_.has(settings, 'server.rewriteBasePath')) { | ||
log( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is there any magic step (apart from defining deprecated keys in config obviously) I should make to see this warning? I don't see any deprecation warnings defined in this file for some reason :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, yeah code lgtm |
||
'You should set server.basePath along with server.rewriteBasePath. Starting in 7.0, Kibana ' + | ||
'will expect that all requests start with server.basePath rather than expecting you to rewrite ' + | ||
'the requests in your reverse proxy. Set server.rewriteBasePath to false to preserve the ' + | ||
'current behavior and silence this warning.' | ||
); | ||
} | ||
}; | ||
|
||
const deprecations = [ | ||
//server | ||
rename('server.ssl.cert', 'server.ssl.certificate'), | ||
|
@@ -37,6 +48,7 @@ const deprecations = [ | |
rename('optimize.lazyProxyTimeout', 'optimize.watchProxyTimeout'), | ||
serverSslEnabled, | ||
savedObjectsIndexCheckTimeout, | ||
rewriteBasePath, | ||
]; | ||
|
||
export const transformDeprecations = createTransform(deprecations); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,185 @@ | ||
import { Server } from 'hapi'; | ||
import expect from 'expect.js'; | ||
import sinon from 'sinon'; | ||
|
||
import { setupBasePathRewrite } from '../setup_base_path_rewrite'; | ||
|
||
describe('server / setup_base_path_rewrite', () => { | ||
function createServer({ basePath, rewriteBasePath }) { | ||
const config = { | ||
get: sinon.stub() | ||
}; | ||
|
||
config.get.withArgs('server.basePath') | ||
.returns(basePath); | ||
config.get.withArgs('server.rewriteBasePath') | ||
.returns(rewriteBasePath); | ||
|
||
const server = new Server(); | ||
server.connection({ port: 0 }); | ||
setupBasePathRewrite({}, server, config); | ||
|
||
server.route({ | ||
method: 'GET', | ||
path: '/', | ||
handler(req, reply) { | ||
reply('resp:/'); | ||
} | ||
}); | ||
|
||
server.route({ | ||
method: 'GET', | ||
path: '/foo', | ||
handler(req, reply) { | ||
reply('resp:/foo'); | ||
} | ||
}); | ||
|
||
return server; | ||
} | ||
|
||
describe('no base path', () => { | ||
let server; | ||
before(() => server = createServer({ basePath: '', rewriteBasePath: false })); | ||
after(() => server = undefined); | ||
|
||
it('/bar => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/bar/ => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar/' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/bar/foo => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar/foo' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/ => /', async () => { | ||
const resp = await server.inject({ | ||
url: '/' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/'); | ||
}); | ||
|
||
it('/foo => /foo', async () => { | ||
const resp = await server.inject({ | ||
url: '/foo' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/foo'); | ||
}); | ||
}); | ||
|
||
describe('base path /bar, rewrite = false', () => { | ||
let server; | ||
before(() => server = createServer({ basePath: '/bar', rewriteBasePath: false })); | ||
after(() => server = undefined); | ||
|
||
it('/bar => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/bar/ => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar/' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/bar/foo => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar/foo' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/ => /', async () => { | ||
const resp = await server.inject({ | ||
url: '/' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/'); | ||
}); | ||
|
||
it('/foo => /foo', async () => { | ||
const resp = await server.inject({ | ||
url: '/foo' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/foo'); | ||
}); | ||
}); | ||
|
||
describe('base path /bar, rewrite = true', () => { | ||
let server; | ||
before(() => server = createServer({ basePath: '/bar', rewriteBasePath: true })); | ||
after(() => server = undefined); | ||
|
||
it('/bar => /', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/'); | ||
}); | ||
|
||
it('/bar/ => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar/' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/'); | ||
}); | ||
|
||
it('/bar/foo => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/bar/foo' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(200); | ||
expect(resp.payload).to.be('resp:/foo'); | ||
}); | ||
|
||
it('/ => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
|
||
it('/foo => 404', async () => { | ||
const resp = await server.inject({ | ||
url: '/foo' | ||
}); | ||
|
||
expect(resp.statusCode).to.be(404); | ||
}); | ||
}); | ||
}); |
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.
++