Remove elasticsearch from default capture proxy base image #1061

AndreKurait · 2024-10-10T21:49:52Z

Description

Currently, the capture proxy container image builds on the es base image. This is not needed for running the capture proxy independently. This change modifies this base image to use AL2023.

This change uses a multi-staged build to generate the certs with openssl without including it in the final image.

Other Changes:

Cleaned up how cdk references docker images (uses local tag instead of rebuilding from final build directory)
Cleaned up how migration console is being built, made more robust by using dedicated build directory instead of the source directory and fixed it erroneously being built
Fixed TrafficCaptureProxyServerTest dockerImageBuild by explicitly declaring dependencies

Category: Enhancement
Why these changes are required? Increased security
What is the old behavior before changes and new behavior after changes?

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-2078

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Local docker/gradle testing, cdk deploy in aws

akurait@80a9970d0850 opensearch-migrations % curl https://localhost:9200 --insecure -u admin:admin -v 
* Host localhost:9200 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:9200...
* Connected to localhost (::1) port 9200
* ALPN: curl offers h2,http/1.1
* (304) (OUT), TLS handshake, Client hello (1):
* (304) (IN), TLS handshake, Server hello (2):
* (304) (IN), TLS handshake, Unknown (8):
* (304) (IN), TLS handshake, Request CERT (13):
* (304) (IN), TLS handshake, Certificate (11):
* (304) (IN), TLS handshake, CERT verify (15):
* (304) (IN), TLS handshake, Finished (20):
* (304) (OUT), TLS handshake, Certificate (11):
* (304) (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / AEAD-AES256-GCM-SHA384 / [blank] / UNDEF
* ALPN: server did not agree on a protocol. Uses default.
* Server certificate:
*  subject: CN=localhost
*  start date: Oct 10 21:34:18 2024 GMT
*  expire date: Oct 10 21:34:18 2025 GMT
*  issuer: CN=localhost
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* using HTTP/1.x
* Server auth using Basic with user 'admin'
> GET / HTTP/1.1
> Host: localhost:9200
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 200 OK
< content-type: application/json; charset=UTF-8
< content-length: 538
< 
{
  "name" : "10b92fdc2ca3",
  "cluster_name" : "docker-cluster",
  "cluster_uuid" : "0MZAhMQlQzejHWzug61-rA",
  "version" : {
    "number" : "7.10.2",
    "build_flavor" : "oss",
    "build_type" : "docker",
    "build_hash" : "747e1cc71def077253878a59143c1f785afa92b9",
    "build_date" : "2021-01-13T00:42:12.435326Z",
    "build_snapshot" : false,
    "lucene_version" : "8.7.0",
    "minimum_wire_compatibility_version" : "6.8.0",
    "minimum_index_compatibility_version" : "6.0.0-beta1"
  },
  "tagline" : "You Know, for Search"
}
* Connection #0 to host localhost left intact

Check List

New functionality includes testing
- All tests pass, including unit test, integration test and doctest
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2024-10-10T21:50:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.19%. Comparing base (0b08287) to head (2bd40b8).
Report is 40 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1061      +/-   ##
============================================
+ Coverage     80.16%   80.19%   +0.02%     
- Complexity     2744     2863     +119     
============================================
  Files           367      383      +16     
  Lines         13743    14333     +590     
  Branches        949      988      +39     
============================================
+ Hits          11017    11494     +477     
- Misses         2149     2245      +96     
- Partials        577      594      +17

Flag	Coverage Δ
gradle-test	`78.21% <ø> (+0.13%)`	⬆️
python-test	`90.24% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ild improvements Signed-off-by: Andre Kurait <[email protected]>

Signed-off-by: Andre Kurait <[email protected]>

lewijacn

Thanks for refactoring some of this, left some comments

TrafficCapture/dockerSolution/build.gradle

deployment/cdk/opensearch-service-migration/lib/common-utilities.ts

lewijacn · 2024-10-14T17:56:03Z

deployment/cdk/opensearch-service-migration/lib/common-utilities.ts

+ * @param {string} imageName - The name of the Docker image to use as the base image.
+ * @returns {ContainerImage} - A `ContainerImage` object representing the Docker image asset.
+ */
+export function makeDockerImageAsset(scope: Construct, serviceName: string, imageName: string): ContainerImage {


Is this much better than what existed previously? I guess it has the added benefit of not potentially rebuilding intermediary layers in our image, but does seem to subtly introduce a new wrapper layer that we are pushing to ECR. This is probably a good trade but curious if you had other thoughts

The other benefit here is that we're not depending on the build output directory anymore. I don't know of any functional difference with the wrapper layer being here

This is just an alias, right? Please rename the function & explain why we need an alias.

Updated, this is an alias, but it also pulls the image locally. This will add it to a private ECR instead of pulling on container start potentially from an outside source (which can add costly nat gateway bills)

TrafficCapture/trafficCaptureProxyServerTest/build.gradle

TrafficCapture/dockerSolution/build.gradle

Signed-off-by: Andre Kurait <[email protected]>

TrafficCapture/dockerSolution/build.gradle

TrafficCapture/dockerSolution/src/main/docker/captureProxyBase/Dockerfile

gregschohn · 2024-10-15T12:45:12Z

TrafficCapture/trafficCaptureProxyServerTest/build.gradle

 def javaContainerServices = [
-        ":TrafficCapture:trafficCaptureProxyServerTest": "jmeter"
+        "jmeter": ":TrafficCapture:trafficCaptureProxyServerTest"
 ]


we should really just kill this thing or figure out how to rig a valuable test with it (I'm fine yanking it until we do)

Call this something other than jmeter since it's pulling one of our packages in too?
Thanks for noticing that they were flipped.

this is published under migrations/jmeter, can we keep this in this PR since this is an existing feature and we can make a separate one to review regarding removing it. I'd like to get more maintainers input on that.

deployment/cdk/opensearch-service-migration/cdk.context.json

deployment/cdk/opensearch-service-migration/lib/service-stacks/migration-service-core.ts

deployment/cdk/opensearch-service-migration/lib/common-utilities.ts

gregschohn · 2024-10-15T13:02:17Z

deployment/cdk/opensearch-service-migration/lib/common-utilities.ts

+ * @param {string} imageName - The name of the Docker image to use as the base image.
+ * @returns {ContainerImage} - A `ContainerImage` object representing the Docker image asset.
+ */
+export function makeDockerImageAsset(scope: Construct, serviceName: string, imageName: string): ContainerImage {


This is just an alias, right? Please rename the function & explain why we need an alias.

gregschohn · 2024-10-15T13:03:12Z

deployment/cdk/opensearch-service-migration/lib/service-stacks/migration-console-stack.ts

@@ -313,7 +313,7 @@ export class MigrationConsoleStack extends MigrationServiceCore {

        this.createService({
            serviceName: "migration-console",
-            dockerDirectoryPath: join(__dirname, "../../../../../", "TrafficCapture/dockerSolution/src/main/docker/migrationConsole"),
+            dockerImageRegistryName: "migrations/migration_console:latest",


awesome! This saves a big headache in an upcoming refactor!

Signed-off-by: Andre Kurait <[email protected]>

gregschohn · 2024-10-16T12:49:23Z

.github/workflows/CI.yml

+          image=$(docker images --format '{{.Repository}}:{{.Tag}}' | head -n 1)
+          echo "Using image for mocked tags: $image"
+          docker tag $image migrations/capture_proxy:latest


I don't like the idea of using a mock image tag that resolves to a valid version of the image for potentially one image and for all others it resolves to some other equally valid image. If you need mock values, can you just hard code them? What are you trying to test here?

resolves to a valid version of the image for potentially one image

This is not the case, this github step doesn't have any built images so this is using the default node docker image for all the tags.

This is verifying the cdk maps to these images and properly synthesizes with docker.

gregschohn · 2024-10-16T13:04:42Z

DocumentsFromSnapshotMigration/build.gradle

@@ -144,6 +144,6 @@ tasks.named('composeUp') {
 }

 tasks.named('slowTest') {
-    dependsOn(':TrafficCapture:dockerSolution:buildDockerImage_elasticsearchTestConsole')
+    dependsOn(':TrafficCapture:dockerSolution:buildDockerImage_elasticsearch_client_test_console')


nit because it's style, but I miss the camel casing. The '' that was previously used was for effect to separate the auto-generated part from the task name. Other than that underscore, none of the other tasks in the tree use it (AFAIK).
Take a look at the gradle CLI on task names - w/ camel case (or kebab-case), you can do abbreviations like agradle :TC:DS:BDI_M - you can still put snake-case as that example shows, but it's a bit clunkier. I'd also be happy if we swapped the "buildDockerImage" prefix for an "Image" suffix or just a "build" prefix.

gregschohn · 2024-10-16T13:13:29Z

TrafficCapture/dockerSolution/src/main/docker/composeExtensions/proxy-single.yml

@@ -25,7 +25,7 @@ services:
      - migrations
    ports:
      - "9200:9200"
-    command: /runJavaWithClasspath.sh org.opensearch.migrations.trafficcapture.proxyserver.CaptureProxy  --kafkaConnection kafka:9092 --destinationUri  https://elasticsearch:9200  --insecureDestination --listenPort 9200 --sslConfigFile /usr/share/elasticsearch/config/proxy_tls.yml --otelCollectorEndpoint http://otel-collector:4317
+    command: /runJavaWithClasspath.sh org.opensearch.migrations.trafficcapture.proxyserver.CaptureProxy  --kafkaConnection kafka:9092 --destinationUri  https://elasticsearch:9200  --insecureDestination --listenPort 9200 --sslConfigFile /usr/share/captureProxy/config/proxy_tls.yml --otelCollectorEndpoint http://otel-collector:4317


How does this line work? I'm not sure that it does since the capture_proxy_es builds off of elasticsearch + searchguard & I don't see any changes to that dockerfile.

Do we have tests for both proxy containers w/ & w/out TLS enabled?

This line is without elastic

capture-proxy: image: 'migrations/capture_proxy:latest' networks:

All of our current testing has TLS

shoot - sorry, there was a collapse bar between this & the previous snippet!

gregschohn · 2024-10-17T03:03:52Z

TrafficCapture/dockerSolution/src/main/docker/composeExtensions/proxy-single.yml

@@ -25,7 +25,7 @@ services:
      - migrations
    ports:
      - "9200:9200"
-    command: /runJavaWithClasspath.sh org.opensearch.migrations.trafficcapture.proxyserver.CaptureProxy  --kafkaConnection kafka:9092 --destinationUri  https://elasticsearch:9200  --insecureDestination --listenPort 9200 --sslConfigFile /usr/share/elasticsearch/config/proxy_tls.yml --otelCollectorEndpoint http://otel-collector:4317
+    command: /runJavaWithClasspath.sh org.opensearch.migrations.trafficcapture.proxyserver.CaptureProxy  --kafkaConnection kafka:9092 --destinationUri  https://elasticsearch:9200  --insecureDestination --listenPort 9200 --sslConfigFile /usr/share/captureProxy/config/proxy_tls.yml --otelCollectorEndpoint http://otel-collector:4317


shoot - sorry, there was a collapse bar between this & the previous snippet!

AndreKurait force-pushed the CaptureProxyBaseImage branch 6 times, most recently from adf072e to 704f424 Compare October 11, 2024 21:32

Split standalone capture proxy from elasticsearch and other docker bu…

e304aa1

…ild improvements Signed-off-by: Andre Kurait <[email protected]>

AndreKurait force-pushed the CaptureProxyBaseImage branch from 81abe7e to 4777117 Compare October 11, 2024 21:41

AndreKurait marked this pull request as ready for review October 11, 2024 21:41

AndreKurait requested review from chelma, gregschohn, lewijacn, mikaylathompson, peternied and sumobrian as code owners October 11, 2024 21:41

Cleanup docker builds in cdk

0a4c8c9

Signed-off-by: Andre Kurait <[email protected]>

AndreKurait force-pushed the CaptureProxyBaseImage branch from 08cb381 to 0a4c8c9 Compare October 14, 2024 16:49

AndreKurait added 2 commits October 14, 2024 11:54

Cleanup how keystore is modified

c15d2aa

Signed-off-by: Andre Kurait <[email protected]>

Cleanup cdk docker image build args

7bc258a

Signed-off-by: Andre Kurait <[email protected]>

lewijacn reviewed Oct 14, 2024

View reviewed changes

Address PR Comments

5c3d165

Signed-off-by: Andre Kurait <[email protected]>

AndreKurait force-pushed the CaptureProxyBaseImage branch from b76a164 to 5c3d165 Compare October 15, 2024 03:53

gregschohn reviewed Oct 15, 2024

View reviewed changes

AndreKurait added 5 commits October 15, 2024 15:19

Address PR Comments

7d0aeea

Signed-off-by: Andre Kurait <[email protected]>

Address pr comments

534458a

Signed-off-by: Andre Kurait <[email protected]>

Rename capture proxy base files to remove es reference

26033a5

Signed-off-by: Andre Kurait <[email protected]>

Improve github actions for cdk jest tests

6d2424a

Signed-off-by: Andre Kurait <[email protected]>

Fix DocumentsFromSnapshotMigration slowTest

715b17e

Signed-off-by: Andre Kurait <[email protected]>

AndreKurait force-pushed the CaptureProxyBaseImage branch from 29968f2 to 715b17e Compare October 15, 2024 21:00

Update default npm test to build images

2bd40b8

Signed-off-by: Andre Kurait <[email protected]>

gregschohn reviewed Oct 16, 2024

View reviewed changes

gregschohn approved these changes Oct 17, 2024

View reviewed changes

AndreKurait merged commit d8b4d6c into opensearch-project:main Oct 17, 2024
14 checks passed

AndreKurait deleted the CaptureProxyBaseImage branch October 17, 2024 04:15

AndreKurait mentioned this pull request Oct 18, 2024

Update cdk ecr images when docker hash changes #1080

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove elasticsearch from default capture proxy base image #1061

Remove elasticsearch from default capture proxy base image #1061

AndreKurait commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

lewijacn left a comment

lewijacn Oct 14, 2024

AndreKurait Oct 14, 2024

gregschohn Oct 15, 2024

AndreKurait Oct 15, 2024

gregschohn Oct 15, 2024

gregschohn Oct 15, 2024

AndreKurait Oct 15, 2024

gregschohn Oct 15, 2024

gregschohn Oct 15, 2024

gregschohn Oct 16, 2024

AndreKurait Oct 16, 2024

gregschohn Oct 16, 2024

gregschohn Oct 16, 2024

gregschohn Oct 16, 2024

AndreKurait Oct 16, 2024

AndreKurait Oct 16, 2024

gregschohn Oct 17, 2024

gregschohn Oct 17, 2024

Remove elasticsearch from default capture proxy base image #1061

Remove elasticsearch from default capture proxy base image #1061

Conversation

AndreKurait commented Oct 10, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

lewijacn 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

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

Choose a reason for hiding this comment

AndreKurait commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading