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

[Ingest Manager] Add namespace validation #75381

Merged
merged 10 commits into from
Aug 20, 2020

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Aug 18, 2020

Summary

Resolves #74667. This PR adds additional validation for namespace field on agent and package policies. As we use the namespace field in the last part of our indexing strategy, the validation method partially implements ES index name rules, omitting the irrelevant ones.

The validation is applied on our API routes using a custom validation schema, as well as UI:

image

image

Checklist

Delete any items that are not applicable to this PR.

@jen-huang jen-huang added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 Team:Fleet Team label for Observability Data Collection Fleet team v7.9.1 labels Aug 18, 2020
@jen-huang jen-huang self-assigned this Aug 18, 2020
@jen-huang jen-huang marked this pull request as ready for review August 18, 2020 23:43
@jen-huang jen-huang requested a review from a team August 18, 2020 23:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀

@jen-huang
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ingestManager 518 +1 517

async chunks size

id value diff baseline
ingestManager 1.1MB +582.0B 1.1MB

page load bundle size

id value diff baseline
ingestManager 466.4KB +507.0B 465.9KB

History

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

@jen-huang jen-huang merged commit 3201efe into elastic:master Aug 20, 2020
@jen-huang jen-huang deleted the ingest/bug/74667 branch August 20, 2020 22:30
jen-huang added a commit to jen-huang/kibana that referenced this pull request Aug 20, 2020
* Add namespace validation on APIs and UI

* Add test coverage

* Fix imports

* Fix schema

* Rename to policy

* Fix typo
jen-huang added a commit to jen-huang/kibana that referenced this pull request Aug 20, 2020
* Add namespace validation on APIs and UI

* Add test coverage

* Fix imports

* Fix schema

* Rename to policy

* Fix typo
# Conflicts:
#	x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_policy/components/agent_policy_form.tsx
#	x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_policy/create_package_policy_page/services/validate_package_policy.ts
#	x-pack/plugins/ingest_manager/public/applications/ingest_manager/services/index.ts
#	x-pack/plugins/ingest_manager/server/types/models/agent_policy.ts
#	x-pack/plugins/ingest_manager/server/types/models/package_policy.ts
#	x-pack/test/ingest_manager_api_integration/apis/agent_policy/agent_policy.ts
#	x-pack/test/ingest_manager_api_integration/apis/package_policy/create.ts
jen-huang added a commit that referenced this pull request Aug 21, 2020
* Add namespace validation on APIs and UI

* Add test coverage

* Fix imports

* Fix schema

* Rename to policy

* Fix typo
jen-huang added a commit that referenced this pull request Aug 21, 2020
* Add namespace validation on APIs and UI

* Add test coverage

* Fix imports

* Fix schema

* Rename to policy

* Fix typo
# Conflicts:
#	x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_policy/components/agent_policy_form.tsx
#	x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_policy/create_package_policy_page/services/validate_package_policy.ts
#	x-pack/plugins/ingest_manager/public/applications/ingest_manager/services/index.ts
#	x-pack/plugins/ingest_manager/server/types/models/agent_policy.ts
#	x-pack/plugins/ingest_manager/server/types/models/package_policy.ts
#	x-pack/test/ingest_manager_api_integration/apis/agent_policy/agent_policy.ts
#	x-pack/test/ingest_manager_api_integration/apis/package_policy/create.ts
thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Aug 21, 2020
* Add namespace validation on APIs and UI

* Add test coverage

* Fix imports

* Fix schema

* Rename to policy

* Fix typo
return (
typeof namespace === 'string' &&
// Lowercase only
namespace === namespace.toLowerCase() &&

Choose a reason for hiding this comment

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

just a few cases missing which agent validates

  • namespace is allowed to contain . but is not allowed to be equal to . or ..
  • namespace cannot be longer than 255 chars
  • namespace cannot start with -, _ or +

Copy link
Contributor

Choose a reason for hiding this comment

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

@michalpristas I think the validation is incorrect,

Cannot be longer than 255 bytes (note it is bytes, so multi-byte characters will count towards the 255 limit faster)

This mean, type + dataset + namespace <= 255 bytes.

Choose a reason for hiding this comment

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

what we do in agent is that we put 255 constraint on dataset, namespace and a product of concatenation, so eventaully we will fail when index > 255.
putting some not communicated constraint felt weird (such as 255/3 or some other math) so i used 255 on each part and result as well.
as namespace is created before configuration and can lead to different indexes based on dataset and type it would make sense to agree on some constraint which can be applied at the time of specifying namespace.

Choose a reason for hiding this comment

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

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 followed up in #75846 (comment).

@ghost
Copy link

ghost commented Aug 31, 2020

Hi @jen-huang /@EricDavisX

We have validated this ticket on latest 7.9.1-SNAPSHOT Kibana cloud environment and found it fixed.

Provide namespace value starting with capital letter in agent configuration or integration while adding and editing them.

Observations:

  1. Error message "Namespace contains invalid characters" is displayed on providing namespace starting with capital letter while creating or editing agent configuration.
  2. Error message "Namespace contains invalid characters" is displayed on providing namespace starting with capital letter while adding or editing integration present in agent configuration.

Screenshot:

  1. While creating or editing agent configuration :-
    image

  2. While creating or editing integration in agent configuration :-
    image

We have created and executed 04 testcases under Add namespace validation TestRun.

Testcases created:
https://elastic.testrail.io/index.php?/cases/view/27064
https://elastic.testrail.io/index.php?/cases/view/27066
https://elastic.testrail.io/index.php?/cases/view/27063
https://elastic.testrail.io/index.php?/cases/view/27065

Hence, we are closing the ticket.

@EricDavisX
Copy link
Contributor

Thanks Rahul

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.1 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ingest Manager] [BUG] Using namespace with capital letters causes agent to not install beats/endpoint
7 participants