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

[Fleet] Restrict integration changes for managed policies #90675

Merged

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Feb 8, 2021

Summary

  • Integrations cannot be added , unless with a force flag
    • API
    • UI
    • tests
  • Integrations cannot be removed , unless with a force flag
    • API
    • UI
    • tests

closes #90445
refs #89617

Cannot add integrations to managed policy

Screen Shot 2021-02-08 at 1 56 32 PM

Cannot delete integrations from managed policy

Screen Shot 2021-02-08 at 3 05 16 PM

Checklist

@jfsiii jfsiii added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Fleet Fleet team's agent central management project Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 labels Feb 8, 2021
@jfsiii jfsiii self-assigned this Feb 8, 2021
@jfsiii jfsiii marked this pull request as ready for review February 9, 2021 20:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Feature:Fleet)

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.

Should we verify in the delete method of the packagePolicy service too that the policy is not managed?

}
if (parentAgentPolicy.is_managed) {
Copy link
Member

Choose a reason for hiding this comment

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

(not blocking) I have the feeling that is time to refactor L64-L80 to a new function something like _validateParentAgentPolicy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wanted to consolidate the two sources of the "There is already a package named..." errors

if (
(parentAgentPolicy.package_policies as PackagePolicy[]).find(
(siblingPackagePolicy) =>
siblingPackagePolicy.id !== id && siblingPackagePolicy.name === packagePolicy.name
)
) {
throw new Error('There is already a package with the same name on this agent policy');
}

And perhaps give it a specific error name/type. I plan on circling back to that after I get the other features in.

I'll open a ticket

@@ -39,6 +39,52 @@ export default function ({ getService }: FtrProviderContext) {
.send({ agentPolicyId });
});

it('should fail for managed agent policies', async function () {
if (server.enabled) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick we have a helper that does the same thing but I found it less verbose https://github.com/elastic/kibana/blob/master/x-pack/test/fleet_api_integration/apis/epm/data_stream.ts/#L37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. I was just copy/pasting from the rest of the file. I'll update

@jfsiii
Copy link
Contributor Author

jfsiii commented Feb 9, 2021

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Feb 9, 2021

Should we verify in the delete method of the packagePolicy service too that the policy is not managed?

@nchaulet I put the check in the AgentPolicyService because Package Policy delete calls it

await agentPolicyService.unassignPackagePolicies(

And in the Package Policy service we only have the Agent Policy id string, not the whole object. Which we do have in the AgentPolicyService.

@simitt
Copy link
Contributor

simitt commented Feb 10, 2021

Integrations cannot be added , unless with a force flag
Integrations cannot be removed , unless with a force flag

Does that mean there is no possibility to force adding/removing an integration? @ruflin has a more concrete plan on the policy set up than I do, but so far I was under the impression that a force flag could be used for the initial set up of the managed policy.

@jfsiii
Copy link
Contributor Author

jfsiii commented Feb 10, 2021

@simitt you cannot current bypass the guards for managed policies. However, you can work around them by doing something like

  • Create a new policy (which aren't managed by default) or update existing policy to unmanaged
  • Add / delete integrations
  • Set policy to managed

I'll open an issue to discuss the force flag (we have it some other places already and it doesn't mean quite the same thing) but I think the "make it unmanaged" workaround means everything is still possible without adding API which we might be inconsistent or changed later.

@simitt
Copy link
Contributor

simitt commented Feb 10, 2021

thanks @jfsiii - the described workflow should work for now.

John Schulz added 2 commits February 10, 2021 14:22
…ithub.com:jfsiii/kibana into 90445-no-integration-changes-for-managed-policies
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

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.

🚀

@jfsiii jfsiii merged commit c92af5a into elastic:master Feb 10, 2021
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 11, 2021
* master: (44 commits)
  [APM] Add experimental support for Data Streams (elastic#89650)
  [Search Session] Control "Kibana / Search Sessions" management section by privileges (elastic#90818)
  [Lens] Median as default function (elastic#90952)
  Implement custom global header banner (elastic#87438)
  [Fleet] Reduce permissions. (elastic#90302)
  Update dependency @elastic/charts to v24.5.1 (elastic#89822)
  [Create index pattern] Can't create single character index without wildcard (elastic#90919)
  [ts/build_ts_refs] add support for --clean flag (elastic#91060)
  Don't clean when running e2e tests (elastic#91057)
  Fixes track_total_hits in the body not having an effect when using search strategy (elastic#91068)
  [Security Solution][Detections] Adds list plugin Saved Objects to Security feature privilege (elastic#90895)
  Removing the code plugin entirely for 8.0 (elastic#77940)
  chore(NA): move the instruction to remove yarn global bazelisk package into the first place on install bazel tools (elastic#91026)
  [jest/ci] remove max-old-space-size override to use 4gb default (elastic#91020)
  [Fleet] Restrict integration changes for managed policies (elastic#90675)
  [CI] Fix auto-backport condditions so that it doesn't trigger for other labels (elastic#91042)
  [DOCS] Uses variable to refer to query profiler (elastic#90976)
  [App Search] Relevance Tuning logic listeners (elastic#89461)
  [Metrics UI] Fix saving/loading saved views from URL (elastic#90216)
  Limit cardinality of transaction.name (elastic#90955)
  ...
jfsiii pushed a commit to jfsiii/kibana that referenced this pull request Feb 11, 2021
)

## Summary

- [x] Integrations cannot be added ~~, unless with a force flag~~
  - [x] API
  - [x] UI
  - [x] tests
- [x] Integrations cannot be removed ~~, unless with a force flag~~
  - [x] API
  - [x] UI
  - [x] tests

closes elastic#90445
refs elastic#89617

### Cannot add integrations to managed policy

<img height="400" alt="Screen Shot 2021-02-08 at 1 56 32 PM" src="https://user-images.githubusercontent.com/57655/107277261-25c48300-6a22-11eb-936a-0a7361667093.png">

### Cannot delete integrations from managed policy

<img  alt="Screen Shot 2021-02-08 at 3 05 16 PM" src="https://user-images.githubusercontent.com/57655/107277318-337a0880-6a22-11eb-836f-fc66b510d257.png">

### Checklist
- [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
jfsiii pushed a commit that referenced this pull request Feb 11, 2021
…91150)

## Summary

- [x] Integrations cannot be added ~~, unless with a force flag~~
  - [x] API
  - [x] UI
  - [x] tests
- [x] Integrations cannot be removed ~~, unless with a force flag~~
  - [x] API
  - [x] UI
  - [x] tests

closes #90445
refs #89617

### Cannot add integrations to managed policy

<img height="400" alt="Screen Shot 2021-02-08 at 1 56 32 PM" src="https://user-images.githubusercontent.com/57655/107277261-25c48300-6a22-11eb-936a-0a7361667093.png">

### Cannot delete integrations from managed policy

<img  alt="Screen Shot 2021-02-08 at 3 05 16 PM" src="https://user-images.githubusercontent.com/57655/107277318-337a0880-6a22-11eb-836f-fc66b510d257.png">

### Checklist
- [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
@jfsiii jfsiii deleted the 90445-no-integration-changes-for-managed-policies branch April 6, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Fleet Fleet team's agent central management project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Cannot add / remove integrations from managed policy
5 participants