Enhancement/4534 remove beats disk queue encryption #42643

kaanyalti · 2025-02-07T23:27:55Z

Enhancement

Proposed commit message

Removed diskqueue encryption, compression and v2 implementation. The main goal of this PR is to remove the diskqueue encryption as it uses an encryption function that is not implemented for Windows in Microsoft's golang fork which we are using for FIPS compliance. Along with encryption this PR also removes compression and the v2 implementation of the diskqueue all together.

Checklist

My code follows the style guidelines of this project
~~[ ] I have commented my code, particularly in hard-to-understand areas~~
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

There shouldn't be any user impacting changes

How to test this PR locally

Related issues

Closes https://github.com/elastic/ingest-dev/issues/4534

mergify · 2025-02-07T23:28:30Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kaanyalti? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

elasticmachine · 2025-02-08T02:08:49Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

leehinman

I think it would be good to leave the V0 & V1 pic and svg files in the docs directory. Those are still current. Also the on-disk-structures.md file in that directory needs to be updated.

Otherwise LGTM

…ed the on-disk-structures markdown

kaanyalti · 2025-02-18T18:34:34Z

libbeat/publisher/queue/diskqueue/docs/on-disk-structures.md

-There are currently 3 versions of the disk queue, and the current code
-base is able to write versions 1 & 2, while it is able to read version
-0, 1, and 2.
+There are currently 2 versions of the disk queue, and the current code


@leehinman Could you please confirm that the information here is correct.

leehinman

LGTM

faec · 2025-02-18T18:37:14Z

libbeat/publisher/queue/diskqueue/segments.go

-	ENABLE_COMPRESSION                    // 0x2
-	ENABLE_PROTOBUF                       // 0x4
-)
+const segmentHeaderSize = 8


I'm not sure about this, it looks like it will break if a preexisting segment file is opened with the new version because it has no way to know how big the header is. A change that affects the size of the segment header requires incrementing the schema version and respecting the header sizes of previous versions on load, so we should either leave the header size as 12 and relabel the last 4 bytes as reserved space, or increment the schema version to 3 and update (*queueSegment).headerSize().

faec · 2025-02-18T18:46:41Z

libbeat/publisher/queue/diskqueue/segments.go

-	header, err := readSegmentHeader(file)
+	// We don't need the header contents here, we just want to advance past the
+	// header region, so discard the return value.
+	_, err = readSegmentHeader(file)


If we don't need these contents then this read can be skipped entirely, the reader loop already seeks to the beginning of the unprocessed data before reading.

faec · 2025-02-18T18:53:00Z

libbeat/publisher/queue/diskqueue/segments.go

