Skip to content

[Http] Added version header to unversioned, public routes#195464

Merged
jloleysens merged 9 commits intoelastic:mainfrom
jloleysens:http/http-api-version-header-in-responses
Oct 9, 2024
Merged

[Http] Added version header to unversioned, public routes#195464
jloleysens merged 9 commits intoelastic:mainfrom
jloleysens:http/http-api-version-header-in-responses

Conversation

@jloleysens
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens commented Oct 8, 2024

Summary

Close #190735

All unversioned, public routes, like versioned routes, will now include the following header in responses:

elastic-api-version: 2023-10-31

...when:

(1) The route produces a response from the handler (any status code)
(2) The route validation failed (400 bad request)

The header will not be included if we encounter a 500 or encounter some issue before reaching the router.

How to test

At time of PR /api/stats is not versioned, but public so useful to test with:

❯ curl -v -uelastic:changeme http://localhost:5601/api/stats
...
< HTTP/1.1 200 OK
< elastic-api-version: 2023-10-31
...

Checklist

@jloleysens jloleysens added Feature:http Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes v9.0.0 v8.16.0 backport:version Backport to applied version labels labels Oct 8, 2024
@jloleysens jloleysens self-assigned this Oct 8, 2024
@jloleysens jloleysens requested a review from a team as a code owner October 8, 2024 16:09
@elasticmachine
Copy link
Copy Markdown
Contributor

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

header: jest.fn().mockImplementation(() => mockResponse),
};
const mockResponseToolkit: any = {
} as unknown as jest.Mocked<ResponseObject>;
Copy link
Copy Markdown
Contributor Author

@jloleysens jloleysens Oct 8, 2024

Choose a reason for hiding this comment

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

Slightly better than any, at least we can use a typed object in these tests

Comment thread packages/core/http/core-http-router-server-internal/src/router.ts
Comment on lines +138 to +140
).resolves.toMatchObject({
message: expect.stringMatching(/Invalid version/),
});
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced use of toEqual(expect.objectContaining({... for more terse toMatchObject({... API

@elasticmachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #66 / dashboard Export import saved objects between versions should render all panels on the dashboard
  • [job] [logs] FTR Configs #25 / ObservabilityApp o11y sidenav sidenav & breadcrumbs renders the correct nav and navigate to links

Metrics [docs]

✅ unchanged

cc @jloleysens

const [first, second] = mockResponse.header.mock.calls
.concat()
.sort(([k1], [k2]) => k1.localeCompare(k2));
expect(first).toEqual(['AAAA', 'test']);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AAAAAAAA

Copy link
Copy Markdown
Contributor

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM

@jloleysens jloleysens merged commit 61251bf into elastic:main Oct 9, 2024
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11259229795

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 9, 2024
) (#195646)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Http] Added version header to unversioned, public routes
(#195464)](#195464)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jean-Louis
Leysens","email":"jeanlouis.leysens@elastic.co"},"sourceCommit":{"committedDate":"2024-10-09T16:33:54Z","message":"[Http]
Added version header to unversioned, public routes
(#195464)","sha":"61251bfdaffdb621558fff96d78cc8b2260c0abe","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:http","Team:Core","release_note:skip","v9.0.0","v8.16.0","backport:version"],"title":"[Http]
Added version header to unversioned, public
routes","number":195464,"url":"https://github.com/elastic/kibana/pull/195464","mergeCommit":{"message":"[Http]
Added version header to unversioned, public routes
(#195464)","sha":"61251bfdaffdb621558fff96d78cc8b2260c0abe"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195464","number":195464,"mergeCommit":{"message":"[Http]
Added version header to unversioned, public routes
(#195464)","sha":"61251bfdaffdb621558fff96d78cc8b2260c0abe"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Jean-Louis Leysens <jeanlouis.leysens@elastic.co>
jloleysens added a commit that referenced this pull request Oct 15, 2024
…#195940)

## Summary

Follow up on #195464

Adds public route registrar option `httpResource` to distinguish API
routes from routes intended to be used for loading resources, [for
ex](https://github.com/elastic/kibana/blob/bd22f1370fc55179ea6f2737176570176f700b6e/x-pack/plugins/security/server/routes/authentication/oidc.ts#L36).

This enables us to avoid returning the version header
`elastic-api-version` for HTTP resource routes.

It's still possible for API authors to use the versioned router for
things that should be HTTP resources, but it's assumed that all routes
registered through HTTP resources services are:

1. Public
2. Not versioned (focus of this PR)
3. Not documented (done in
#192675)

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 15, 2024
…elastic#195940)

## Summary

Follow up on elastic#195464

Adds public route registrar option `httpResource` to distinguish API
routes from routes intended to be used for loading resources, [for
ex](https://github.com/elastic/kibana/blob/bd22f1370fc55179ea6f2737176570176f700b6e/x-pack/plugins/security/server/routes/authentication/oidc.ts#L36).

This enables us to avoid returning the version header
`elastic-api-version` for HTTP resource routes.

It's still possible for API authors to use the versioned router for
things that should be HTTP resources, but it's assumed that all routes
registered through HTTP resources services are:

1. Public
2. Not versioned (focus of this PR)
3. Not documented (done in
elastic#192675)

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 72f3d2d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:http release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.16.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Not all HTTP APIs returning elastic-api-version headers in serverless

4 participants