Skip to content

Restore code review changes#17539

Merged
donoghuc merged 3 commits intoelastic:8.19from
donoghuc:restore-code-review-changes
May 20, 2025
Merged

Restore code review changes#17539
donoghuc merged 3 commits intoelastic:8.19from
donoghuc:restore-code-review-changes

Conversation

@donoghuc
Copy link
Copy Markdown
Member

@donoghuc donoghuc commented Apr 9, 2025

Release notes

[rn:skip]

What does this PR do?

Some code review comments from #17486 were committed via the github UI but when locally rebased they were excluded due to a missed sync. This PR restores them.

Why is it important/What is the impact to the user?

N/A

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 (and/or docker env variables)
  • [ ] I have added tests that prove my fix is effective or that my feature works

Related Issues

@donoghuc
Copy link
Copy Markdown
Member Author

donoghuc commented Apr 9, 2025

@yaauie I accidentally dropped some of the suggestions from #17486 (comment) This resotres them.

@donoghuc donoghuc requested a review from yaauie April 9, 2025 21:01
@donoghuc donoghuc changed the base branch from feature/fedramp-high-8.x to 8.x April 10, 2025 22:05
@donoghuc donoghuc force-pushed the restore-code-review-changes branch from 63a5147 to 74fe4d3 Compare April 10, 2025 22:08
@donoghuc
Copy link
Copy Markdown
Member Author

Rebased on 8.x now that feature branch has been merged.

@elastic-sonarqube
Copy link
Copy Markdown

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@elasticmachine
Copy link
Copy Markdown

💚 Build Succeeded

History

Copy link
Copy Markdown
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM!

@donoghuc donoghuc changed the base branch from old-8.x to 8.19 May 20, 2025 21:47
@donoghuc donoghuc merged commit 74bba3f into elastic:8.19 May 20, 2025
7 checks passed
yaauie pushed a commit to yaauie/logstash that referenced this pull request Jul 9, 2025
* Comment to clarify why FIPS flag is not needed for smoke tests

* Use full versions of docker commands for readability

* Simplify grock pattern match

The grok pattern is unanchored-by-default, we don't need the leading and trailing
wildcards.
yaauie pushed a commit to yaauie/logstash that referenced this pull request Jul 14, 2025
* Comment to clarify why FIPS flag is not needed for smoke tests

* Use full versions of docker commands for readability

* Simplify grock pattern match

The grok pattern is unanchored-by-default, we don't need the leading and trailing
wildcards.
yaauie pushed a commit to yaauie/logstash that referenced this pull request Jul 15, 2025
* Comment to clarify why FIPS flag is not needed for smoke tests

* Use full versions of docker commands for readability

* Simplify grock pattern match

The grok pattern is unanchored-by-default, we don't need the leading and trailing
wildcards.
yaauie added a commit that referenced this pull request Jul 16, 2025
…in (#17785)

* forward-port observabilitySRE image creation into `main`

This is the CLEAN subset of a cherry-pick of the merge-commit from the
observabilitySRE feature branch into 8.x in PR #17541 (0b1d299),
OMITTING changes to `docker/*` and `rakelib/artifacts.rake` that would
conflict due to substantial refactorings on `main`.

* forward-port observabilitySRE image creation into `main` (re-implament)

This is a forward-port of _functionality_ from the observabilitySRE feature
branch into 8.x in PR #17541 (0b1d299),
wholly re-implementing the changes in `docker/*` and `rakelib/artifacts.rake`
from the 8.x-style docker structure to the refactored structure present
on `main`.

* Fix pull request pipeline definition for buildkite (#17552)

When the fedramp high feature branch was merged into 8.x the PR pipeline
accidentally duplicated the top level `steps` key. This was a mistake and is
causing issues generating exhaustive test pipeline definition. This commit fixes
the bug by ensuring there is a single `steps` key that defines all the steps in
the pipeline.

* Ensure observabilitySRE image is pushed on DRA staging (#17569)

The `artifactDockerObservabilitySRE` gradle task *always* produces a tag with a
`SNAPSHOT` postfix. In the staging pipeline we use the shared
`qualified-version` script for determining the LS version. That script correctly
handles conditionally adding a `SNAPSHOT` postfix which is important for the
tagging scheme for pushing to our container registry. Given the intermediate tag
produced by the gradle task is never pushed anywhere we can update the build
script to ensure the "local" artifact is always referenced with the `SNAPSHOT`
postfix.

* Use dedicated elasticsearch image for observabilitySRE smoke testing (#17627)

* Use dedicated elasticsearch image for observabilitySRE smoke testing

The ES team has started publishing a purpose built image for the fedramp high
project. Update our smoke test stack to use this container.

* Override default entrypoint into elasticsearch container

The new image does not provide the stub `/app/elasticsearch.sh` file
https://github.com/elastic/elasticsearch/blob/1a1763c591c4c32bf66f0df3bce2040e8f19a1a2/distribution/docker/README.md?plain=1#L16-L19
previously available. This commit overrides the entrypoint to avoid needing that
file. See: https://github.com/elastic/elasticsearch/blob/1a1763c591c4c32bf66f0df3bce2040e8f19a1a2/distribution/docker/src/docker/Dockerfile.ess#L38C5-L40C37

* Remove entrypoint workaround due to fix landing upstream

* Restore code review changes (#17539)

* Comment to clarify why FIPS flag is not needed for smoke tests

* Use full versions of docker commands for readability

* Simplify grock pattern match

The grok pattern is unanchored-by-default, we don't need the leading and trailing
wildcards.

* Add a step to exhaustive tests for observabilitySRE accetpance testing (#17623)

* Add a step to exhaustive tests for observabilitySRE accetpance testing

This commit shows the proposed pattern for adding acceptance testing for the
observability SRE image. This will run when exhaustive tests run. A new gradle
task will hook in to rspec similar to how it is done for the smoke tests. The
main difference is that instead of building a container, the latest is pulled
from the container registry and run on a fips configured host VM.

* WIP: Idea for how to handle multipe container configs for acceptance tests

This commit shows the rough structure for how I am planning on handling docker
compose networks for acceptance tests. The main idea is to use interpolation in
the docker compose file to point to different configuration files for
filebeat/logstash/elasticsearch. This is mainly due to the nature of these tests
showing behavior when the system is and is not configured properly for FIPS. The
breakdown in responsibility is:

1. Gradle handles cert generation (similar to smoke test, this avoids checking
in PKI)
2. Rspec handles stopping/starting docker compose and managing environment vars
for intperolation in docker compose manifests (different from smoke tests where
a single static docker compose is started in gradle)
3. Rspec handles deciding when containers are ready and querying state about
data flowing through the system
4. Gradle cleans up certs

THis is just a rough sketch, there are still bugs to be worked out but before i
get too far in to it I want to get the idea out there.

* Add tests describing behavior of LS -> ES with non-fips config

This commit adds a test to show that data will not flow from LS to ES
when weak non fips config is used.

* Use latest ES image

This will be handled separately in a separate PR, but taking this
commit for now on this branch.

* Remove custom entrypoint from new container

The latest ES images do not require this workaround.

* Take up code review suggestions

1. Remove rogue character from test file causing interpreter failure
2. Split out helpers for docker compose orchestration
3. Only send a single message instead of infinite through to ES

* Add full prefix name for new image

* Test filebeat -> LS -> ES using fips config

As described in elastic/ingest-dev#5471 this commit
adds a test for filebeat sending data through logstash to elasticsearch using
fips config.

* Test LS wont accept input from non fips configured filebeat

This test ensures logstash will not accept data from filebeat when using weak
tls configuration.

See elastic/ingest-dev#5472

* Fix a funny typo.

Crytpo is actually kind of a funny.

* Ensure we are using the purpose build ES image in testing

Similar to #17627

* Ensure JAVA_HOME is set etc

Use the same buildkite agent script for setting up a vm based runner as other pipes

---------

Co-authored-by: Cas Donoghue <cas.donoghue@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants