update: engine_version in semver representation #2838

loresuso · 2023-09-26T10:29:23Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:
This PR convert the falco engine_version to semver. It does so while trying to deal with possible breaking changes with rules. In particular, if required_engine_version yaml field in rule is

numeric, it is converted into a semver string by appending and prepending major and patch number (0.x.0)
a string, it will be handled as a semver string

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update: engine_version in semver representation

jasondellaluce

Looks great! Left some comments. Also, not strong on this but we may want to check that

falco/.github/workflows/ci.yml

Line 100 in cca1d70

    
                     base_engine_ver=$(grep ENGINE_VERSION "./userspace/engine/falco_engine_version.h" | awk '{print $3}' | sed -e 's/(//g' -e 's/)//g')

works as expected.

userspace/engine/falco_engine_version.h

userspace/engine/rule_loader_collector.cpp

userspace/falco/grpc_server_impl.cpp

Signed-off-by: Lorenzo Susini <[email protected]>

… able to load rules with both numeric and semver string required_engine_version Signed-off-by: Lorenzo Susini <[email protected]>

…ns_info Signed-off-by: Lorenzo Susini <[email protected]>

…nability Signed-off-by: Lorenzo Susini <[email protected]>

Signed-off-by: Lorenzo Susini <[email protected]>

jasondellaluce · 2023-09-27T15:35:44Z

/milestone 0.37.0

poiana · 2023-09-27T15:38:50Z

LGTM label has been added.

Git tree hash: 449e969acb49f3b7c9f24d28311a8428611e2641

Signed-off-by: Lorenzo Susini <[email protected]>

loresuso · 2023-09-27T16:14:04Z

Updated the checksum with the last commit

poiana · 2023-09-27T17:22:32Z

LGTM label has been added.

Git tree hash: 9dc3bd714eb7c4ef601c7a15555ee93fa7bd2112

loresuso · 2023-09-28T14:23:39Z

Unfortunately CI Build / test-dev-packages / test-packages is failing. I'm investigating the issue

jasondellaluce

/approve

poiana · 2023-09-28T17:15:20Z

LGTM label has been added.

Git tree hash: 9dc3bd714eb7c4ef601c7a15555ee93fa7bd2112

incertum

/approve

poiana · 2023-09-28T18:04:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum, jasondellaluce, loresuso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

incertum · 2023-09-28T18:13:54Z

Great work!!! 🎉

Andreagit97 · 2023-09-29T08:00:12Z

userspace/falco/version.proto

@@ -45,6 +45,9 @@ message response
  string prerelease = 5;
  string build = 6;
 // falco engine version
-  uint32 engine_version = 7;
+  uint32 engine_minor = 7;


why did we change the proto here? I've seen that we keep engine_version as a field but we moved it to 11, maybe we can keep it here, WDYT?

Andreagit97 · 2023-09-29T09:21:49Z

userspace/falco/grpc_server_impl.cpp

