Skip to content

Commit

Permalink
[Profiling] checking viewer resources in the admin check (elastic#164086
Browse files Browse the repository at this point in the history
)

We identified a bug in the set up check where for admin users we should
also include the checks that were made in the step above.

We also decided to revert the changes in the package names.

---------

Co-authored-by: Francesco Gualazzi <inge4pres@users.noreply.github.com>
  • Loading branch information
cauemarcondes and inge4pres authored Aug 22, 2023
1 parent 826633c commit 2e854d6
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 8 deletions.
54 changes: 54 additions & 0 deletions x-pack/plugins/profiling/common/setup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ describe('Merging partial state operations', () => {
const defaultSetupState = createDefaultSetupState();
it('returns false when permission is not configured', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(true),
createResourceState({ enabled: true, created: true }),
createSettingsState(true),
createPermissionState(false),
Expand All @@ -92,6 +95,9 @@ describe('Merging partial state operations', () => {

it('returns false when resource management is not enabled', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(true),
createResourceState({ enabled: false, created: true }),
createSettingsState(true),
createPermissionState(true),
Expand All @@ -102,6 +108,9 @@ describe('Merging partial state operations', () => {

it('returns false when resources are not created', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(true),
createResourceState({ enabled: true, created: false }),
createSettingsState(true),
createPermissionState(true),
Expand All @@ -112,6 +121,9 @@ describe('Merging partial state operations', () => {

it('returns false when settings are not configured', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(true),
createResourceState({ enabled: true, created: true }),
createSettingsState(false),
createPermissionState(true),
Expand All @@ -122,13 +134,55 @@ describe('Merging partial state operations', () => {

it('returns true when all checks are valid', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(false),
createResourceState({ enabled: true, created: true }),
createSettingsState(true),
createPermissionState(true),
]);

expect(areResourcesSetupForAdmin(mergedState)).toBeTruthy();
});

it('returns false when collector is not found', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(false),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(false),
createResourceState({ enabled: true, created: true }),
createSettingsState(true),
createPermissionState(true),
]);

expect(areResourcesSetupForAdmin(mergedState)).toBeFalsy();
});

it('returns false when symbolizer is not found', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(false),
createProfilingInApmPolicyState(false),
createResourceState({ enabled: true, created: true }),
createSettingsState(true),
createPermissionState(true),
]);

expect(areResourcesSetupForAdmin(mergedState)).toBeFalsy();
});

it('returns false when profiling is in APM server', () => {
const mergedState = mergePartialSetupStates(defaultSetupState, [
createCollectorPolicyState(true),
createSymbolizerPolicyState(true),
createProfilingInApmPolicyState(true),
createResourceState({ enabled: true, created: true }),
createSettingsState(true),
createPermissionState(true),
]);

expect(areResourcesSetupForAdmin(mergedState)).toBeFalsy();
});
});

describe('For viewer users', () => {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/profiling/common/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export function areResourcesSetupForViewer(state: SetupState): boolean {

export function areResourcesSetupForAdmin(state: SetupState): boolean {
return (
areResourcesSetupForViewer(state) &&
state.resource_management.enabled &&
state.resources.created &&
state.permissions.configured &&
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/profiling/server/lib/setup/fleet_policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { ELASTIC_CLOUD_APM_POLICY, getApmPolicy } from './get_apm_policy';
import { ProfilingSetupOptions } from './types';

const CLOUD_AGENT_POLICY_ID = 'policy-elastic-agent-on-cloud';
const COLLECTOR_PACKAGE_POLICY_NAME = 'Universal Profiling Collector';
const SYMBOLIZER_PACKAGE_POLICY_NAME = 'Universal Profiling Symbolizer';
const COLLECTOR_PACKAGE_POLICY_NAME = 'elastic-universal-profiling-collector';
const SYMBOLIZER_PACKAGE_POLICY_NAME = 'elastic-universal-profiling-symbolizer';

async function getPackagePolicy({
soClient,
Expand Down Expand Up @@ -103,7 +103,7 @@ export async function createCollectorPackagePolicy({
enabled: true,
package: {
name: packageName,
title: COLLECTOR_PACKAGE_POLICY_NAME,
title: 'Universal Profiling Collector',
version,
},
name: COLLECTOR_PACKAGE_POLICY_NAME,
Expand Down Expand Up @@ -161,7 +161,7 @@ export async function createSymbolizerPackagePolicy({
enabled: true,
package: {
name: packageName,
title: SYMBOLIZER_PACKAGE_POLICY_NAME,
title: 'Universal Profiling Symbolizer',
version,
},
name: SYMBOLIZER_PACKAGE_POLICY_NAME,
Expand Down
5 changes: 1 addition & 4 deletions x-pack/plugins/profiling/server/routes/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,7 @@ export function registerSetupRoute({
* because of users with viewer privileges
* cannot get the cluster settings
*/
if (
areResourcesSetupForViewer(mergedStateForViewer) &&
mergedStateForViewer.data.available
) {
if (areResourcesSetupForViewer(mergedStateForViewer)) {
return response.ok({
body: {
has_setup: true,
Expand Down

0 comments on commit 2e854d6

Please sign in to comment.