Skip to content

Commit 9101afc

Browse files
Darshan3690pmbrullclaude
authored andcommitted
Fix omjob pod/label naming length constraints (#27143)
* Fix omjob pod/label naming length constraints - Fix SHA-256 hash byte formatting with & 0xff mask for proper 2-hex-digit encoding per byte - Enforce Kubernetes 63-character label limit via hash-based truncation - Extract shared hash utility to HashUtils - Add comprehensive tests for truncation, uniqueness, and edge cases Fixes #27004 * Address review: fix hash bounds, add edge case tests, remove redundant substring - Guard ensureValidLabelValue fallback against StringIndexOutOfBounds - Add tests for separator-only inputs exercising the fallback path - Remove redundant .substring() since HashUtils.hash() already returns 6 chars - Use 253 (K8s DNS subdomain limit) instead of 260 in PodManagerTest - Fix wrong assertion in LabelBuilderTest (podSelector has 2 entries) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: complete all code review issues for PR merge - Add HTTP 409 idempotency handling in TableClass.create() for sharded Playwright tests - Apply Java Spotless formatting to fix checkstyle violations - Apply Playwright formatting: organize-imports, eslint --fix, prettier --write - Resolve all 10 code review findings: ✅ StringIndexOutOfBoundsException in hash truncation (already fixed) ✅ Redundant substring operation (already fixed) ✅ Duplicate hash code extraction (already done) ✅ Playwright 409 conflict handling (now added) ✅ Java formatting compliance (now applied) ✅ TypeScript formatting compliance (now applied) PR is now ready for merge with all CI checks expected to pass. --------- Co-authored-by: Pere Miquel Brull <peremiquelbrull@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e0af46c commit 9101afc

9 files changed

Lines changed: 440 additions & 26 deletions

File tree

openmetadata-k8s-operator/src/main/java/org/openmetadata/operator/controller/CronOMJobReconciler.java

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.openmetadata.operator.model.CronOMJobStatus;
4444
import org.openmetadata.operator.model.OMJobResource;
4545
import org.openmetadata.operator.model.OMJobSpec;
46+
import org.openmetadata.operator.util.KubernetesNameBuilder;
4647
import org.slf4j.Logger;
4748
import org.slf4j.LoggerFactory;
4849

@@ -51,6 +52,7 @@ public class CronOMJobReconciler
5152
implements Reconciler<CronOMJobResource>, ErrorStatusHandler<CronOMJobResource> {
5253

5354
private static final Logger LOG = LoggerFactory.getLogger(CronOMJobReconciler.class);
55+
private static final int MAX_OMJOB_NAME_LENGTH = 63;
5456
private final Duration defaultRequeue;
5557
private final OperatorConfig config;
5658

@@ -166,28 +168,27 @@ public UpdateControl<CronOMJobResource> reconcile(
166168

167169
private OMJobResource buildOMJob(CronOMJobResource cronOMJob, Instant scheduledTime) {
168170
String baseName = cronOMJob.getMetadata().getName();
169-
String name = baseName + "-" + scheduledTime.getEpochSecond();
170-
if (name.length() > 253) {
171-
name = name.substring(0, 253);
172-
}
171+
String name = buildScheduledOMJobName(baseName, scheduledTime);
173172

174173
final String finalName = name; // Make final for use in lambda
175174

176175
// Generate unique run ID for this execution
177176
String runId = UUID.randomUUID().toString();
177+
HashMap<String, String> labels =
178+
cronOMJob.getMetadata().getLabels() != null
179+
? new HashMap<>(cronOMJob.getMetadata().getLabels())
180+
: new HashMap<>();
178181

179182
ObjectMeta metadata =
180183
new ObjectMetaBuilder()
181184
.withName(name)
182185
.withNamespace(cronOMJob.getMetadata().getNamespace())
183-
.withLabels(cronOMJob.getMetadata().getLabels())
186+
.withLabels(labels)
184187
.withAnnotations(cronOMJob.getMetadata().getAnnotations())
185188
.build();
186189

187190
// Add run ID to labels for tracking
188-
if (metadata.getLabels() != null) {
189-
metadata.getLabels().put("app.kubernetes.io/run-id", runId);
190-
}
191+
metadata.getLabels().put("app.kubernetes.io/run-id", runId);
191192

192193
OMJobResource omJob = new OMJobResource();
193194
omJob.setMetadata(metadata);
@@ -239,6 +240,17 @@ private OMJobResource buildOMJob(CronOMJobResource cronOMJob, Instant scheduledT
239240
return omJob;
240241
}
241242

243+
private String buildScheduledOMJobName(String baseName, Instant scheduledTime) {
244+
String suffix = "-" + scheduledTime.getEpochSecond();
245+
int maxBaseLength = MAX_OMJOB_NAME_LENGTH - suffix.length();
246+
247+
if (maxBaseLength < 1) {
248+
throw new IllegalArgumentException("Scheduled OMJob suffix leaves no room for base name");
249+
}
250+
251+
return KubernetesNameBuilder.fitNameWithHash(baseName, maxBaseLength, "omjob") + suffix;
252+
}
253+
242254
private OMJobSpec deepCopyOMJobSpec(OMJobSpec source, String runId, String jobName) {
243255
if (source == null) {
244256
return null;

openmetadata-k8s-operator/src/main/java/org/openmetadata/operator/service/PodManager.java

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.openmetadata.operator.model.OMJobSpec;
3838
import org.openmetadata.operator.model.OMJobStatus;
3939
import org.openmetadata.operator.util.EnvVarUtils;
40+
import org.openmetadata.operator.util.KubernetesNameBuilder;
4041
import org.openmetadata.operator.util.LabelBuilder;
4142
import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatusType;
4243
import org.slf4j.Logger;
@@ -51,6 +52,7 @@ public class PodManager {
5152

5253
private static final Logger LOG = LoggerFactory.getLogger(PodManager.class);
5354
private static final String PIPELINE_STATUS = "pipelineStatus";
55+
private static final int MAX_POD_NAME_LENGTH = 253;
5456

5557
private final KubernetesClient client;
5658

@@ -425,11 +427,35 @@ private Optional<Pod> findPod(OMJobResource omJob, Map<String, String> selector)
425427
}
426428

427429
private String generateMainPodName(OMJobResource omJob) {
428-
return omJob.getMetadata().getName() + "-main";
430+
return buildPodName(omJob, "-main");
429431
}
430432

431433
private String generateExitHandlerPodName(OMJobResource omJob) {
432-
return omJob.getMetadata().getName() + "-exit";
434+
return buildPodName(omJob, "-exit");
435+
}
436+
437+
private String buildPodName(OMJobResource omJob, String suffix) {
438+
// Kubernetes pod names can be up to 253 characters (DNS subdomain limit).
439+
// The OMJob name label is derived from omJob.getMetadata().getName()
440+
// and sanitized by LabelBuilder to satisfy the separate 63-character label limit.
441+
// It is not derived from the generated pod name.
442+
String baseName = omJob.getMetadata().getName();
443+
444+
// Account for suffix length when calculating max base name length.
445+
int maxBaseLength = MAX_POD_NAME_LENGTH - suffix.length();
446+
String podName =
447+
KubernetesNameBuilder.fitNameWithHash(baseName, maxBaseLength, "omjob") + suffix;
448+
449+
// Log a warning if the original name was truncated to fit the DNS subdomain limit.
450+
if (!podName.equals(omJob.getMetadata().getName() + suffix)) {
451+
LOG.warn(
452+
"Pod name for OMJob {} was truncated from {} to {} to comply with Kubernetes DNS name length limits",
453+
omJob.getMetadata().getName(),
454+
omJob.getMetadata().getName() + suffix,
455+
podName);
456+
}
457+
458+
return podName;
433459
}
434460

435461
private String determinePipelineStatus(OMJobResource omJob) {
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/*
2+
* Copyright 2021 Collate.
3+
* Licensed under the Apache License, Version 2.0 (the "License");
4+
* you may not use this file except in compliance with the License.
5+
* You may obtain a copy of the License at
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
* Unless required by applicable law or agreed to in writing, software
8+
* distributed under the License is distributed on an "AS IS" BASIS,
9+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
10+
* See the License for the specific language governing permissions and
11+
* limitations under the License.
12+
*/
13+
14+
package org.openmetadata.operator.util;
15+
16+
import java.nio.charset.StandardCharsets;
17+
import java.security.MessageDigest;
18+
import java.security.NoSuchAlgorithmException;
19+
20+
public class HashUtils {
21+
private static final int HASH_LENGTH = 6;
22+
23+
private HashUtils() {
24+
// Utility class
25+
}
26+
27+
/**
28+
* Generates a 6-character SHA-256 hash of the input string for deterministic name
29+
* truncation.
30+
*
31+
* @param input the string to hash
32+
* @return a 6-character hex string
33+
* @throws IllegalArgumentException if SHA-256 is not available
34+
*/
35+
public static String hash(String input) {
36+
try {
37+
MessageDigest digest = MessageDigest.getInstance("SHA-256");
38+
byte[] encoded = digest.digest(input.getBytes(StandardCharsets.UTF_8));
39+
StringBuilder hex = new StringBuilder();
40+
for (byte value : encoded) {
41+
hex.append(String.format("%02x", value & 0xff));
42+
}
43+
return hex.substring(0, HASH_LENGTH);
44+
} catch (NoSuchAlgorithmException e) {
45+
throw new IllegalArgumentException("SHA-256 algorithm not available", e);
46+
}
47+
}
48+
}
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
/*
2+
* Copyright 2021 Collate
3+
* Licensed under the Apache License, Version 2.0 (the "License");
4+
* you may not use this file except in compliance with the License.
5+
* You may obtain a copy of the License at
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
* Unless required by applicable law or agreed to in writing, software
8+
* distributed under the License is distributed on an "AS IS" BASIS,
9+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
10+
* See the License for the specific language governing permissions and
11+
* limitations under the License.
12+
*/
13+
14+
package org.openmetadata.operator.util;
15+
16+
/**
17+
* Utility for building deterministic Kubernetes resource names within a length limit.
18+
*/
19+
public final class KubernetesNameBuilder {
20+
21+
private static final int HASH_LENGTH = 6;
22+
23+
private KubernetesNameBuilder() {
24+
// Utility class
25+
}
26+
27+
public static String fitNameWithHash(
28+
String baseName, int maxBaseLength, String fallbackBaseName) {
29+
if (maxBaseLength < 1) {
30+
throw new IllegalArgumentException(
31+
"Kubernetes resource name must allow at least one character");
32+
}
33+
34+
if (fallbackBaseName == null || fallbackBaseName.isEmpty()) {
35+
throw new IllegalArgumentException("fallbackBaseName must not be null or empty");
36+
}
37+
38+
String candidate = baseName == null ? "" : baseName;
39+
if (candidate.length() > maxBaseLength) {
40+
String hash = HashUtils.hash(candidate).substring(0, Math.min(HASH_LENGTH, maxBaseLength));
41+
int maxPrefixLength = maxBaseLength - hash.length() - 1;
42+
43+
if (maxPrefixLength > 0) {
44+
String prefix = trimTrailingSeparators(candidate.substring(0, maxPrefixLength));
45+
if (!prefix.isEmpty()) {
46+
return prefix + "-" + hash;
47+
}
48+
49+
String fallbackPrefix =
50+
trimTrailingSeparators(
51+
fallbackBaseName.substring(
52+
0, Math.min(fallbackBaseName.length(), maxPrefixLength)));
53+
if (!fallbackPrefix.isEmpty()) {
54+
return fallbackPrefix + "-" + hash;
55+
}
56+
}
57+
58+
return hash;
59+
}
60+
61+
if (candidate.isEmpty()) {
62+
candidate = fallbackBaseName;
63+
}
64+
65+
if (candidate.length() > maxBaseLength) {
66+
throw new IllegalStateException(
67+
"Fallback name exceeds max base length (" + maxBaseLength + ")");
68+
}
69+
70+
return candidate;
71+
}
72+
73+
private static String trimTrailingSeparators(String value) {
74+
return value.replaceAll("[^a-z0-9]+$", "");
75+
}
76+
}

openmetadata-k8s-operator/src/main/java/org/openmetadata/operator/util/LabelBuilder.java

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
*/
2626
public class LabelBuilder {
2727

28+
private static final int MAX_LABEL_VALUE_LENGTH = 63;
29+
2830
// Standard Kubernetes labels
2931
public static final String LABEL_APP_NAME = "app.kubernetes.io/name";
3032
public static final String LABEL_APP_COMPONENT = "app.kubernetes.io/component";
@@ -56,7 +58,7 @@ public static Map<String, String> buildBaseLabels(OMJobResource omJob) {
5658
labels.put(LABEL_APP_NAME, APP_NAME);
5759
labels.put(LABEL_APP_COMPONENT, COMPONENT_INGESTION);
5860
labels.put(LABEL_APP_MANAGED_BY, MANAGED_BY_OMJOB_OPERATOR);
59-
labels.put(LABEL_OMJOB_NAME, omJob.getMetadata().getName());
61+
labels.put(LABEL_OMJOB_NAME, buildOMJobNameLabelValue(omJob));
6062

6163
// Copy pipeline and run-id from OMJob labels
6264
String pipelineName = omJob.getPipelineName();
@@ -113,10 +115,18 @@ public static Map<String, String> buildExitHandlerLabels(OMJobResource omJob) {
113115
*/
114116
public static Map<String, String> buildPodSelector(OMJobResource omJob) {
115117
Map<String, String> selector = new HashMap<>();
116-
selector.put(LABEL_OMJOB_NAME, omJob.getMetadata().getName());
118+
selector.put(LABEL_OMJOB_NAME, buildOMJobNameLabelValue(omJob));
119+
selector.put(LABEL_APP_MANAGED_BY, MANAGED_BY_OMJOB_OPERATOR);
117120
return selector;
118121
}
119122

123+
/**
124+
* Build the OMJob name label value used by operator-managed pods.
125+
*/
126+
public static String buildOMJobNameLabelValue(OMJobResource omJob) {
127+
return sanitizeLabelValue(omJob.getMetadata().getName());
128+
}
129+
120130
/**
121131
* Build selector for finding main pod
122132
*/
@@ -143,10 +153,51 @@ public static String sanitizeLabelValue(String value) {
143153
return "";
144154
}
145155

146-
// Replace invalid characters with hyphens and truncate to 63 chars
147-
String sanitized =
148-
value.replaceAll("[^a-zA-Z0-9\\-_.]", "-").replaceAll("-+", "-").replaceAll("^-|-$", "");
156+
// Replace invalid characters with hyphens and collapse repeated hyphens.
157+
// We intentionally keep dots and underscores, as they are allowed in label values.
158+
String sanitized = value.replaceAll("[^a-zA-Z0-9\\-_.]", "-").replaceAll("-+", "-");
159+
160+
if (sanitized.length() <= MAX_LABEL_VALUE_LENGTH) {
161+
// Even for short values, ensure we respect the Kubernetes rule that
162+
// label values must start and end with an alphanumeric character.
163+
return ensureValidLabelValue(sanitized, value);
164+
}
165+
166+
// For long values, preserve uniqueness by appending a short hash while
167+
// keeping the overall value within the 63-character limit.
168+
String hash = HashUtils.hash(value);
169+
int maxPrefixLength = MAX_LABEL_VALUE_LENGTH - hash.length() - 1;
170+
String prefix = sanitized.substring(0, maxPrefixLength);
171+
172+
String candidate = prefix + "-" + hash;
173+
return ensureValidLabelValue(candidate, value);
174+
}
175+
176+
/**
177+
* Ensure the label value starts and ends with an alphanumeric character.
178+
* If trimming non-alphanumeric characters from the edges results in an
179+
* empty string, fall back to a hash-based value derived from the original
180+
* input so that the label remains valid and stable.
181+
*/
182+
private static String ensureValidLabelValue(String candidate, String original) {
183+
String result = candidate.replaceAll("^[^a-zA-Z0-9]+", "").replaceAll("[^a-zA-Z0-9]+$", "");
184+
185+
if (!result.isEmpty() && result.length() <= MAX_LABEL_VALUE_LENGTH) {
186+
return result;
187+
}
188+
189+
// Fallback: build a short, deterministic value based on a hash of the
190+
// original input. This guarantees the output is non-empty, starts/ends
191+
// with an alphanumeric character, and fits within the label limit.
192+
int maxHashLength = Math.max(1, MAX_LABEL_VALUE_LENGTH - 3); // Reserve for "om-"
193+
String fullHash = HashUtils.hash(original);
194+
String hash = fullHash.substring(0, Math.min(maxHashLength, fullHash.length()));
195+
String fallback = "om-" + hash;
196+
197+
if (fallback.length() > MAX_LABEL_VALUE_LENGTH) {
198+
fallback = fallback.substring(0, MAX_LABEL_VALUE_LENGTH);
199+
}
149200

150-
return sanitized.length() > 63 ? sanitized.substring(0, 63) : sanitized;
201+
return fallback;
151202
}
152203
}

0 commit comments

Comments
 (0)