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] Fleet server cannot be removed from a managed agent policy #89617

Closed
mostlyjason opened this issue Jan 28, 2021 · 19 comments
Closed

[Fleet] Fleet server cannot be removed from a managed agent policy #89617

mostlyjason opened this issue Jan 28, 2021 · 19 comments
Assignees
Labels
Feature:Fleet Fleet team's agent central management project Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@mostlyjason
Copy link
Contributor

Hosted Elastic Agents need connectivity through Fleet in order to be managed. This means that Fleet server is required for these agents and it should not be disabled. To enforce this, we need to prevent users from removing or disabling the Fleet server integration policy in managed agent policies. When a user attempts to remove or disable the Fleet server integration policy, we should show an error message indicating the request failed and explain why. This should be implemented in the API layer to enforce it for both the API and UI.

@mostlyjason mostlyjason added Feature:Fleet Fleet team's agent central management project Team:Fleet Team label for Observability Data Collection Fleet team labels Jan 28, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Feature:Fleet)

@elasticmachine
Copy link
Contributor

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

@ph
Copy link
Contributor

ph commented Jan 28, 2021

@ruflin I am copying you point from the linked issue:

We should indeed have a limitation in place to not be able to remove the fleet-server integration at least, maybe both. @blakerouse I wonder if in general we need this for the fleet-server integration that the user should never be able to remove it from the policy as long as there are Agents with fleet-server enrolled.

I am looking at the AC defined by Jason and its not clear to me: To enforce this, we need to prevent users from removing or disabling the Fleet server integration policy in managed agent policies.

  • What the implementation would look like?
  • Where this limit is defined, it is in the spec or somewhere else?
  • Can we generalize that "concept"?

@ruflin
Copy link
Member

ruflin commented Jan 29, 2021

In a first phase, this is covered by #88688 I think. The user cannot add / remove any integrations from the policy and with this also not fleet-server integration.

In a more generic way, this issue is not related to a managed policy. Any policy can contain the fleet-server. I wonder if in a more generic way, we should tell the user to not disable a fleet-server integration as long as still Agents are connected to it or other integrations are part of the same policy. Are there other scenarios where a user should not remove the fleet-server integration?

@mostlyjason
Copy link
Contributor Author

The user cannot add / remove any integrations from the policy

@ruflin I don't see that restriction mentioned in the PR description or the linked issue. I thought we were planning to use the allowlist/blocklist to limit the integrations? This would not prevent someone from removing the fleet server integration though. I'd be hesitant to restrict adding/removing all integrations because its a heavy handed approach that will block the user from adding new integrations in later releases.

@ruflin
Copy link
Member

ruflin commented Feb 1, 2021

