From b3ae98ed8f10706def9c40505eabeb1bdc8ea40d Mon Sep 17 00:00:00 2001 From: Kyle Pollich Date: Mon, 16 May 2022 09:19:41 -0400 Subject: [PATCH] [Fleet] Fix missing package-level vars in GCP policy editor (#132068) * Fix missing package-level vars in GCP policy editor Display package-level variables even if they aren't defined on the existing package policy during an upgrade, honoring default values for all variables if they exist. Fixes #131251 * Fix not persisting default values for new variables * Fix tests * Address PR feedback + update tests (cherry picked from commit 5fff510009b627ef5824a68cae3bfc8fc0d02e47) # Conflicts: # x-pack/plugins/fleet/common/services/validate_package_policy.test.ts # x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx --- .../services/validate_package_policy.test.ts | 62 +++++ .../services/validate_package_policy.ts | 1 + .../step_define_package_policy.test.tsx | 228 ++++++++++++++++++ .../step_define_package_policy.tsx | 22 +- 4 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx diff --git a/x-pack/plugins/fleet/common/services/validate_package_policy.test.ts b/x-pack/plugins/fleet/common/services/validate_package_policy.test.ts index e4566bacdae54..00aef893d98ef 100644 --- a/x-pack/plugins/fleet/common/services/validate_package_policy.test.ts +++ b/x-pack/plugins/fleet/common/services/validate_package_policy.test.ts @@ -241,6 +241,7 @@ describe('Fleet - validatePackagePolicy()', () => { ], }, ], + vars: {}, }; const invalidPackagePolicy: NewPackagePolicy = { @@ -332,6 +333,7 @@ describe('Fleet - validatePackagePolicy()', () => { ], }, ], + vars: {}, }; const noErrorsValidationResults = { @@ -370,6 +372,7 @@ describe('Fleet - validatePackagePolicy()', () => { vars: { 'var-name': null }, }, }, + vars: {}, }; it('returns no errors for valid package policy', () => { @@ -416,6 +419,7 @@ describe('Fleet - validatePackagePolicy()', () => { streams: { 'with-no-stream-vars-bar': {} }, }, }, + vars: {}, }); }); @@ -487,6 +491,7 @@ describe('Fleet - validatePackagePolicy()', () => { streams: { 'with-no-stream-vars-bar': {} }, }, }, + vars: {}, }); }); @@ -505,6 +510,7 @@ describe('Fleet - validatePackagePolicy()', () => { description: null, namespace: null, inputs: null, + vars: {}, }); expect( validatePackagePolicy( @@ -520,6 +526,7 @@ describe('Fleet - validatePackagePolicy()', () => { description: null, namespace: null, inputs: null, + vars: {}, }); }); @@ -538,6 +545,7 @@ describe('Fleet - validatePackagePolicy()', () => { description: null, namespace: null, inputs: null, + vars: {}, }); expect( validatePackagePolicy( @@ -553,6 +561,59 @@ describe('Fleet - validatePackagePolicy()', () => { description: null, namespace: null, inputs: null, + vars: {}, + }); + }); + + it('returns no errors when required field is present but empty', () => { + expect( + validatePackagePolicy( + { + ...validPackagePolicy, + inputs: [ + { + type: 'foo', + policy_template: 'pkgPolicy1', + enabled: true, + vars: { + 'foo-input-var-name': { value: '', type: 'text' }, + 'foo-input2-var-name': { value: '', type: 'text' }, + 'foo-input3-var-name': { value: ['test'], type: 'text' }, + }, + streams: [ + { + data_stream: { dataset: 'foo', type: 'logs' }, + enabled: true, + vars: { 'var-name': { value: 'test_yaml: value', type: 'yaml' } }, + }, + ], + }, + ], + }, + mockPackage, + safeLoad + ) + ).toEqual({ + name: null, + description: null, + namespace: null, + inputs: { + foo: { + streams: { + foo: { + vars: { + 'var-name': null, + }, + }, + }, + vars: { + 'foo-input-var-name': null, + 'foo-input2-var-name': null, + 'foo-input3-var-name': null, + }, + }, + }, + vars: {}, }); }); @@ -678,6 +739,7 @@ describe('Fleet - validatePackagePolicy()', () => { }, }, }, + vars: {}, name: null, namespace: null, }); diff --git a/x-pack/plugins/fleet/common/services/validate_package_policy.ts b/x-pack/plugins/fleet/common/services/validate_package_policy.ts index 3c0c0e5e92116..9e1db9ec286e7 100644 --- a/x-pack/plugins/fleet/common/services/validate_package_policy.ts +++ b/x-pack/plugins/fleet/common/services/validate_package_policy.ts @@ -55,6 +55,7 @@ export const validatePackagePolicy = ( description: null, namespace: null, inputs: {}, + vars: {}, }; const namespaceValidation = isValidNamespace(packagePolicy.namespace); diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx new file mode 100644 index 0000000000000..64fa3ad96fed3 --- /dev/null +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.test.tsx @@ -0,0 +1,228 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from 'react'; +import { act, fireEvent, waitFor } from '@testing-library/react'; + +import type { TestRenderer } from '../../../../../mock'; +import { createFleetTestRendererMock } from '../../../../../mock'; +import type { AgentPolicy, NewPackagePolicy, PackageInfo } from '../../../types'; + +import { useGetPackagePolicies } from '../../../hooks'; + +import { StepDefinePackagePolicy } from './step_define_package_policy'; + +jest.mock('../../../hooks', () => { + return { + ...jest.requireActual('../../../hooks'), + useGetPackagePolicies: jest.fn().mockReturnValue({ + data: { + items: [{ name: 'nginx-1' }, { name: 'other-policy' }], + }, + isLoading: false, + }), + useFleetStatus: jest.fn().mockReturnValue({ isReady: true } as any), + sendGetStatus: jest + .fn() + .mockResolvedValue({ data: { isReady: true, missing_requirements: [] } }), + }; +}); + +describe('StepDefinePackagePolicy', () => { + const packageInfo: PackageInfo = { + name: 'apache', + version: '1.0.0', + description: '', + format_version: '', + release: 'ga', + owner: { github: '' }, + title: 'Apache', + latestVersion: '', + assets: {} as any, + status: 'not_installed', + vars: [ + { + show_user: true, + name: 'Show user var', + type: 'string', + default: 'showUserVarVal', + }, + { + required: true, + name: 'Required var', + type: 'bool', + }, + { + name: 'Advanced var', + type: 'bool', + default: true, + }, + ], + }; + const agentPolicy: AgentPolicy = { + id: 'agent-policy-1', + namespace: 'ns', + name: 'Agent policy 1', + is_managed: false, + status: 'active', + updated_at: '', + updated_by: '', + revision: 1, + package_policies: [], + }; + let packagePolicy: NewPackagePolicy; + const mockUpdatePackagePolicy = jest.fn().mockImplementation((val: any) => { + packagePolicy = { + ...val, + ...packagePolicy, + }; + }); + + const validationResults = { + name: null, + description: null, + namespace: null, + inputs: {}, + vars: {}, + }; + + let testRenderer: TestRenderer; + let renderResult: ReturnType; + const render = ({ isUpdate } = { isUpdate: false }) => + (renderResult = testRenderer.render( + + )); + + beforeEach(() => { + packagePolicy = { + name: '', + description: 'desc', + namespace: 'default', + policy_id: '', + enabled: true, + output_id: '', + inputs: [], + }; + testRenderer = createFleetTestRendererMock(); + }); + + describe('default API response', () => { + beforeEach(() => { + render(); + }); + + it('should set index 1 name to package policy on init if no package policies exist for this package', () => { + waitFor(() => { + expect(renderResult.getByDisplayValue('apache-1')).toBeInTheDocument(); + expect(renderResult.getByDisplayValue('desc')).toBeInTheDocument(); + }); + + expect(mockUpdatePackagePolicy.mock.calls[0]).toEqual([ + { + description: 'desc', + enabled: true, + inputs: [], + name: 'apache-1', + namespace: 'default', + policy_id: 'agent-policy-1', + output_id: '', + package: { + name: 'apache', + title: 'Apache', + version: '1.0.0', + }, + vars: { + 'Advanced var': { + type: 'bool', + value: true, + }, + 'Required var': { + type: 'bool', + value: undefined, + }, + 'Show user var': { + type: 'string', + value: 'showUserVarVal', + }, + }, + }, + ]); + expect(mockUpdatePackagePolicy.mock.calls[1]).toEqual([ + { + namespace: 'ns', + policy_id: 'agent-policy-1', + }, + ]); + }); + + it('should display vars coming from package policy', async () => { + waitFor(() => { + expect(renderResult.getByDisplayValue('showUserVarVal')).toBeInTheDocument(); + expect(renderResult.getByRole('switch')).toHaveAttribute('aria-label', 'Required var'); + expect(renderResult.getByText('Required var is required')).toHaveAttribute( + 'class', + 'euiFormErrorText' + ); + }); + + await act(async () => { + fireEvent.click(renderResult.getByText('Advanced options').closest('button')!); + }); + + waitFor(() => { + expect(renderResult.getByRole('switch')).toHaveAttribute('aria-label', 'Advanced var'); + }); + }); + }); + + it('should set incremented name if other package policies exist', () => { + (useGetPackagePolicies as jest.MockedFunction).mockReturnValueOnce({ + data: { + items: [ + { name: 'apache-1' }, + { name: 'apache-2' }, + { name: 'apache-9' }, + { name: 'apache-10' }, + ], + }, + isLoading: false, + }); + + render(); + + waitFor(() => { + expect(renderResult.getByDisplayValue('apache-11')).toBeInTheDocument(); + }); + }); + + describe('update', () => { + describe('when package vars are introduced in a new package version', () => { + it('should display new package vars', () => { + render({ isUpdate: true }); + + waitFor(async () => { + expect(renderResult.getByDisplayValue('showUserVarVal')).toBeInTheDocument(); + expect(renderResult.getByText('Required var')).toBeInTheDocument(); + + await act(async () => { + fireEvent.click(renderResult.getByText('Advanced options').closest('button')!); + }); + + expect(renderResult.getByText('Advanced var')).toBeInTheDocument(); + }); + }); + }); + }); +}); diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx index 435d7d7b244fa..43133301b32cb 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx @@ -90,9 +90,28 @@ export const StepDefinePackagePolicy: React.FunctionComponent<{ // Update package policy's package and agent policy info useEffect(() => { - if (isUpdate || isLoadingPackagePolicies) { + if (isLoadingPackagePolicies) { return; } + + if (isUpdate) { + // If we're upgrading, we need to make sure we catch an addition of package-level + // vars when they were previously no package-level vars defined + if (!packagePolicy.vars && packageInfo.vars) { + updatePackagePolicy( + packageToPackagePolicy( + packageInfo, + agentPolicy?.id || '', + packagePolicy.output_id, + packagePolicy.namespace, + packagePolicy.name, + packagePolicy.description, + integrationToEnable + ) + ); + } + } + const pkg = packagePolicy.package; const currentPkgKey = pkg ? pkgKeyFromPackageInfo(pkg) : ''; const pkgKey = pkgKeyFromPackageInfo(packageInfo); @@ -224,6 +243,7 @@ export const StepDefinePackagePolicy: React.FunctionComponent<{ const { name: varName, type: varType } = varDef; if (!packagePolicy.vars || !packagePolicy.vars[varName]) return null; const value = packagePolicy.vars[varName].value; + return (