Gargi-jais11 commented on code in PR #9868: URL: https://github.com/apache/ozone/pull/9868#discussion_r3049180586
########## hadoop-ozone/cli-debug/src/main/java/org/apache/hadoop/ozone/debug/kerberos/DiagnoseSubcommand.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.hadoop.ozone.debug.kerberos; + +import java.io.BufferedWriter; +import java.io.ByteArrayOutputStream; +import java.io.OutputStreamWriter; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.Callable; +import org.apache.hadoop.hdds.cli.AbstractSubcommand; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import picocli.CommandLine; + +/** + * Kerberos diagnostic command for Ozone. + * Usage:Validates each registered probe serially + * and prints diagnostic summary. + * Example: ozone debug kerberos diagnose + */ [email protected](name = "diagnose", + description = "Diagnose Kerberos configuration issues.") +public class DiagnoseSubcommand extends AbstractSubcommand + implements Callable<Integer> { + @Override + public Integer call() throws Exception { + out().println("\n== Ozone Kerberos Diagnostics ==\n"); + OzoneConfiguration conf = getOzoneConf(); + List<DiagnosticProbe> probes = Arrays.asList( + new HostProbe(), + new EnvironmentProbe(), + new JvmKerberosProbe(), + new KerberosConfigProbe(), + new KinitProbe(), + new KeytabProbe(), + new KerberosTicketProbe(), + new PrincipalMappingProbe(), + new OzonePrincipalProbe(), + new SecurityConfigProbe(), + new AuthorizationProbe(), + new HttpAuthProbe()); + + int pass = 0, warn = 0, fail = 0; + + for (DiagnosticProbe probe : probes) { + + out().println("-- " + probe.name() + " --"); + + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + BufferedWriter writer = new BufferedWriter( + new OutputStreamWriter(buffer, StandardCharsets.UTF_8)); Review Comment: A `BufferedWriter` is created that wraps the `ByteArrayOutputStream` buffer. writer is created and closed in finally — that's it. The stream that actually captures probe output is `PrintStream ps`. Only ps is ever used. BufferWriter is a dead code and can be remove Line 67-68 and Line 89. ########## hadoop-ozone/cli-debug/src/main/java/org/apache/hadoop/ozone/debug/kerberos/DiagnoseSubcommand.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.hadoop.ozone.debug.kerberos; + +import java.io.BufferedWriter; +import java.io.ByteArrayOutputStream; +import java.io.OutputStreamWriter; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.Callable; +import org.apache.hadoop.hdds.cli.AbstractSubcommand; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import picocli.CommandLine; + +/** + * Kerberos diagnostic command for Ozone. + * Usage:Validates each registered probe serially + * and prints diagnostic summary. + * Example: ozone debug kerberos diagnose + */ [email protected](name = "diagnose", + description = "Diagnose Kerberos configuration issues.") +public class DiagnoseSubcommand extends AbstractSubcommand + implements Callable<Integer> { + @Override + public Integer call() throws Exception { + out().println("\n== Ozone Kerberos Diagnostics ==\n"); + OzoneConfiguration conf = getOzoneConf(); + List<DiagnosticProbe> probes = Arrays.asList( + new HostProbe(), + new EnvironmentProbe(), + new JvmKerberosProbe(), + new KerberosConfigProbe(), + new KinitProbe(), + new KeytabProbe(), + new KerberosTicketProbe(), + new PrincipalMappingProbe(), + new OzonePrincipalProbe(), + new SecurityConfigProbe(), + new AuthorizationProbe(), + new HttpAuthProbe()); + + int pass = 0, warn = 0, fail = 0; + + for (DiagnosticProbe probe : probes) { + + out().println("-- " + probe.name() + " --"); + + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + BufferedWriter writer = new BufferedWriter( + new OutputStreamWriter(buffer, StandardCharsets.UTF_8)); + + PrintStream ps = new PrintStream( + buffer, true, java.nio.charset.StandardCharsets.UTF_8.name()); + PrintStream oldOut = System.out; + PrintStream oldErr = System.err; + + System.setOut(ps); + System.setErr(ps); + + boolean valid = false; + String output; + try { + valid = probe.test(conf); + } catch (Throwable t) { + t.printStackTrace(System.err); + System.err.println("ERROR: Probe execution failed: " + t.getMessage()); + } finally { + System.setOut(oldOut); + System.setErr(oldErr); + ps.close(); + writer.close(); + } + + output = new String(buffer.toByteArray(), StandardCharsets.UTF_8); + out().print(output); + + if (output.contains("ERROR:")) { + fail++; + out().println("[FAIL] " + probe.name()); + } else if (!valid) { + warn++; + out().println("[WARN] " + probe.name()); + } else { + pass++; + out().println("[PASS] " + probe.name()); + } + out().println(); + } Review Comment: Right now **FAIL** is infferred with output.contains("ERROR:") while **WARN/PASS** use `probe.test()`’s boolean return. Those two signals can disagree (e.g. a probe prints ERROR: but still returns success, or the opposite). It is better to have one source of truth and drop the string scan. Right Approach would be: **step1:** create them as an enum ``` public enum ProbeResult { PASS, // probe ran cleanly, everything looks healthy WARN, // probe ran, something is misconfigured but not broken FAIL // probe detected a critical problem } ``` **Step2:** Update the `DiagnosticProbe` interface to: ``` ProbeResult test(OzoneConfiguration conf) throws Exception; // was: boolean ``` **step3:** then you can Update DiagnoseSubcommand — remove the string scan entirely and use the Enum created **step4:** Update probes to return ProbeResult explicitly Here are examples of all three cases: > Purely informational probe (was always return true) — EnvironmentProbe > WARN probe (suboptimal but not broken) — AuthorizationProbe > FAIL probe (critical, system broken) — KerberosConfigProbe > A probe with both WARN and FAIL paths — KerberosTicketProbe ########## hadoop-ozone/cli-debug/src/main/java/org/apache/hadoop/ozone/debug/kerberos/TranslatePrincipalSubcommand.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.hadoop.ozone.debug.kerberos; + +import java.util.List; +import java.util.concurrent.Callable; +import org.apache.hadoop.hdds.cli.AbstractSubcommand; +import org.apache.hadoop.security.authentication.util.KerberosName; +import picocli.CommandLine; + +/** + * Debug command to translate one or more Kerberos principals into local user names + * using the configured auth_to_local rules. + * + * Example: + * ozone debug kerberos translate-principal testuser/[email protected] + */ [email protected]( + name = "translate-principal", + description = "Translate Kerberos principal(s) using auth_to_local rules." +) +public class TranslatePrincipalSubcommand extends AbstractSubcommand + implements Callable<Void> { + + @CommandLine.Parameters(arity = "1..*", + description = "Kerberos principal(s) to translate" + ) + private List<String> principals; + + @Override + public Void call() throws Exception { + + System.out.println("\n== Kerberos Principal Translation ==\n"); + + String rules = getOzoneConf() + .getTrimmed("hadoop.security.auth_to_local", "DEFAULT"); + System.out.println("auth_to_local rules = " + rules); + + KerberosName.setRules(rules); + for (String principal : principals) { + try { + KerberosName kerbName = new KerberosName(principal); + String shortName = kerbName.getShortName(); + System.out.println(String.format( + "Principal = %s to Local user = %s", principal, shortName)); + } catch (Exception e) { + System.err.println("ERROR: Failed to translate principal " + + principal + " : " + e.getMessage()); + } + } + return null; Review Comment: This subcommand impliments `Callable<Void> ` and always returns null (**exit code 0)**, even when a principal fails t o translates. It should be `Callable<Integer>` and return **a non-zero code** if any principal translation fails — consistent with how `DiagnoseSubcommand `which returns 1 on failures. -- 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]
