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

[8.15] [Http] Gracefully onboarding internal routes to VersionedRouter (#194789) #194905

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

jloleysens
Copy link
Contributor

Backport

This will backport the following commits from main to 8.15:

Questions ?

Please refer to the Backport tool documentation

…astic#194789)

## Summary

If an internal route goes from unversioned -> versioned today this will
surface as a breaking change. This PR allows internal routes to
gracefully onboard to the versioned router.

The fix is to default to version `1` of an internal route if no version
is provided. In this way old clients can continue to interact with the
new servers as developers onboard to the versioned router and roll out
v2+ of an internal route.

## Notes

We could use `buildNr` to narrow down internal routes defaulting to v1
to older clients only. IMO this would be more accurate, but also of
marginal value. My rationale is: by requiring explicit versions during
dev time the risk of us shipping new builds that don't provide a version
is effectively nullified. Additionally we already run this risk with our
public route default behaviour (we even require that public routes have
explicit version in dev to mitigate this same risk for existing public
clients).

If reviewers feel otherwise I'm happy to revisit this!

### Checklist

Delete any items that are not applicable to this PR.

- [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

### Risk Matrix

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| By defaulting internal routes to v1 it's possible that behaviour
becomes less predictable. | Low | Low | This is already true for public
routes and we are allowing a more limited/more predictable version of
that here. |

(cherry picked from commit afecfd8)

# Conflicts:
#	packages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.test.ts
#	packages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts
Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

LGTM I suppose Ale's question applies here too

@jloleysens
Copy link
Contributor Author

@gsoldevila I responded, let me know what you think!

@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

@jloleysens jloleysens merged commit 21c2326 into elastic:8.15 Oct 7, 2024
21 checks passed
@jloleysens jloleysens deleted the backport/8.15/pr-194789 branch October 9, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants