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

Beats Management plugin: migrate server-side code #70930

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jul 7, 2020

Summary

Fix #67980

Checklist

@pgayvallet pgayvallet added chore Feature:Legacy Removal Issues related to removing legacy Kibana Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 7, 2020
Copy link
Contributor Author

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

The FTR test suite (x-pack/test/api_integration/apis/beats) is passing, unfortunately it doesn't cover all the endpoints.

Comment on lines -100 to +102
success: false,
error: { code: 400, message: 'Invalid enrollment token' },
statusCode: 400,
error: 'Bad Request',
message: 'Invalid enrollment token',
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 legacy plugin was using custom body payload for error responses, which is not possible with KP. The client-side code actually don't care about this format changes, as the body of an error response is never accessed.

Comment on lines +16 to +18
export function wrapRouteWithSecurity<P, Q, B>(
{
requiredLicense = [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted/adapted from x-pack/legacy/plugins/beats_management/server/lib/framework.ts#wrapRouteWithSecurity

Comment on lines +23 to +25
export class KibanaBackendFrameworkAdapter implements BackendFrameworkAdapter {
public readonly internalUser = internalUser;
public info: null | FrameworkInfo = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too many changes to keep file track, but this was x-pack/legacy/plugins/beats_management/server/lib/adapters/framework/kibana_framework_adapter.ts

Comment on lines 13 to 16
/*
import Joi from 'joi';
import { FrameworkRequest } from '../../lib/adapters/framework/adapter_types';
import { CMServerLibs } from '../../lib/types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the legacy route registration commented in the route files to ease the diff when reviewing. I will remove them all before merging

Comment on lines +115 to +116
// TODO: fixme eventually, need to access `info.remoteAddress` from KibanaRequest.
const legacyRequest = ensureRawRequest(request);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to access legacyRequest.info.remoteAddress, which is not exposed on KibanaRequest. I discussed with @restrry of the possibility to add it to core API. However as this plugin is the only one having this need and knowing that it will be removed for 8.0, I'm just using the private ensureRawRequest utility here.

I can implement the 'correct' way if that's preferable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Josh added one in #71019

Copy link
Contributor

Choose a reason for hiding this comment

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

Ended up implementing it differently, so that PR no longer adds this API. I'm fine with this implementation for now and if other plugins (that aren't about to get deleted) also need this, we can re-evaluate adding it then.

Comment on lines 21 to 23
headers: Joi.object({
'kbn-beats-access-token': Joi.string().required(),
}).options({ allowUnknown: 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.

This misleading kbn-beats-access-token token validation is actually doing nothing, as the object is not .required(). Also, the header is not used in the route handler, and neither the FTR test suite nor the client-side API are sending the header, which make me think this was just bad copypasta.

All that to say: the fact that this validation is not present on the KP handler is not a mistake.

Comment on lines +68 to +69
// TODO: write to Kibana audit log file
router.put(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just kept the TODO comments unchanged.

Comment on lines +67 to +71
body: schema.nullable(
schema.object({
num_tokens: schema.number({ defaultValue: DEFAULT_NUM_TOKENS, min: 1 }),
})
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL an empty payload is null, not undefined for hapi.

@pgayvallet pgayvallet marked this pull request as ready for review July 7, 2020 15:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor Author

@ph @justinkambic would you mind sparing some time to perform a quick user testing of the plugin's functionalities?

@pgayvallet pgayvallet requested review from a team, justinkambic and ph July 7, 2020 15:49
@ph
Copy link
Contributor

ph commented Jul 8, 2020

This look targetting for 7.10 so I will give it a shot after FF? is that okay with you?/

@pgayvallet
Copy link
Contributor Author

@ph yea, it's planned for 7.10, so after FF is perfectly fine. Thanks a lot.

@pgayvallet pgayvallet mentioned this pull request Jul 23, 2020
14 tasks
@pgayvallet
Copy link
Contributor Author

@ph @justinkambic do you think you'll have the capacity to take a look in the next few weeks?

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Review summary

I primarily reviewed the front-end changes, which are more-or-less an update to the imports to account for file migration. Nothing stood out to me on that account. I did look over server code as well, everything seems to be working as I'd expect, and the code migration largely made sense.

I attempted to utilize each of this plugin's routes while doing my functional review as well.

Functional summary

My functional review consisted of:

  • Enroll a beat
  • Create a tag
    • Create config block for Filebeat to tail a log
    • Create an output config block for ES access
  • Attach a tag to a beat
  • Create and attach multiple tags to beats
  • Remove tag from beat
  • Run Filebeat and ingest data, ensure my logs are indexed
  • Delete a tag
  • Unenroll the beat

image

Bugs

  • I did notice some weirdness when it came to deleting tags. It seems that when a tag is selected and deleted from the list, all the other tags are dropped from the list except the one deleted. Re-fetching of the available tags does show that the deleted one is gone and the rest remain, but it's definitely an unsettling UX. I'm not sure if this is how the table already behaves in master.

Aug-11-2020 10-33-30

  • A similar behavior when unenrolling beats. I unenrolled my Filebeat and the UI didn't reflect the change appropriately. After refresh the list displayed no items, like I would expect. Again, this bug might already be in master and not a side effect of this change.

Aug-11-2020 11-06-26

docs/developer/architecture/code-exploration.asciidoc Outdated Show resolved Hide resolved
@pgayvallet
Copy link
Contributor Author

@justinkambic thanks a lot of the review. Regarding the refresh bugs, I just checked master, and can confirm they are already present on our master branch. Looking at the client-side migration PR from @joshdover , it looks like it was even present before we migrated the client-side code, as the action handlers for the various table did not really change. So I guess this is 'alright'.

I can open a follow-up issue to track this if we feel like it's necessary.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Reviewed the key integration points & all route code. Wasn't able to find inconsistencies with the old routes that could create issues. Of course there's plenty of opportunity for improvement in this plugin, but not worth the effort since it will be sunset soon. Integration with Core APIs LGTM.

Do you mind go ahead and renaming this plugin's id to beatsManagement so we can do away with that deprecation log?

Comment on lines +44 to +50
if (user.kind !== 'authenticated') {
return response.forbidden({
body: {
message: `Request must be authenticated`,
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not even sure this is necessary since this is the default behavior? Maybe not worth changing the original implementation though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, honestly I tried to reduce the changes as much as possible, so the 'security' handler wrapper may have some redundancy with core's defaults, but I feel like it's not worth doing any additional changes.

@justinkambic
Copy link
Contributor

I can open a follow-up issue to track this if we feel like it's necessary.

I think this is a question more for the Ingest team if they want someone to fix it. Given that we know these issues exist, perhaps it would be a good idea to track them in an issue and make Ingest aware of them.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
beats_management - -307 307
beatsManagement 307 +307 -
total -0

async chunks size

id value diff baseline
beats_management - -409.3KB 409.3KB
beatsManagement 409.3KB ⚠️ +409.3KB -
total -4.0B

page load bundle size

id value diff baseline
beats_management - -581.6KB 581.6KB
beatsManagement 581.6KB ⚠️ +581.6KB -
total -8.0B

History

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

@pgayvallet pgayvallet merged commit 890fc31 into elastic:master Aug 18, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Aug 18, 2020
* move all the legacy codebase to KP plugin

* fix imports

* add empty handler context & rename routes folder

* start migrating routes

* migrate rest of the routes

* migrate adapters

* remove beats from legacy xpack plugin

* use nullable + replace response.custom

* fix wrapRouteWithSecurity, remove incorrect header validation

* remove comment

* updating generated plugin list

* fix typos in doc

* adapt readme too.

* remove old commented routes from route files

* remove eslint disabling

* use camel case for plugin id

* update generated doc

Co-authored-by: Josh Dover <[email protected]>
# Conflicts:
#	x-pack/legacy/plugins/beats_management/server/rest_api/beats/configuration.ts
#	x-pack/legacy/plugins/beats_management/server/rest_api/configurations/upsert.ts
pgayvallet added a commit that referenced this pull request Aug 18, 2020
* move all the legacy codebase to KP plugin

* fix imports

* add empty handler context & rename routes folder

* start migrating routes

* migrate rest of the routes

* migrate adapters

* remove beats from legacy xpack plugin

* use nullable + replace response.custom

* fix wrapRouteWithSecurity, remove incorrect header validation

* remove comment

* updating generated plugin list

* fix typos in doc

* adapt readme too.

* remove old commented routes from route files

* remove eslint disabling

* use camel case for plugin id

* update generated doc

Co-authored-by: Josh Dover <[email protected]>
# Conflicts:
#	x-pack/legacy/plugins/beats_management/server/rest_api/beats/configuration.ts
#	x-pack/legacy/plugins/beats_management/server/rest_api/configurations/upsert.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Beats Management server-side
7 participants