-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add support for /api/status before Kibana completes startup #79012
Conversation
a7e3af4
to
a0790a7
Compare
I haven't updated any unit tests yet, so this is sure to break some things right now. |
this.notReadyServer.route({ | ||
this.notReadyServer = new HttpServer(this.logger, 'NotReady'); | ||
const notReadySetup = await this.notReadyServer.setup(config); | ||
notReadySetup.server.route({ |
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.
Let's use newly added API instead of hapi one?
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.
I was going to do that, but since we don't support some of the HTTP methods right now, I left it like this so that it covers any request.
a0790a7
to
1fc08ef
Compare
49cde1f
to
567da20
Compare
@joshdover if you want to bring this back up-to-date with |
@legrego Sure thing, I should be able to get to this tomorrow, if that's alright. I'm also thinking that we may need to do some refactoring for #89287. For one, this should probably be called something different than the "notReadyServer", maybe "initializationServer"?. I also think we may need to formalize an API on the |
@elasticmachine merge upstream |
…pdate-not-ready-status
…as started listening
@joshdover I brought this PR back up to date with In short, there were three main reasons for the test failures:
|
Pinging @elastic/kibana-core (Team:Core) |
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.
Is there any expectation for supporting basic auth (from the browser) before startup completes? The login page is unavailable, and api/status will return a 401.
Have we put any thought into whether this is a breaking change, and documenting best practices for status monitoring? I'm okay with addressing this separately.
Changes LGTM otherwise.
@jbudz great question -- the NotReady server can't support basic auth at this point, unless we were to hardcode credentials into
I personally hadn't considered it, so thank you for raising this. Our docs only make a passing reference to That said, I don't know how other stack components consume this endpoint |
Yeah that would make some sense, though it doesn't feel great to have this sometimes available. We'd likely replace this with a more limited health check endpoint that's safe to be public at some point. (#46984). |
@elasticmachine merge upstream |
@mshustov would you mind reviewing one last time to get the requested codeowner approval from |
@elasticmachine merge upstream |
@legrego I don't know why GH didn't count my approval #79012 (review) (probably because I did it on the draft state). Approved again. |
💚 Build Succeeded
Metrics [docs]History
To update your PR or re-run it, just comment with: |
…79012) Co-authored-by: Larry Gregory <larry.gregory@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…98329) Co-authored-by: Larry Gregory <larry.gregory@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Josh Dover <1813008+joshdover@users.noreply.github.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co>
Summary
Checklist
Delete any items that are not applicable to this PR.
For maintainers