Skip to content

HADOOP-19342. SaslRpcServer.AuthMethod print INFO messages in client side #7173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
szetszwo wants to merge 4 commits into from
Closed

HADOOP-19342. SaslRpcServer.AuthMethod print INFO messages in client side #7173

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4e04b6
HADOOP-19342. SaslRpcServer.AuthMethod print INFO messages in client …
szetszwo Nov 18, 2024
c2b6ab4
Avoid using Guava.
szetszwo Nov 19, 2024
5d3699f
Fix checkstyle.
szetszwo Nov 19, 2024
6d001e5
Address review comments.
szetszwo Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions ...-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslMechanismFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ public final class SaslMechanismFactory {
static final Logger LOG = LoggerFactory.getLogger(SaslMechanismFactory.class);

private static final String SASL_MECHANISM_ENV = "HADOOP_SASL_MECHANISM";
private static final String SASL_MECHANISM;
private static volatile String mechanism;

static {
private static synchronized String getSynchronously() {
// env
final String envValue = System.getenv(SASL_MECHANISM_ENV);
LOG.debug("{} = {} (env)", SASL_MECHANISM_ENV, envValue);

// conf
final Configuration conf = new Configuration(false);
final Configuration conf = new Configuration();
final String confValue = conf.get(HADOOP_SECURITY_SASL_MECHANISM_KEY,
HADOOP_SECURITY_SASL_MECHANISM_DEFAULT);
LOG.debug("{} = {} (conf)", HADOOP_SECURITY_SASL_MECHANISM_KEY, confValue);
Expand All @@ -57,14 +57,16 @@ public final class SaslMechanismFactory {
}
}

SASL_MECHANISM = envValue != null ? envValue
mechanism = envValue != null ? envValue
: confValue != null ? confValue
: HADOOP_SECURITY_SASL_MECHANISM_DEFAULT;
LOG.debug("SASL_MECHANISM = {} (effective)", SASL_MECHANISM);
LOG.debug("SASL_MECHANISM = {} (effective)", mechanism);
return mechanism;
}

public static String getMechanism() {
return SASL_MECHANISM;
final String value = mechanism;
return value != null ? value : getSynchronously();
}

public static boolean isDefaultMechanism(String mechanism) {
Expand Down
15 changes: 10 additions & 5 deletions ...-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslRpcServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

import javax.security.auth.callback.Callback;
import javax.security.auth.callback.CallbackHandler;
Expand Down Expand Up @@ -225,18 +226,22 @@ public enum AuthMethod {
SIMPLE((byte) 80, ""),
KERBEROS((byte) 81, "GSSAPI"),
@Deprecated
DIGEST((byte) 82, SaslMechanismFactory.getMechanism()),
TOKEN((byte) 82, SaslMechanismFactory.getMechanism()),
DIGEST((byte) 82, SaslMechanismFactory::getMechanism),
TOKEN((byte) 82, SaslMechanismFactory::getMechanism),
PLAIN((byte) 83, "PLAIN");

/** The code for this method. */
public final byte code;
public final String mechanismName;
private final Supplier<String> mechanismName;

private AuthMethod(byte code, String mechanismName) {
this(code, () -> mechanismName);
}

AuthMethod(byte code, Supplier<String> mechanismName) {
this.code = code;
this.mechanismName = mechanismName;
LOG.info("{} {}: code={}, mechanism=\"{}\"",
LOG.debug("{} {}: code={}, mechanism=\"{}\"",
Copy link
Contributor

@stoty stoty Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT this ultimately gets called at roughly the same point during client initialization as previously, it just doesn't show up because the debug level has been reduced to DEBUG from INFO.

Copy link
Contributor

@stoty stoty Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So apart from the log level the only difference is that this will pick up changes in the config
if a new SaslRPCServer is created at a later time ?

Copy link
Contributor

@steveloughran steveloughran Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you aren't using SASL stuff saslMechanismFactory::getMechanism doesn't get invoked.

Copy link
Contributor Author

@szetszwo szetszwo Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... this will pick up changes in the config if a new SaslRPCServer is created at a later time ?

It will load the config only when SaslMechanismFactory.getMechanism is called. So, it won't be loaded with UGI.

getClass().getSimpleName(), name(), code, mechanismName);
}

Expand All @@ -253,7 +258,7 @@ private static AuthMethod valueOf(byte code) {
* @return mechanismName.
*/
public String getMechanismName() {
return mechanismName;
return mechanismName.get();
}

/**
Expand Down
73 changes: 73 additions & 0 deletions ...common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestAuthMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* 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.security;

import org.apache.hadoop.conf.Configuration;
Copy link
Contributor

@steveloughran steveloughran Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: split out org.apache and put in a separate block below

import org.apache.hadoop.test.GenericTestUtils;
import org.junit.Assert;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.slf4j.event.Level;

import java.io.OutputStream;
import java.net.URLDecoder;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;

import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_SASL_MECHANISM_KEY;

/** Test {@link SaslRpcServer.AuthMethod}. */
public class TestAuthMethod {
Copy link
Contributor

@steveloughran steveloughran Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extends AbstractHadoopTestBase for timeouts, thread names etc

@Test
public void testConfiguration() throws Exception {
final Logger log = LoggerFactory.getLogger("org.apache.hadoop.security.SaslMechanismFactory");
GenericTestUtils.setLogLevel(log, Level.TRACE);

// init the mechanism
final String mechanism = "TEST-MECHANISM";
initConf(mechanism, log);

// test get from new conf
final Configuration conf = new Configuration();
final String confValue = conf.get(HADOOP_SECURITY_SASL_MECHANISM_KEY);
log.info("getConf: {} = {}", HADOOP_SECURITY_SASL_MECHANISM_KEY, confValue);
Assert.assertEquals(mechanism, confValue);

// test AuthMethod
Assert.assertEquals(mechanism, SaslRpcServer.AuthMethod.TOKEN.getMechanismName());

// the mechanism won't change after initialized
final String newMechanism = "NEW-MECHANISM";
initConf(newMechanism, log);
Assert.assertEquals(mechanism, SaslRpcServer.AuthMethod.TOKEN.getMechanismName());
}

static void initConf(String mechanism, Logger log) throws Exception {
final Configuration conf = new Configuration();
conf.set(HADOOP_SECURITY_SASL_MECHANISM_KEY, mechanism);
log.info("setConf: {} = {}", HADOOP_SECURITY_SASL_MECHANISM_KEY, mechanism);

final String coreSite = URLDecoder.decode(conf.getResource("core-site.xml").getPath(), "UTF-8");
try (OutputStream out = Files.newOutputStream(Paths.get(coreSite), StandardOpenOption.CREATE)) {
conf.writeXml(out);
}
Configuration.addDefaultResource(coreSite);
Copy link
Contributor

@steveloughran steveloughran Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is going to contaminate all tests which follow in the same JVM, and make debugging conf problems a lot harder.

what about

  • create a new xml conf resource adjacent to core-site.xml
  • give it the name of this test
  • modify this method so if mechanism == null, the conf option is not set.
  • after the test, you could set to being empty XML. (then load a configuration to make sure it still loads)
Configuration conf = new Configuration(false)
if (mechanism != null) conf.set(HADOOP_SECURITY_SASL_MECHANISM_KEY, mechanism);

String newResource = /* work this out */
conf.write(new resource)

Configuration.addDefaultResource(newResource);

and in and @after method, use initConf(null) to reset it.

Avoids overwriting core-site.xml

Copy link
Contributor Author

@szetszwo szetszwo Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that it contaminates all tests.

Tried multiple things but not working very well. One problem is that Configuration has addDefaultResource but not removeDefaultResource. Let skip the test.

log.info("writeXml: {}", coreSite);
}
}