@mostlyjason Sorry, you are right it is not specified in the issue :-( @jfsiii Is my assumption correct that currently a user cannot add / remove any integration without using the force flag? Should we update the PR / issue?

The mid term plan is to use the allowlist / blocklist approach but it is simpler to start with just managed. Managed can be extended with the allowlist / blocklist concept in future releases to offer more flexibility. Even if there is a allowlist / blocklist, it will still be a managed policy.

@simitt
Copy link
Contributor

simitt commented Feb 1, 2021

The same should be true for the APM integration for managed policies - users should not be allowed to remove them. But to generalize this more, at this stage, users should not be able to remove or add any integration of a managed agent policy. Isn't that very much related to #76841?

@jfsiii
Copy link
Contributor

jfsiii commented Feb 1, 2021

@ruflin

Is my assumption correct that currently a user cannot add / remove any integration without using the force flag?

No, it's not. At least I don't believe so. Nothing has changed regarding integrations or package policies.

Should we update the PR / issue?

Yes, please.

To make sure I understand correctly, we're saying a user should not be abled to add or delete an integration from a managed policy on this view (or the related APIs), correct?
Screen Shot 2021-02-01 at 7 36 25 AM

@simitt
Copy link
Contributor

simitt commented Feb 1, 2021

@jfsiii in my understanding fleet should also not allow to install an integration coming from the Integrations view (and according API). In the UI that means not showing any managed policy in the drop-down when navigating to the Integrations section:
Screenshot 2021-02-01 at 13 42 52

@jfsiii
Copy link
Contributor

jfsiii commented Feb 1, 2021

@simitt Yes, thanks for adding that!

I used that view to add an integration to a managed policy (which should not be allowed) before getting to the other view for adding/removing integrations.

@ruflin
Copy link
Member

ruflin commented Feb 1, 2021

@jfsiii Correct. Could you update the issue in case you agree? Also no need to add it to the current open PR, this can be a follow up.
@mostlyjason Thanks for checking it in details!

@mostlyjason
Copy link
Contributor Author

Just want to make sure the eng team is aware of the options here, you can decide how to proceed. One option is just to have a blanket restriction that integration policies cannot be added or removed from agent policies. It is simple, but we'll have to remove this restriction later, so its throwaway code. I'd propose not investing in UI and just giving API error messages if we plan to remove this code later.

Alternatively, the same result can be achieved by combining two existing restrictions and one new. However, these will be relevant long term.

  1. Users cannot add integrations to a managed agent policy. This is accomplished through an allow list containing only fleet server and apm server [Fleet] Show agent as Unhealthy if agent reports an error about incompatible input(s) #76841. When we are ready to add more integrations later, we just add them to the allow list.
  2. Have a requirement of max 1 integration policy per agent policy for fleet server and apm server, so they cannot add any more. This restriction already exists for endpoint. I imagine we'd want this long term since the ports are fixed?
  3. New Users cannot remove fleet server or apm server from a managed agent policy. This will allow future integrations (like AWS) to be removed. Is implementing this restriction more effort than a blanket restriction on all integration policies?

@simitt
Copy link
Contributor

simitt commented Feb 2, 2021

  1. Have a requirement of max 1 integration policy per agent policy for fleet server and apm server, so they cannot add any more. This restriction already exists for endpoint. I imagine we'd want this long term since the ports are fixed?

For APM Server there is an open issue elastic/apm-server#4539 to only allow one APM integration per agent policy.

@ph
Copy link
Contributor

ph commented Feb 2, 2021

@ruflin and @jfsiii Can you answer #89617 (comment) ? Please review the options and have a proposition.

@ruflin
Copy link
Member

ruflin commented Feb 3, 2021

My comment in #89617 (comment) still applies. No throw away code here. Mid term we will need both the managed feature and allow / blocklist, starting with managed only for now simplifies the implementation but is not limiting us long term. In both cases, the user will always have an option to overwrite it through a --force flag or similar.

I'll sync with @jfsiii to make sure we are on the same page here.

@jfsiii
Copy link
Contributor

jfsiii commented Feb 8, 2021

@simitt @mostlyjason @ph @ruflin I started a Draft PR at #90675

There are no tests yet but it should prevent any integrations being added or deleted from a managed policy

The UI merely forwards the API error instead of filtering the integration or policy from a list, but that's consistent with how we dealt with other actions involving managed policies

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

jfsiii pushed a commit that referenced this issue Feb 10, 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 #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 pushed a commit to jfsiii/kibana that referenced this issue 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 issue 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
Copy link
Contributor

jfsiii commented Feb 12, 2021

This merged with #90675

Fleet server cannot be removed from a managed policy because no integrations can be removed from a managed agent policies.

@ph @ruflin Not sure if we want to close this and open another for this work to make an allowlist or update it to reflect the new work to be done.

@ruflin
Copy link
Member

ruflin commented Feb 15, 2021

From my perspective we can close this and open a separate issue for a future allow / block list feature.

@ph
Copy link
Contributor

ph commented Feb 15, 2021

Agree with @ruflin comment above. We also discussed it in the sync.

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 Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

No branches or pull requests

6 participants