HIVE-29999: HiveCluster CR string fields should be validated before b… by tanishq-chugh · Pull Request #6802 · apache/hive

tanishq-chugh · 2026-09-18T15:29:40Z

…eing injected into k8s operator generated bash commands

What changes were proposed in this pull request?

Validate HiveCluster CR string fields before adding them into bash commands

Why are the changes needed?

To prevent any additional non-intended values being injected into specific HiveCluster CR string fields

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manual Testing

…eing injected into k8s operator generated bash commands

tanishq-chugh · 2026-09-18T15:30:57Z

Hi @ayushtkn
Could you please help with a review on this PR?
Thanks!

Copilot

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds runtime and CRD validation for database settings and external JAR locations, and reports dependent workflow errors in status.

Changes:

Validates database types and shell-sensitive values.
Validates external JAR URLs before command generation.
Adds workflow error status propagation.

File summaries

File	Description
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/reconciler/HiveClusterReconciler.java	Updated as part of this pull request.
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/model/spec/DatabaseConfig.java	Updated as part of this pull request.
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java	Updated as part of this pull request.
packaging/src/kubernetes/helm/hive-operator/crds/hiveclusters.hive.apache.org-v1.yml	Updated as part of this pull request.

Review details

Suppressed comments (4)

packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:384

These checks cover only the metastore database fields, but HiveServer2DeploymentDependent also appends the CR-provided metastore.externalUri, llap.serviceHosts, and zookeeper.quorum to the same SERVICE_OPTS string (at its lines 117-139). Values containing whitespace or other rejected characters can therefore still reach the generated startup command, so the stated validation boundary can be bypassed. Route every CR value used to build SERVICE_OPTS through the same validator, or validate them before constructing it.

    validateOptValue("spec.metastore.database.url", db.url());
    validateOptValue("spec.metastore.database.driver", db.driver());
    validateOptValue("spec.metastore.database.username", db.username());

packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:401

This accepts an empty string, but callers treat any non-null driverJarUrl as configured and externalJars may also contain empty items. The value then falls through to the non-HTTP branch and generates hadoop fs -copyToLocal '', so malformed input still produces a failing init command instead of being rejected here. Reject empty (and null) locations before building the command.

    if (containsUnsafeShellChars(jarUrl)) {
      throw new IllegalArgumentException("external JAR location must not contain whitespace, quotes, backslashes or "
          + "control characters: " + jarUrl);

packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:407

Correct the spelling in this newly added comment: use “spun-up” rather than “spinned up.”

      // and the spinned up pods to enter CrashLoopBackOff.

packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/model/spec/DatabaseConfig.java:30

Adding this CRD pattern changes the user-visible behavior: Kubernetes now rejects an invalid database type at admission instead of accepting the CR and letting reconciliation fail later. That conflicts with the PR description's Does this PR introduce any user-facing change? No; please update the description to document the validation behavior.

    @Pattern("^(derby|mysql|postgres|mssql|oracle)$")

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Redact JAR URLs from validation errors and Ready status · New
Validate all CR values before appending them to SERVICE_OPTS · New

Resolved since last review (2)

SERVICE_OPTS is emitted as a Kubernetes EnvVar, and this code deliberately relies on…
An errored dependent can carry an exception with a null message (for example, `new…

Previously missed (1)

In code that hasn't changed since last review

Fix typo and grammar in the new comment

packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:407

The new comment has a typo and awkward grammar: “spinned” should be “spun,” and the sentence needs a connector before “preventing.”

tanishq-chugh · 2026-09-20T01:36:45Z

+    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);


Addressed in commit: 3cb1889

tanishq-chugh · 2026-09-20T01:36:38Z

+    validateOptValue("spec.metastore.database.url", db.url());
+    validateOptValue("spec.metastore.database.driver", db.driver());
+    validateOptValue("spec.metastore.database.username", db.username());


Addressed in commit: 3cb1889

…zAM envVars

Copilot

Copilot review overview

🔵 Needs a closer look

The changes require final human review because they are too complex or risky for automated approval.

Review effort: Lite
Findings: 1 · 1

Open (2)

Redact JAR URLs from validation errors and Ready status
Validate all CR values before appending them to SERVICE_OPTS

sonarqubecloud · 2026-09-20T07:43:35Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ayushtkn

LGTM

HIVE-29999: HiveCluster CR string fields should be validated before b…

09fa6f4

…eing injected into k8s operator generated bash commands

asf-ci-hive added the tests pending label Sep 18, 2026

ayushtkn requested review from ayushtkn and a lite review from Copilot September 18, 2026 15:35

Copilot started reviewing on behalf of ayushtkn September 18, 2026 15:35 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread ...kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java Outdated

Comment thread ...ubernetes/src/java/org/apache/hive/kubernetes/operator/reconciler/HiveClusterReconciler.java Outdated

tanishq-chugh added 2 commits September 19, 2026 01:07

Address co-pilot comments

9635ad4

Address SonarQube

fa5c965

asf-ci-hive added tests unstable tests pending tests passed and removed tests pending tests unstable labels Sep 18, 2026

ayushtkn requested a lite review from Copilot September 19, 2026 00:59

Copilot started reviewing on behalf of ayushtkn September 19, 2026 01:00 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Add Validation for args being added into HS2's SERVICE_OPTS & LLAP/Te…

3cb1889

…zAM envVars

asf-ci-hive added tests pending and removed tests passed labels Sep 20, 2026

ayushtkn requested a lite review from Copilot September 20, 2026 01:40

Copilot started reviewing on behalf of ayushtkn September 20, 2026 01:40 View session

Copilot AI reviewed Sep 20, 2026

View reviewed changes

asf-ci-hive added tests unstable tests pending and removed tests pending tests unstable labels Sep 20, 2026

asf-ci-hive removed the tests pending label Sep 20, 2026

asf-ci-hive added the tests passed label Sep 20, 2026

ayushtkn approved these changes Sep 21, 2026

View reviewed changes

ayushtkn merged commit 828f668 into apache:master Sep 21, 2026
5 checks passed

Navigation Menu

HIVE-29999: HiveCluster CR string fields should be validated before b… - #6802

HIVE-29999: HiveCluster CR string fields should be validated before b…#6802
ayushtkn merged 4 commits into
apache:masterfrom
tanishq-chugh:op-resc-check

tanishq-chugh commented Sep 18, 2026

Uh oh!

tanishq-chugh commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

tanishq-chugh Sep 20, 2026

Uh oh!

tanishq-chugh Sep 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sonarqubecloud Bot commented Sep 20, 2026

Uh oh!

ayushtkn left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

tanishq-chugh commented Sep 18, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

tanishq-chugh commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

tanishq-chugh Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

tanishq-chugh Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

sonarqubecloud Bot commented Sep 20, 2026

Quality Gate passed

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants