Skip to content

Commit

Permalink
[Fleet] fixed bug in output secret diff logic (#172081)
Browse files Browse the repository at this point in the history
## Summary

Related to #104986

Found a bug in `diffOutputSecretPaths` where output secret was deleted
if updating an output without change of service_token. Added unit tests
to cover the logic.

Steps to verify:
- enable feature flags: `xpack.fleet.enableExperimental:
['remoteESOutput', 'outputSecretsStorage']`
- create a remote es output with a service_token
- check that the service_token is stored as secret in `.fleet-secrets`
- update host in remote es output
- verify that the secret is not deleted


### 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
  • Loading branch information
juliaElastic authored Nov 29, 2023
1 parent 6bdc094 commit 2ff6a7c
Show file tree
Hide file tree
Showing 3 changed files with 193 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ export function useOutputForm(onSucess: () => void, output?: Output) {
is_default: false,
is_default_monitoring: defaultMonitoringOutputInput.value,
config_yaml: additionalYamlConfigInput.value,
service_token: serviceTokenInput.value,
service_token: serviceTokenInput.value || undefined,
...(!serviceTokenInput.value &&
serviceTokenSecretInput.value && {
secrets: {
Expand Down
188 changes: 186 additions & 2 deletions x-pack/plugins/fleet/server/services/secrets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ import { createAppContextStartContractMock } from '../mocks';
import type { NewPackagePolicy, PackageInfo } from '../types';

import { appContextService } from './app_context';

import { getPolicySecretPaths, diffSecretPaths, extractAndWriteSecrets } from './secrets';
import {
getPolicySecretPaths,
diffSecretPaths,
diffOutputSecretPaths,
extractAndWriteSecrets,
} from './secrets';

describe('secrets', () => {
let mockContract: ReturnType<typeof createAppContextStartContractMock>;
Expand Down Expand Up @@ -916,3 +920,183 @@ describe('secrets', () => {
});
});
});

describe('diffOutputSecretPaths', () => {
it('should return empty array if no secrets', () => {
expect(diffOutputSecretPaths([], [])).toEqual({
toCreate: [],
toDelete: [],
noChange: [],
});
});
it('should return empty array if single secret not changed', () => {
const paths = [
{
path: 'somepath',
value: {
id: 'secret-1',
},
},
];
expect(diffOutputSecretPaths(paths, paths)).toEqual({
toCreate: [],
toDelete: [],
noChange: paths,
});
});
it('should return empty array if multiple secrets not changed', () => {
const paths = [
{
path: 'somepath',
value: {
id: 'secret-1',
},
},
{
path: 'somepath2',
value: {
id: 'secret-2',
},
},
{
path: 'somepath3',
value: {
id: 'secret-3',
},
},
];

expect(diffOutputSecretPaths(paths, paths.slice().reverse())).toEqual({
toCreate: [],
toDelete: [],
noChange: paths,
});
});
it('single secret modified', () => {
const paths1 = [
{
path: 'somepath1',
value: {
id: 'secret-1',
},
},
{
path: 'somepath2',
value: {
id: 'secret-2',
},
},
];

const paths2 = [
paths1[0],
{
path: 'somepath2',
value: 'newvalue',
},
];

expect(diffOutputSecretPaths(paths1, paths2)).toEqual({
toCreate: [
{
path: 'somepath2',
value: 'newvalue',
},
],
toDelete: [
{
path: 'somepath2',
value: {
id: 'secret-2',
},
},
],
noChange: [paths1[0]],
});
});
it('double secret modified', () => {
const paths1 = [
{
path: 'somepath1',
value: {
id: 'secret-1',
},
},
{
path: 'somepath2',
value: {
id: 'secret-2',
},
},
];

const paths2 = [
{
path: 'somepath1',
value: 'newvalue1',
},
{
path: 'somepath2',
value: 'newvalue2',
},
];

expect(diffOutputSecretPaths(paths1, paths2)).toEqual({
toCreate: [
{
path: 'somepath1',
value: 'newvalue1',
},
{
path: 'somepath2',
value: 'newvalue2',
},
],
toDelete: [
{
path: 'somepath1',
value: {
id: 'secret-1',
},
},
{
path: 'somepath2',
value: {
id: 'secret-2',
},
},
],
noChange: [],
});
});

it('single secret added', () => {
const paths1 = [
{
path: 'somepath1',
value: {
id: 'secret-1',
},
},
];

const paths2 = [
paths1[0],
{
path: 'somepath2',
value: 'newvalue',
},
];

expect(diffOutputSecretPaths(paths1, paths2)).toEqual({
toCreate: [
{
path: 'somepath2',
value: 'newvalue',
},
],
toDelete: [],
noChange: [paths1[0]],
});
});
});
10 changes: 6 additions & 4 deletions x-pack/plugins/fleet/server/services/secrets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -545,10 +545,12 @@ export function diffOutputSecretPaths(
const newPath = newPathsByPath[oldPath.path];
if (newPath && newPath.value) {
const newValue = newPath.value;
if (typeof newValue === 'string') toCreate.push(newPath);
toDelete.push(oldPath);
} else {
noChange.push(newPath);
if (typeof newValue === 'string') {
toCreate.push(newPath);
toDelete.push(oldPath);
} else {
noChange.push(newPath);
}
}
delete newPathsByPath[oldPath.path];
}
Expand Down

0 comments on commit 2ff6a7c

Please sign in to comment.