atharvalade commented on code in PR #2898:
URL: https://github.com/apache/iggy/pull/2898#discussion_r2916079676
##########
foreign/go/client/tcp/tcp_core.go:
##########
Review Comment:
If `createTLSConfig() `fails here, the TCP connection opened by `net.Dial` a
few lines above just gets abandoned. Nothing closes it. The handshake failure
path right below already does `connection.Close() `but this early return was
missed.
This gets worse because the retry loop defaults to infinite retries. So if
the TLS config keeps failing (bad CA path, for example), every retry opens a
new TCP socket and leaks it. You will eventually run out of file descriptors.
Add `_ = connection.Close()` before the return to match what the handshake
error path already does.
##########
scripts/run-go-examples-from-readme.sh:
##########
@@ -169,9 +169,85 @@ cd ../..
kill -TERM "$(cat ${PID_FILE})"
test -e ${PID_FILE} && rm ${PID_FILE}
+# --- TLS Test Pass ---
+if [ "${exit_code}" -eq 0 ]; then
+ echo ""
+ echo -e "\e[36m=== Starting TLS test pass ===\e[0m"
+ echo ""
+
+ # Clean data and logs for fresh TLS start
+ rm -fr local_data
+ rm -f ${LOG_FILE}
+
+ # Start server with TLS enabled
+ echo "Starting server with TLS enabled..."
+ IGGY_ROOT_USERNAME=iggy IGGY_ROOT_PASSWORD=iggy IGGY_TCP_TLS_ENABLED=true
${SERVER_BIN} &>${LOG_FILE} &
+ echo $! >${PID_FILE}
+
+ # Wait for server to start
+ SERVER_START_TIME=0
+ while ! grep -q "has started" ${LOG_FILE}; do
+ if [ ${SERVER_START_TIME} -gt ${TIMEOUT} ]; then
+ echo "TLS server did not start within ${TIMEOUT} seconds."
+ ps fx
+ cat ${LOG_FILE}
+ exit 1
+ fi
+ echo "Waiting for Iggy TLS server to start... ${SERVER_START_TIME}"
+ sleep 1
+ ((SERVER_START_TIME += 1))
+ done
+
+ # Verify TLS is enabled
+ if ! grep -q "tls: { enabled: true" ${LOG_FILE}; then
+ echo -e "\e[31mError: TLS not enabled on server\e[0m"
+ grep -A 5 "tcp:" ${LOG_FILE} || true
+ exit 1
+ fi
+ echo -e "\e[32m✓ TLS enabled on server\e[0m"
+
+ cd examples/go || exit 1
+
+ # Run getting-started examples with TLS flags
+ # Use localhost instead of 127.0.0.1 to match the cert's SAN
(DNS:localhost)
+ TLS_CA_FILE="../../core/certs/iggy_ca_cert.pem"
+ TLS_ADDR="localhost:8090"
+
+ for cmd in \
+ "go run getting-started/producer/main.go --tcp-server-address
${TLS_ADDR} --tls --tls-ca-file ${TLS_CA_FILE}" \
+ "go run getting-started/consumer/main.go --tcp-server-address
${TLS_ADDR} --tls --tls-ca-file ${TLS_CA_FILE}"; do
+
+ echo -e "\e[33mChecking TLS example:\e[0m ${cmd}"
+ echo ""
+
+ set +e
+ eval "timeout 10 ${cmd}"
+ test_exit_code=$?
+ set -e
+
+ if [[ $test_exit_code -ne 0 && $test_exit_code -ne 124 ]]; then
+ echo ""
+ echo -e "\e[31mTLS example command failed:\e[0m ${cmd}"
+ echo ""
+ exit_code=$test_exit_code
+ break
+ fi
+ sleep 2
+ done
+
+ cd ../..
+
+ # Terminate TLS server
+ kill -TERM "$(cat ${PID_FILE})"
+ kill -TERM "$(cat ${PID_FILE})" 2>/dev/null || true
Review Comment:
There are two back-to-back kill -TERM calls on the same PID. The first one
has no error handling, and the second one has `2>/dev/null || true`. Since the
script uses set -e, if the process already exited before line 241 runs, the
first kill will fail and the script will abort before it even reaches the safe
version on line 242.
##########
foreign/go/tests/tls_test.go:
##########
@@ -0,0 +1,296 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Package tests_test contains TLS integration tests for the Go SDK.
+//
+// These tests use testcontainers to spin up a TLS-enabled Iggy server,
+// making them fully self-contained and suitable for CI.
+//
+// The IGGY_SERVER_DOCKER_IMAGE environment variable must be set to a locally
+// built Docker image. Do NOT use images from DockerHub — CI must test the code
+// from the current PR, not a previously released version.
+//
+// Build the image locally:
+//
+// cargo build && docker build -f core/server/Dockerfile \
+// --build-arg PREBUILT_IGGY_SERVER=target/debug/iggy-server \
+// --build-arg PREBUILT_IGGY_CLI=target/debug/iggy \
+// --build-arg LIBC=glibc --target runtime-prebuilt -t iggy-server:test .
+//
+// Then run tests:
+//
+// IGGY_SERVER_DOCKER_IMAGE=iggy-server:test go test -v ./tests
+//
+// In CI, the image is built by
.github/actions/utils/docker-build-test-server/action.yml
+package tests_test
+
+import (
+ "context"
+ "fmt"
+ "net"
+ "os"
+ "path/filepath"
+ "testing"
+ "time"
+
+ "github.com/apache/iggy/foreign/go/client"
+ "github.com/apache/iggy/foreign/go/client/tcp"
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "github.com/testcontainers/testcontainers-go"
+ "github.com/testcontainers/testcontainers-go/wait"
+)
+
+const (
+ defaultUsername = "iggy"
+ defaultPassword = "iggy"
+)
+
+// setupTLSContainer starts a TLS-enabled Iggy server container.
+func setupTLSContainer(t *testing.T) (testcontainers.Container, string,
string) {
+ ctx := context.Background()
+
+ repoRoot, err := filepath.Abs(filepath.Join("..", "..", ".."))
+ require.NoError(t, err)
+ certsPath := filepath.Join(repoRoot, "core", "certs")
+ caFile := filepath.Join(certsPath, "iggy_ca_cert.pem")
+
+ // Verify certs exist
+ _, err = os.Stat(certsPath)
+ require.NoError(t, err, "Certs directory not found at %s", certsPath)
+ _, err = os.Stat(caFile)
+ require.NoError(t, err, "CA cert not found at %s", caFile)
+
+ // Require locally built Docker image — never pull from DockerHub.
+ // In CI, the image is built by docker-build-test-server action.
+ // Locally, build with: cargo build && docker build -f
core/server/Dockerfile ...
+ dockerImage := os.Getenv("IGGY_SERVER_DOCKER_IMAGE")
+ if dockerImage == "" {
+ t.Fatal("IGGY_SERVER_DOCKER_IMAGE env var is required. " +
Review Comment:
Both the C# and Python SDKs fall back to `apache/iggy:edge `when
`IGGY_SERVER_DOCKER_IMAGE` is not set. This hard-fails instead, which makes
running these tests locally more cumbersome than in the other SDKs. Consider
defaulting to `apache/iggy:edge` to match the established pattern across the
project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
