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

Introduce Enroll API endpoint. #108835

Merged
merged 12 commits into from
Aug 19, 2021
Merged

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Aug 17, 2021

Summary

In this PR we introduce a new Enroll API endpoint for the Interactive Setup plugin.

Part of: #104068

@azasypkin azasypkin added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 17, 2021
Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for this! Couple comments below but generally happy with the approach.

@azasypkin
Copy link
Member Author

Looks great, thanks for this! Couple comments below but generally happy with the approach.

Thanks for the feedback, should be handled now.I'm moving to unit tests, will tag you once PR is ready for review.

Comment on lines +20 to +22
if (value.asSeconds() < 1) {
return 'the value must be greater or equal to 1 second.';
}
Copy link
Member Author

Choose a reason for hiding this comment

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

note: just like with minimum value for the session cleanup interval, we set minimum to 1 seconds to prevent our users from shooting themselves in the foot.

});
});

describe('#enroll()', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: quite a bit of "testing of internal implementation" here, but I feel like it's super important to not break anything in this area.

serviceAccountToken: { name: string; value: string };
}

export class ElasticsearchService {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: I finally moved this to a "Service" since it has a clear lifetime with appropriate setup/stop hooks.

Comment on lines +110 to +112
pingError instanceof errors.ConnectionError
? ElasticsearchConnectionStatus.NotConfigured
: ElasticsearchConnectionStatus.Configured
Copy link
Member Author

Choose a reason for hiding this comment

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

note: I am a bit cautious here and don't try to handle other potentially acceptable errors (e.g. Timeout), it's better to be 100% sure here that Kibana is really unable to connect to ES.

@@ -13,11 +13,18 @@ import type { CorePreboot, Logger, PluginInitializerContext, PrebootPlugin } fro

import { ElasticsearchConnectionStatus } from '../common';
Copy link
Member Author

Choose a reason for hiding this comment

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

note: didn't write tests for this class yet as I think it's too early, let's stabilize implementation a bit more.

@azasypkin
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@azasypkin azasypkin marked this pull request as ready for review August 18, 2021 15:54
@azasypkin azasypkin requested a review from a team as a code owner August 18, 2021 15:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

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

LGTM!

@azasypkin azasypkin added the backport:skip This commit does not require backporting label Aug 19, 2021
@azasypkin azasypkin merged commit cb0ce59 into elastic:master Aug 19, 2021
@azasypkin azasypkin deleted the issue-xxx-enroll-api branch August 19, 2021 09:24
@azasypkin azasypkin self-assigned this Aug 19, 2021
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 release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants