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.

Sign up for GitHub

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

Jump to bottom

[Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix #80742

Merged
merged 6 commits into from
Oct 16, 2020
Merged

[Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix #80742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f1197b
remove -SNAPSHOT from kibana version
neptunian Oct 15, 2020
7a7dfb7
add integration tests with -SNAPSHOT version of kibana
neptunian Oct 16, 2020
e4cf535
update isAgentUpgradeable to compare version numbers only
neptunian Oct 16, 2020
6f244ba
continue to send the kibana version with snapshot suffix to agent
neptunian Oct 16, 2020
a2fefd3
cleanup code into one function
neptunian Oct 16, 2020
aaf4e29
fix test to check for snapshot before adding suffix
neptunian Oct 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions x-pack/plugins/ingest_manager/common/services/is_agent_upgradeable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,35 @@ describe('Ingest Manager - isAgentUpgradeable', () => {
true
);
});
it('returns false if agent reports upgradeable, with agent snapshot version === kibana version', () => {
expect(
isAgentUpgradeable(getAgent({ version: '7.9.0-SNAPSHOT', upgradeable: true }), '7.9.0')
Copy link

Choose a reason for hiding this comment

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

are you also checking the option where agent reports

version: 7.9.0
upgradeable: true
snapshot: true

Copy link
Contributor Author

@neptunian neptunian Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean checking whether or not it is a snapshot? It does not test for snapshot because it does not matter if its a snapshot whether or not its upgradeable (unless we decide to filter out snapshots). But I can add the option anyway as part of the tests. Let me know if I am misunderstanding or mistaken with the logic.

).toBe(false);
});
it('returns false if agent reports upgradeable, with agent version === kibana snapshot version', () => {
expect(
isAgentUpgradeable(getAgent({ version: '7.9.0', upgradeable: true }), '7.9.0-SNAPSHOT')
).toBe(false);
});
it('returns true if agent reports upgradeable, with agent snapshot version < kibana snapshot version', () => {
expect(
isAgentUpgradeable(
getAgent({ version: '7.9.0-SNAPSHOT', upgradeable: true }),
'8.0.0-SNAPSHOT'
)
).toBe(true);
});
it('returns false if agent reports upgradeable, with agent snapshot version === kibana snapshot version', () => {
expect(
isAgentUpgradeable(
Copy link

Choose a reason for hiding this comment

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

i'm wondering whether this should be upgradable.
e.g you're running snapshot 8.0.0 but want to update to todays snapshot

Copy link
Contributor Author

@neptunian neptunian Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

in the UI we only show upgradeable if the version is less than the kibana version, regardless of whether its a snapshot. Also the agent version should never have the '-SNAPSHOT' suffix in it (it does not currently), I just put it in case that were to change. It currently only reports the number in the metadata.

If we want to make an exception where if an agent is a snapshot that the versions can be equal and still upgradeable, I would need to check the agent local metadata to see whether its a snapshot and consider it upgradeable when versions are the same. I am fine with doing that, but not sure how common this case would be. I'd rather add it as an enhancement in the next version. I am not sure this makes much sense to have at the moment because if we are always using whether or not Kibana is on a snapshot as the version, we shouldn't be checking whether or not the agent is on a snapshot. For example, agent is a snapshot but kibana is not, we check that Agent is a snapshot and let it upgrade even though its the same version as kibana, then we provide a stable version download url because kibana is not a snapshot, and so they do not even download the snapshot anyway. Unless we want to do a check that agent and kibana are both snapshots.

getAgent({ version: '8.0.0-SNAPSHOT', upgradeable: true }),
'8.0.0-SNAPSHOT'
)
).toBe(false);
});
it('returns true if agent reports upgradeable, with agent version < kibana snapshot version', () => {
expect(
isAgentUpgradeable(getAgent({ version: '7.9.0', upgradeable: true }), '8.0.0-SNAPSHOT')
).toBe(true);
});
});
11 changes: 7 additions & 4 deletions x-pack/plugins/ingest_manager/common/services/is_agent_upgradeable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@ export function isAgentUpgradeable(agent: Agent, kibanaVersion: string) {
return false;
}
if (agent.unenrollment_started_at || agent.unenrolled_at) return false;
const kibanaVersionParsed = semver.parse(kibanaVersion);
const agentVersionParsed = semver.parse(agentVersion);
if (!agentVersionParsed || !kibanaVersionParsed) return false;
if (!agent.local_metadata.elastic.agent.upgradeable) return false;
return semver.lt(agentVersionParsed, kibanaVersionParsed);

// make sure versions are only the number before comparison
const agentVersionNumber = semver.coerce(agentVersion);
if (!agentVersionNumber) throw new Error('agent version is invalid');
const kibanaVersionNumber = semver.coerce(kibanaVersion);
if (!kibanaVersionNumber) throw new Error('kibana version is invalid');
return semver.lt(agentVersionNumber, kibanaVersionNumber);
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I think this is doing the same not sure which approach we should favorise https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/services/agents/checkin/state_new_actions.ts/#L127-L137

Copy link
Contributor Author

