Copilot commented on code in PR #6811:
URL: https://github.com/apache/hive/pull/6811#discussion_r4068733238
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:
##########
@@ -597,6 +629,56 @@ protected static void applyTolerations(PodSpec podSpec,
List<Toleration> tolerat
}
}
+ /**
+ * Applies a restricted Security Context to every pod spec the operator
+ * generates, aligned with Kubernetes Pod Security restricted Standards.
+ */
+ protected static void applyRestrictedSecurityContext(PodSpec podSpec, Long
runAsUser) {
+ if (podSpec.getSecurityContext() == null) {
+ PodSecurityContextBuilder podSc = new PodSecurityContextBuilder()
+ .withRunAsNonRoot(true)
+ .withSeccompProfile(new SeccompProfileBuilder()
+ .withType("RuntimeDefault")
+ .build());
+ if (runAsUser != null) {
+ podSc.withRunAsUser(runAsUser);
+ }
Review Comment:
With the documented raw manifests, `runAsUser` is absent, so this
conditional leaves only `runAsNonRoot=true`. The bundled Hive image declares
`USER hive` (a non-numeric image user), which makes kubelet reject every
generated pod because it cannot verify the user is non-root. Either
default/derive UID 1000 for the bundled image or update the raw examples and
require a UID before enabling this restriction.
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:
##########
@@ -411,6 +419,30 @@ public static void
validateLlapEmbeddedValues(HiveClusterSpec spec, LlapSpec lla
validateOptValue("spec.llapClusters.serviceHosts", llap.serviceHosts());
}
+ /**
+ * Ensures spec.serviceAccountName references a ServiceAccount in the CR
+ * namespace that is explicitly approved, preventing a principal with only
+ * HiveCluster RBAC running pods as a more privileged SA.
+ */
+ public static void validateServiceAccountName(
+ KubernetesClient client, String namespace, String serviceAccountName) {
+ if (serviceAccountName == null || serviceAccountName.isBlank()) {
+ return;
+ }
+ var sa =
client.serviceAccounts().inNamespace(namespace).withName(serviceAccountName).get();
+ if (sa == null) {
+ throw new IllegalArgumentException(
+ "serviceAccountName '" + serviceAccountName + "' not found in
namespace " + namespace);
Review Comment:
The documented non-Helm installation applies
`config/rbac/cluster-role.yaml`, but that manifest was not updated with
permission to GET ServiceAccounts. Any raw-manifest deployment using a
non-empty serviceAccountName will now fail this call with Forbidden, even when
the account has the approval label; update the non-Helm RBAC manifest as part
of this change.
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:
##########
@@ -411,6 +419,30 @@ public static void
validateLlapEmbeddedValues(HiveClusterSpec spec, LlapSpec lla
validateOptValue("spec.llapClusters.serviceHosts", llap.serviceHosts());
}
+ /**
+ * Ensures spec.serviceAccountName references a ServiceAccount in the CR
+ * namespace that is explicitly approved, preventing a principal with only
+ * HiveCluster RBAC running pods as a more privileged SA.
+ */
+ public static void validateServiceAccountName(
+ KubernetesClient client, String namespace, String serviceAccountName) {
+ if (serviceAccountName == null || serviceAccountName.isBlank()) {
+ return;
+ }
Review Comment:
Returning for blank names bypasses the approval check while Kubernetes still
resolves the pod's ServiceAccount to the namespace's `default` account. A
HiveCluster author can therefore omit this field and run every generated
workload as an elevated default account, contrary to the method's stated
protection; validate the effective default account or require an explicitly
approved name.
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/model/spec/RestrictedVolume.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 org.apache.hive.kubernetes.operator.model.spec;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonPropertyDescription;
+import io.fabric8.generator.annotation.Required;
+import io.fabric8.kubernetes.api.model.ConfigMapVolumeSource;
+import io.fabric8.kubernetes.api.model.EmptyDirVolumeSource;
+import io.fabric8.kubernetes.api.model.PersistentVolumeClaimVolumeSource;
+import io.fabric8.kubernetes.api.model.SecretVolumeSource;
+import io.fabric8.kubernetes.api.model.Volume;
+import io.fabric8.kubernetes.api.model.VolumeBuilder;
+
+/**
+ * A restricted pod volume source for HiveCluster specs. Only types that
+ * reference namespaced API objects or pod-local storage are permitted.
+ */
+public record RestrictedVolume(
+ @Required
+ @JsonPropertyDescription("Volume name, referenced by volumeMounts")
+ String name,
+ @JsonPropertyDescription("ConfigMap volume source")
+ ConfigMapVolumeSource configMap,
+ @JsonPropertyDescription("Secret volume source")
+ SecretVolumeSource secret,
+ @JsonPropertyDescription("EmptyDir volume source")
+ EmptyDirVolumeSource emptyDir,
+ @JsonPropertyDescription("PersistentVolumeClaim volume source")
+ PersistentVolumeClaimVolumeSource persistentVolumeClaim) {
+
+ public Volume toKubernetesVolume() {
+ return new VolumeBuilder()
+ .withName(name)
+ .withConfigMap(configMap)
+ .withSecret(secret)
+ .withEmptyDir(emptyDir)
+ .withPersistentVolumeClaim(persistentVolumeClaim)
+ .build();
+ }
Review Comment:
All four source fields are optional and this builder forwards every non-null
one. A CR containing multiple sources (or no source) therefore passes the
generated CRD but produces an invalid Kubernetes Volume, so workload creation
fails only during reconciliation; enforce exactly one source here or express a
oneOf constraint in the CRD.
--
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]
