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

Turn on internal API restriction for serverless tests #162636

Merged
merged 9 commits into from
Aug 1, 2023

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jul 27, 2023

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

@jloleysens jloleysens added Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.10.0 labels Jul 27, 2023
@jloleysens jloleysens requested review from a team as code owners July 27, 2023 13:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@jloleysens jloleysens requested a review from a team as a code owner July 27, 2023 14:53
@jeramysoucy
Copy link
Contributor

@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.

@jloleysens jloleysens requested a review from a team as a code owner July 28, 2023 11:18
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jul 28, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

Copy link
Contributor

@kc13greiner kc13greiner left a 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`,
Copy link
Contributor Author

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

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a 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())
Copy link
Contributor

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

Copy link
Contributor

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())
Copy link
Contributor

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')
Copy link
Contributor

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');
Copy link
Contributor

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')
Copy link
Contributor

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');
Copy link
Contributor

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

@dmlemeshko dmlemeshko left a 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

Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala left a 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

@jloleysens jloleysens merged commit 87ff936 into elastic:main Aug 1, 2023
2 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 1, 2023
@jloleysens jloleysens deleted the serverless-e2e-test branch August 1, 2023 08:20
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:build-serverless-image Feature:http release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.