@neptunian neptunian Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@nchaulet The thing that makes me a bit paranoid is semver.lt('7.9.0-SNAPSHOT', '7.9.0', { includePrerelease: true }); or semver.lt('7.9.0-SNAPSHOT', '7.9.0', { includePrerelease: false }); evaluates to being true, and in our case because a version is a snapshot, it shouldn't be considered less than when the versions are the same but one is a snapshot. Is that not the case in your example?

}
32 changes: 24 additions & 8 deletions x-pack/plugins/ingest_manager/server/routes/agent/upgrade_handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { RequestHandler } from 'src/core/server';
import { TypeOf } from '@kbn/config-schema';
import semver from 'semver';
import {
AgentSOAttributes,
PostAgentUpgradeResponse,
Expand All @@ -26,17 +27,18 @@ export const postAgentUpgradeHandler: RequestHandler<
> = async (context, request, response) => {
const soClient = context.core.savedObjects.client;
const { version, source_uri: sourceUri } = request.body;

// temporarily only allow upgrading to the same version as the installed kibana version
const kibanaVersion = appContextService.getKibanaVersion();
if (kibanaVersion !== version) {
try {
checkVersionIsSame(version, kibanaVersion);
} catch (err) {
return response.customError({
statusCode: 400,
body: {
message: `cannot upgrade agent to ${version} because it is different than the installed kibana version ${kibanaVersion}`,
message: err.message,
},
});
}

const agentSO = await soClient.get<AgentSOAttributes>(
AGENT_SAVED_OBJECT_TYPE,
request.params.agentId
Expand Down Expand Up @@ -82,14 +84,14 @@ export const postBulkAgentsUpgradeHandler: RequestHandler<
> = async (context, request, response) => {
const soClient = context.core.savedObjects.client;
const { version, source_uri: sourceUri, agents } = request.body;

// temporarily only allow upgrading to the same version as the installed kibana version
const kibanaVersion = appContextService.getKibanaVersion();
if (kibanaVersion !== version) {
try {
checkVersionIsSame(version, kibanaVersion);
} catch (err) {
return response.customError({
statusCode: 400,
body: {
message: `cannot upgrade agent to ${version} because it is different than the installed kibana version ${kibanaVersion}`,
message: err.message,
},
});
}
Expand All @@ -115,3 +117,17 @@ export const postBulkAgentsUpgradeHandler: RequestHandler<
return defaultIngestErrorHandler({ error, response });
}
};

export const checkVersionIsSame = (version: string, kibanaVersion: string) => {
// get version number only in case "-SNAPSHOT" is in it
const kibanaVersionNumber = semver.coerce(kibanaVersion)?.version;
if (!kibanaVersionNumber) throw new Error(`kibanaVersion ${kibanaVersionNumber} is not valid`);
const versionToUpgradeNumber = semver.coerce(version)?.version;
if (!versionToUpgradeNumber)
throw new Error(`version to upgrade ${versionToUpgradeNumber} is not valid`);
// temporarily only allow upgrading to the same version as the installed kibana version
if (kibanaVersionNumber !== versionToUpgradeNumber)
throw new Error(
`cannot upgrade agent to ${versionToUpgradeNumber} because it is different than the installed kibana version ${kibanaVersionNumber}`
);
};
41 changes: 41 additions & 0 deletions x-pack/test/ingest_manager_api_integration/apis/fleet/agents/upgrade.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ import { setupIngest } from './services';
import { skipIfNoDockerRegistry } from '../../../helpers';
import { AGENT_SAVED_OBJECT_TYPE } from '../../../../../plugins/ingest_manager/common';

const makeSnapshotVersion = (version: string) => {
return version.endsWith('-SNAPSHOT') ? version : `${version}-SNAPSHOT`;
};

export default function (providerContext: FtrProviderContext) {
const { getService } = providerContext;
const supertest = getService('supertest');
Expand Down Expand Up @@ -48,6 +52,43 @@ export default function (providerContext: FtrProviderContext) {
const res = await supertest.get(`/api/fleet/agents/agent1`).set('kbn-xsrf', 'xxx');
expect(typeof res.body.item.upgrade_started_at).to.be('string');
});
it('should respond 400 if upgrading agent with version the same as snapshot version', async () => {
const kibanaVersion = await kibanaServer.version.get();
const kibanaVersionSnapshot = makeSnapshotVersion(kibanaVersion);
await kibanaServer.savedObjects.update({
id: 'agent1',
type: AGENT_SAVED_OBJECT_TYPE,
attributes: {
local_metadata: { elastic: { agent: { upgradeable: true, version: kibanaVersion } } },
},
});
await supertest
.post(`/api/fleet/agents/agent1/upgrade`)
.set('kbn-xsrf', 'xxx')
.send({
version: kibanaVersionSnapshot,
})
.expect(400);
});
it('should respond 200 if upgrading agent with version less than kibana snapshot version', async () => {
const kibanaVersion = await kibanaServer.version.get();
const kibanaVersionSnapshot = makeSnapshotVersion(kibanaVersion);

await kibanaServer.savedObjects.update({
id: 'agent1',
type: AGENT_SAVED_OBJECT_TYPE,
attributes: {
local_metadata: { elastic: { agent: { upgradeable: true, version: '0.0.0' } } },
},
});
await supertest
.post(`/api/fleet/agents/agent1/upgrade`)
.set('kbn-xsrf', 'xxx')
.send({
version: kibanaVersionSnapshot,
})
.expect(200);
});
it('should respond 200 to upgrade agent and update the agent SO without source_uri', async () => {
const kibanaVersion = await kibanaServer.version.get();
await kibanaServer.savedObjects.update({
Expand Down