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] support force flag to add/remove package_policies #96713

Merged
merged 4 commits into from
Apr 12, 2021

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Apr 10, 2021

Summary

Can now pass a force=true parameter to add & remove integrations on hosted policies as originally intended [1] & [2]

  • Add force param for POST /api/fleet/package_policies & /api/fleet/package_policies/delete to a policy. Update tests to confirm
  • Not strictly required, but "while I was in there"
    • Updated a few places to throw IngestManagerError vs Error for 400 response vs 500. Updated tests.
    • removed a few unnecessary awaits of sync function

[1] #92426 (comment)
[2] #90445

Checklist

@jfsiii jfsiii self-assigned this Apr 10, 2021
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Apr 10, 2021
@@ -79,18 +79,20 @@ export const createPackagePolicyHandler: RequestHandler<
> = async (context, request, response) => {
const soClient = context.core.savedObjects.client;
const esClient = context.core.elasticsearch.client.asCurrentUser;
const user = (await appContextService.getSecurity()?.authc.getCurrentUser(request)) || undefined;
const user = appContextService.getSecurity()?.authc.getCurrentUser(request) || undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

saw this in VS code:
Screen Shot 2021-04-11 at 3 50 19 PM

Everything I found also indicates getCurrentUser is sync

getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null;
&
const getCurrentUser = (request: KibanaRequest) =>
http.auth.get<AuthenticatedUser>(request).state ?? null;

version: '0.13.0',
},
})
.expect(400);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now expects 400 vs 500

version: '0.1.0',
},
})
.expect(400);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now expects 400 vs 500

} else {
warnAndSkipTest(this, log);
}
it('should return a 400 if there is another package policy with the same name', async function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now expects 400 vs 500

namespace: 'default',
is_managed: true,
});
it('can only add to managed agent policies using the force parameter', async function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test the new behavior (force bypasses hosted policy restriction)

@@ -47,230 +46,229 @@ export default function ({ getService }: FtrProviderContext) {
.send({ agentPolicyId });
});

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

Choose a reason for hiding this comment

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

Most of the changes here are whitespace only from replacing the if (server.enabled) { ... } else { ... } pattern with skipIfNoDockerRegistry.

@jfsiii jfsiii marked this pull request as ready for review April 11, 2021 20:08
@jfsiii jfsiii requested a review from a team as a code owner April 11, 2021 20:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jfsiii
Copy link
Contributor Author

jfsiii commented Apr 11, 2021

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Apr 12, 2021

cc @simitt this means the "make it unmanaged first" workaround from #90675 (comment) is no longer required

@mdelapenya
Copy link
Contributor

/run-fleet-e2e-tests

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.

Did not run it locally but code looks good and if the api integration test works 🚀

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @jfsiii

@jfsiii jfsiii added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 12, 2021
@jfsiii jfsiii merged commit cb3c4e3 into elastic:master Apr 12, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 12, 2021
)

## Summary

Can now pass a `force=true` parameter to add & remove integrations on hosted policies as originally intended [1] & [2]

 * Add `force` param for `POST` `/api/fleet/package_policies` & `/api/fleet/package_policies/delete` to a policy. Update tests to confirm
 * Not strictly required, but "while I was in there"
   * Updated a few places to throw `IngestManagerError` vs `Error` for `400` response vs `500`. Updated tests.
   * removed a few unnecessary `await`s of sync function

[1] elastic#92426 (comment)
[2] elastic#90445

### 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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 12, 2021
…96868)

## Summary

Can now pass a `force=true` parameter to add & remove integrations on hosted policies as originally intended [1] & [2]

 * Add `force` param for `POST` `/api/fleet/package_policies` & `/api/fleet/package_policies/delete` to a policy. Update tests to confirm
 * Not strictly required, but "while I was in there"
   * Updated a few places to throw `IngestManagerError` vs `Error` for `400` response vs `500`. Updated tests.
   * removed a few unnecessary `await`s of sync function

[1] #92426 (comment)
[2] #90445

### 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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: John Schulz <john.schulz@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants