-
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
Turn on internal API restriction for serverless tests #162636
Conversation
Pinging @elastic/kibana-core (Team:Core) |
@jloleysens We just merged a fix for the security request headers tests. When you resolve conflicts you should be able to just use what's in main. |
Pinging @elastic/apm-ui (Team:APM) |
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!
@@ -33,6 +33,7 @@ export default async () => { | |||
}, | |||
sourceArgs: ['--no-base-path', '--env.name=development'], | |||
serverArgs: [ | |||
`--server.restrictInternalApis=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.
The most important change of the PR
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Most Fleet API's are intended to be public but the work is still pending. As it is right now, they're internal.
Ideally, we need Fleet's approval before going ahead with this rather than assuming they are.
@@ -16,7 +16,7 @@ export default function ({ getService }: FtrProviderContext) { | |||
it('rejects request to create a new fleet server hosts', async () => { | |||
const { body, status } = await supertest | |||
.post('/api/fleet/fleet_server_hosts') | |||
.set(svlCommonApi.getCommonRequestHeader()) | |||
.set(svlCommonApi.getInternalRequestHeader()) |
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.
the API's public, not internal. Fleet has an open issue to change to 'public' again: https://github.com/elastic/ingest-dev/issues/1921
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.
This means until the linked issue is resolved, this API would be treated as internal ?
@@ -34,7 +34,7 @@ export default function ({ getService }: FtrProviderContext) { | |||
it('rejects request to create a new proxy', async () => { | |||
const { body, status } = await supertest | |||
.post('/api/fleet/proxies') | |||
.set(svlCommonApi.getCommonRequestHeader()) | |||
.set(svlCommonApi.getInternalRequestHeader()) |
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.
public
@@ -54,6 +55,7 @@ export async function createRule({ | |||
const { body } = await supertest | |||
.post(`/api/alerting/rule`) | |||
.set('kbn-xsrf', 'foo') | |||
.set('x-elastic-internal-origin', 'foo') |
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.
👍 . confirmed with @elastic/actionable-observability, their APIs are 'internal'
const response = await supertest | ||
.get(`/api/alerting/rule/${id}`) | ||
.set('kbn-xsrf', 'foo') | ||
.set('x-elastic-internal-origin', 'foo'); |
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.
👍
@@ -21,6 +21,7 @@ export const createDataView = async ({ | |||
const { body } = await supertest | |||
.post(`/api/content_management/rpc/create`) | |||
.set('kbn-xsrf', 'foo') | |||
.set('x-elastic-internal-origin', 'foo') |
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.
👍
await supertest | ||
.delete(`/api/alerting/rule/${ruleId}`) | ||
.set('kbn-xsrf', 'foo') | ||
.set('x-elastic-internal-origin', 'foo'); |
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.
👍
await supertest | ||
.delete(`/api/actions/connector/${actionId}`) | ||
.set('kbn-xsrf', 'foo') | ||
.set('x-elastic-internal-origin', 'foo'); |
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.
👍
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 Code review only
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.
APM changes are good to go, only concern is with the Fleet API which @TinaHeiligers has already highlighted
## Summary Since we already have some E2E tests running for serverless, this PR turns on the internal API restriction flag to test whether our UI functions _as such_ under these tests. An alternative could be to have a specific smoke test for this, but it seems this is thoroughly covered by piggy-backing off the existing set of tests. Blocks: elastic#162149
Summary
Since we already have some E2E tests running for serverless, this PR turns on the internal API restriction flag to test whether our UI functions as such under these tests.
An alternative could be to have a specific smoke test for this, but it seems this is thoroughly covered by piggy-backing off the existing set of tests.
Blocks: #162149