@@ -79,6 +80,10 @@ void falco::grpc::server_impl::version(const context& ctx, const version::reques

 	res.set_engine_version(FALCO_ENGINE_VERSION);
 	res.set_engine_fields_checksum(FALCO_ENGINE_CHECKSUM); 
+	auto engine_version = falco_engine::engine_version();
+	res.set_engine_major(engine_version.major());


i would remove the above line

res.set_engine_version(FALCO_ENGINE_VERSION);

and use

res.set_engine_version(engine_version.as_string());

poiana added do-not-merge/work-in-progress kind/feature dco-signoff: yes area/engine do-not-merge/release-note-label-needed labels Sep 26, 2023

poiana requested review from Kaizhe and mstemm September 26, 2023 10:29

poiana added size/M release-note and removed do-not-merge/release-note-label-needed labels Sep 26, 2023

jasondellaluce reviewed Sep 26, 2023

View reviewed changes

userspace/engine/falco_engine_version.h Outdated Show resolved Hide resolved

userspace/engine/rule_loader_collector.cpp Outdated Show resolved Hide resolved

userspace/falco/grpc_server_impl.cpp Outdated Show resolved Hide resolved

loresuso mentioned this pull request Sep 27, 2023

new(userspace/libsinsp): implement versions operator <,>,== falcosecurity/libs#1370

Merged

loresuso force-pushed the update/engine-version-semver branch from e4f0306 to 7dcc2fb Compare September 27, 2023 10:45

poiana added size/L and removed size/M labels Sep 27, 2023

loresuso force-pushed the update/engine-version-semver branch 3 times, most recently from 1a7c330 to 106e822 Compare September 27, 2023 12:46

loresuso mentioned this pull request Sep 27, 2023

update: check semver engine version and related errors falcosecurity/testing#29

Merged

loresuso added 4 commits September 27, 2023 15:11

update(userspace/engine): convert engine version to semver string

e3827d0

Signed-off-by: Lorenzo Susini <[email protected]>

update(userspace/engine): let the rule loader reader and collector be…

d4ea0fd

… able to load rules with both numeric and semver string required_engine_version Signed-off-by: Lorenzo Susini <[email protected]>

update(userspace/falco): engine version semver in protobuf and versio…

7d2f2ec

…ns_info Signed-off-by: Lorenzo Susini <[email protected]>

update(userspace/engine): address jasondellaluce comments for maintai…

419b2ab

…nability Signed-off-by: Lorenzo Susini <[email protected]>

loresuso force-pushed the update/engine-version-semver branch from 6326958 to 86c20cf Compare September 27, 2023 15:12

update(cmake): bump libs version to add the new sinsp_version class impl

0b6b432

Signed-off-by: Lorenzo Susini <[email protected]>

loresuso force-pushed the update/engine-version-semver branch from 86c20cf to 0b6b432 Compare September 27, 2023 15:14

loresuso changed the title ~~wip: update: engine_version in semver representation~~ update: engine_version in semver representation Sep 27, 2023

poiana removed the do-not-merge/work-in-progress label Sep 27, 2023

update(submodule): update testing submodule

a5a5207

Signed-off-by: Lorenzo Susini <[email protected]>

poiana added the approved label Sep 27, 2023

update(userspace/engine): update falco engine checksum

f3ae2c4

Signed-off-by: Lorenzo Susini <[email protected]>

loresuso dismissed jasondellaluce’s stale review via f3ae2c4 September 27, 2023 16:10

poiana removed the lgtm label Sep 27, 2023

poiana requested a review from jasondellaluce September 27, 2023 16:10

jasondellaluce previously approved these changes Sep 27, 2023

View reviewed changes

poiana added the lgtm label Sep 27, 2023

loresuso mentioned this pull request Sep 28, 2023

update(rules): convert required engine version to new semver represen… falcosecurity/rules#178

Merged

loresuso dismissed jasondellaluce’s stale review via cee5c42 September 28, 2023 09:52

poiana removed the lgtm label Sep 28, 2023

poiana requested a review from jasondellaluce September 28, 2023 09:52

loresuso mentioned this pull request Sep 28, 2023

wip: update(internal/follower): handle required engine version mismatch wi… falcosecurity/falcoctl#329

Closed

loresuso changed the title ~~update: engine_version in semver representation~~ wip: update: engine_version in semver representation Sep 28, 2023

poiana added the do-not-merge/work-in-progress label Sep 28, 2023

loresuso force-pushed the update/engine-version-semver branch from cee5c42 to f3ae2c4 Compare September 28, 2023 15:08

loresuso changed the title ~~wip: update: engine_version in semver representation~~ update: engine_version in semver representation Sep 28, 2023

poiana removed the do-not-merge/work-in-progress label Sep 28, 2023

jasondellaluce approved these changes Sep 28, 2023

View reviewed changes

poiana added the lgtm label Sep 28, 2023

incertum approved these changes Sep 28, 2023

View reviewed changes

poiana assigned incertum Sep 28, 2023

poiana merged commit 09b1f92 into falcosecurity:master Sep 28, 2023

Andreagit97 reviewed Sep 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update: engine_version in semver representation #2838

update: engine_version in semver representation #2838

loresuso commented Sep 26, 2023 •

edited

Loading

jasondellaluce left a comment

jasondellaluce commented Sep 27, 2023

poiana commented Sep 27, 2023

loresuso commented Sep 27, 2023

poiana commented Sep 27, 2023

loresuso commented Sep 28, 2023

jasondellaluce left a comment

poiana commented Sep 28, 2023

incertum left a comment

poiana commented Sep 28, 2023

incertum commented Sep 28, 2023

Andreagit97 Sep 29, 2023

Andreagit97 Sep 29, 2023

update: engine_version in semver representation #2838

update: engine_version in semver representation #2838

Conversation

loresuso commented Sep 26, 2023 • edited Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

jasondellaluce commented Sep 27, 2023

poiana commented Sep 27, 2023

loresuso commented Sep 27, 2023

poiana commented Sep 27, 2023

loresuso commented Sep 28, 2023

jasondellaluce left a comment

Choose a reason for hiding this comment

poiana commented Sep 28, 2023

incertum left a comment

Choose a reason for hiding this comment

poiana commented Sep 28, 2023

incertum commented Sep 28, 2023

Andreagit97 Sep 29, 2023

Choose a reason for hiding this comment

Andreagit97 Sep 29, 2023

Choose a reason for hiding this comment

loresuso commented Sep 26, 2023 •

edited

Loading