Copilot commented on code in PR #6802:
URL: https://github.com/apache/hive/pull/6802#discussion_r4048240325
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:
##########
@@ -360,13 +369,82 @@ protected static String sha256(String... inputs) {
}
}
+ /**
+ * Validates CR-provided database fields before they are embedded into
+ * shell command lines (schematool -dbType in the schema-init Job) and
+ * into SERVICE_OPTS, which the image entrypoint expands into JVM arguments.
+ */
+ protected static void validateDatabaseConfig(DatabaseConfig db) {
+ if (!DB_TYPE_PATTERN.matcher(db.type()).matches()) {
+ throw new IllegalArgumentException(
+ "spec.metastore.database.type must be one of derby, mysql, postgres,
mssql, oracle; got: " + db.type());
+ }
+ validateOptValue("spec.metastore.database.url", db.url());
+ validateOptValue("spec.metastore.database.driver", db.driver());
+ validateOptValue("spec.metastore.database.username", db.username());
+ }
+
+ private static void validateOptValue(String field, String value) {
+ if (containsUnsafeShellChars(value)) {
+ throw new IllegalArgumentException(field + " must not contain
whitespace, quotes, backslashes or "
+ + "control characters");
Review Comment:
`SERVICE_OPTS` is emitted as a Kubernetes `EnvVar`, and this code
deliberately relies on `$(DBPASSWORD)` expansion earlier in the method. A CR
value containing `$(DBPASSWORD)` (or another defined variable) in `url`,
`driver`, or `username` is therefore expanded by the kubelet after this check,
so the value reaching the JVM is not the validated string and can include
secret contents or extra option text. Reject Kubernetes `$(...)` references for
these fields, or avoid placing raw CR text in an expandable env value.
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/reconciler/HiveClusterReconciler.java:
##########
@@ -387,6 +389,41 @@ private <T extends HasMetadata> ComponentStatus
buildComponentStatus(
return cs;
}
+ /**
+ * When the managed workflow dependents incur failures, update the Ready
+ * condition with the incurred error, while preserving component conditions.
+ */
+ private boolean applyWorkflowDependentErrors(HiveCluster resource,
Context<HiveCluster> context,
+ HiveClusterStatus newStatus, HiveClusterStatus existingStatus) {
+ var workflowResult =
context.managedWorkflowAndDependentResourceContext().getWorkflowReconcileResult();
+ if (workflowResult.isEmpty() ||
!workflowResult.get().erroredDependentsExist()) {
+ return false;
+ }
+
+ Exception error =
workflowResult.get().getErroredDependents().values().iterator().next();
+ String errorMessage = error.getMessage();
+ LOG.error("Error reconciling HiveCluster: {}/{} - {}",
resource.getMetadata().getNamespace(),
+ resource.getMetadata().getName(), errorMessage, error);
+
+ List<Condition> existingConditions = existingStatus != null &&
existingStatus.getConditions() != null
+ ? existingStatus.getConditions() : Collections.emptyList();
+ boolean alreadyReported = existingConditions.stream()
+ .anyMatch(c -> "Ready".equals(c.getType())
+ && "False".equals(c.getStatus())
+ && "ReconciliationError".equals(c.getReason())
+ && errorMessage.equals(c.getMessage()));
Review Comment:
An errored dependent can carry an exception with a null message (for
example, `new RuntimeException()`), and after the first status write
`existingConditions` contains `Ready`, so this predicate calls
`errorMessage.equals(...)` and throws an NPE on every subsequent reconcile
instead of reporting the workflow error. Use a null-safe comparison.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