@@ -424,25 +356,34 @@ func readSegmentHeader(in io.Reader) (*segmentHeader, error) {
 	if err != nil {
 		return nil, fmt.Errorf("could not read segment version: %w", err)
 	}
-
 	if header.version > currentSegmentVersion {


This looks like it will refuse to open any segments created with a previous build, we should still accept schema 2 as valid input since it doesn't inherently require any of the encryption code that's being removed.(This is why I prefer incrementing the schema version rather than reverting to 1, because it keeps the special cases restricted to headerSize() which is explicitly a backwards compatibility wrapper. But if we do revert the "current" schema to 1 then we will need to special-case this check and document along with the current version that version 2 is reserved and should be skipped if the schema ever changes again.)

The main goal of this PR is to remove the encryption code. After discussing how to approach this, I decided to revert the PR that added encryption which also was the PR that added v2. If the issue here is the reduced header size, instead of reverting I can just remove encryption. I may still have to fill the options with null bytes in that case for v2.

I just saw your comment below. I will then leave everything as is, and remove encryption, and fill in the options with null bytes.

faec · 2025-02-18T19:00:33Z

libbeat/publisher/queue/diskqueue/segments.go

+		return err
+	}
+	err = binary.Write(out, binary.LittleEndian, frameCount)
+	return err


If we do end up sticking with schema 2 instead of incrementing to 3, then we should probably still write out null bytes for the options here.

dliappis · 2025-02-19T08:24:19Z

.buildkite/pipeline-scheduler.py

@@ -73,7 +73,8 @@ def get_release_branches() -> typing.List[str]:
    try:
        release_branches = [branch for branch in resp["branches"]]
    except KeyError:
-        fail_with_error(f'''Didn't find the excepted structure ["branches"] in the response [{resp}] from [{ACTIVE_BRANCHES_URL}]''')
+        fail_with_error(
+            f'''Didn't find the excepted structure ["branches"] in the response [{resp}] from [{ACTIVE_BRANCHES_URL}]''')


nit: this is out of scope for this PR do you think we could undo it? Changing this, due to the CODEOWNERS file, resulted in requesting a review from an additional team (ingest-eng-prod) which isn't otherwise required for this PR.

dliappis · 2025-02-19T08:24:28Z

.buildkite/pipeline-scheduler.py

@@ -103,7 +104,8 @@ def generate_pipeline(pipelines_to_trigger: typing.List[str], branches: typing.L

    target_branches = sorted(list(set(release_branches).difference(exclude_branches)))
    if len(target_branches) == 0 or target_branches[0].isspace():
-        fail_with_error(f"Calculated target branches were empty! You passed EXCLUDE_BRANCHES={exclude_branches} and release branches are {release_branches} the difference of which results in {target_branches}.")
+        fail_with_error(
+            f"Calculated target branches were empty! You passed EXCLUDE_BRANCHES={exclude_branches} and release branches are {release_branches} the difference of which results in {target_branches}.")


same comment as above.

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 7, 2025

mergify bot assigned kaanyalti Feb 7, 2025

kaanyalti force-pushed the enhancement/4534_remove_beats_disk_queue_encryption branch from 580f087 to c1e7aba Compare February 8, 2025 00:53

kaanyalti marked this pull request as ready for review February 8, 2025 02:05

kaanyalti requested review from a team as code owners February 8, 2025 02:06

kaanyalti requested review from faec and leehinman February 8, 2025 02:06

kaanyalti added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Feb 8, 2025

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 8, 2025

leehinman reviewed Feb 10, 2025

View reviewed changes

kaanyalti added 9 commits February 18, 2025 12:54

enhancement(4534): reverting encryption disqueue changes

70c1dca

enhancement(4523): updated reader loop

bba9afe

enhancement(4534): updated tests

bfea45d

enhancement(4534): ran make fmt

b9dab36

enhancement(4534): ran make udpate

f88345f

enhancement(4534): remove useJSON from encoder

b51e7cf

enhancement(4534): remove deprecated rand and use rand/v2

678de96

enhancement(4534): made changes to resolve linting issues

c5a3dda

enhancement(4534): added error checks to resolve linting issues

200dc1f

kaanyalti force-pushed the enhancement/4534_remove_beats_disk_queue_encryption branch from 0f1ef70 to 200dc1f Compare February 18, 2025 17:55

enhancement(4534): added diagrams back in, removed v2 diagrams, updat…

f23b655

…ed the on-disk-structures markdown

kaanyalti commented Feb 18, 2025

View reviewed changes

kaanyalti requested a review from leehinman February 18, 2025 18:34

kaanyalti added 2 commits February 18, 2025 13:39

enhancement(4534): added changelog

f539b6e

enhancement(4534): added issue and pr information to changelog

320322f

leehinman approved these changes Feb 18, 2025

View reviewed changes

faec requested changes Feb 18, 2025

View reviewed changes

dliappis reviewed Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/4534 remove beats disk queue encryption #42643

Enhancement/4534 remove beats disk queue encryption #42643

kaanyalti commented Feb 7, 2025 •

edited

Loading

mergify bot commented Feb 7, 2025

elasticmachine commented Feb 8, 2025

leehinman left a comment

kaanyalti Feb 18, 2025

leehinman left a comment

faec Feb 18, 2025

faec Feb 18, 2025

faec Feb 18, 2025

kaanyalti Feb 18, 2025

kaanyalti Feb 18, 2025

faec Feb 18, 2025

dliappis Feb 19, 2025

dliappis Feb 19, 2025

Enhancement/4534 remove beats disk queue encryption #42643

Are you sure you want to change the base?

Enhancement/4534 remove beats disk queue encryption #42643

Conversation

kaanyalti commented Feb 7, 2025 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

How to test this PR locally

Related issues

mergify bot commented Feb 7, 2025

elasticmachine commented Feb 8, 2025

leehinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leehinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaanyalti commented Feb 7, 2025 •

edited

Loading