Address coderabbitai comments

Author: anlowee

tools/deployment/presto-clp/scripts/generate-user-env-vars-file.py

@@ -111,7 +111,9 @@ def _add_clp_env_vars(
         / "data"
         / ("archives" if clp_archive_output_storage_type == "fs" else "staged-archives"),
     )
-    if "s3" == clp_archive_output_storage_type:
+    if "fs" == clp_archive_output_storage_type:
+        pass
+    elif "s3" == clp_archive_output_storage_type:
         s3_config_key_prefix = f"archive_output.storage.s3_config"
         s3_credentials_key_prefix = f"{s3_config_key_prefix}.aws_authentication.credentials"
 
@@ -121,15 +123,31 @@ def _add_clp_env_vars(
 
         s3_bucket = _get_config_value(clp_config, f"{s3_config_key_prefix}.bucket")
         s3_region_code = _get_config_value(clp_config, f"{s3_config_key_prefix}.region_code")
-        s3_end_point = f"https://{s3_bucket}.s3.{s3_region_code}.amazonaws.com/"
 
         s3_secret_access_key = _get_config_value(
             clp_config, f"{s3_credentials_key_prefix}.secret_access_key"
         )
 
+        # Validate required S3 fields
+        missing = []
+
+        for k, v in {
+            f"{s3_credentials_key_prefix}.access_key_id": s3_access_key_id,
+            f"{s3_credentials_key_prefix}.secret_access_key": s3_secret_access_key,
+            f"{s3_config_key_prefix}.bucket": s3_bucket,
+            f"{s3_config_key_prefix}.region_code": s3_region_code,
+        }.items():
+            if not v:
+                missing.append(k)
+
+        if missing:
+            logger.error("Missing required S3 config key(s): %s", ", ".join(missing))
+            return False
+
         env_vars["PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE"] = "s3"
         env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER"] = "clp_package"
         env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID"] = s3_access_key_id
+        s3_end_point = f"https://{s3_bucket}.s3.{s3_region_code}.amazonaws.com/"
         env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT"] = s3_end_point
         env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY"] = s3_secret_access_key
     else:

tools/deployment/presto-clp/worker/scripts/generate-configs.sh

@@ -56,7 +56,7 @@ update_config_file() {
         echo "${key}=${value}" >>"$file_path"
     fi
     if [[ "$sensitive" == "true" ]]; then
-        local masked="****${value: -4}"
+        local masked="****"
         log "INFO" "Set ${key}=${masked} in ${file_path}"
     else
         log "INFO" "Set ${key}=${value} in ${file_path}"
@@ -84,15 +84,20 @@ mv "${PRESTO_CONFIG_DIR}/clp.properties" "${PRESTO_CONFIG_DIR}/catalog"
 readonly CLP_PROPERTIES_FILE="/opt/presto-server/etc/catalog/clp.properties"
 if [ "${PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE:-}" = "s3" ]; then
     log "INFO" "Enable S3 support"
+    missing=()
     for var in PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER \
            PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID \
            PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY \
            PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT; do
         if [ -z "${!var:-}" ]; then
-            log "ERROR" "Missing required env var: $var"
-            exit 1
+            missing+=("$var")
         fi
     done
+    if [ ${#missing[@]} -gt 0 ]; then
+        log "ERROR" "Missing required env var(s): ${missing[*]}"
+        exit 1
+    fi
+
     update_config_file "$CLP_PROPERTIES_FILE" "clp.storage-type" "${PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE}"
     update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-auth-provider" "${PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER}"
     update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-access-key-id" "${PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID}" true