opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.14' into stable-2.15

Browse Source 
* stable-2.14:
  Update git submodules
  Update git submodules
  ldap.Helper: Use local logger and make logger in LdapRealm private
  Remove ValidationError#createLoggerSink to avoid passing around loggers
  LdapLoginServlet: Improve exception handling
  OperatingSystemMXBeanProvider: Log exception for ReflectiveOperationException
  WorkQueue: Don't fail when queue metric already exists
  WorkQueue: Sanitize metric name when queue is created
  DropWizardMetricMaker: Introduce method to sanitize metric name

Change-Id: I4729d537aeb5ef934fcae90b610e28966a6ada9a
This commit is contained in:
David Pursehouse 2018-05-18 10:31:18 +09:00
commit a17c4f558f
12 changed files with 103 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java
View File

@ -30,6 +30,7 @@ import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AccountUserNameException;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.account.AuthResult;
import com.google.gerrit.server.account.AuthenticationFailedException;
import com.google.gerrit.server.auth.AuthenticationUnavailableException;
import com.google.gwtexpui.server.CacheHeaders;
import com.google.inject.Inject;
@ -126,10 +127,16 @@ class LdapLoginServlet extends HttpServlet {
} catch (AuthenticationUnavailableException e) {
sendForm(req, res, "Authentication unavailable at this time.");
return;
} catch (AccountException e) {
log.info(String.format("'%s' failed to sign in: %s", username, e.getMessage()));
} catch (AuthenticationFailedException e) {
// This exception is thrown if the user provided wrong credentials, we don't need to log a
// stacktrace for it.
log.warn("'{}' failed to sign in: {}", username, e.getMessage());
sendForm(req, res, "Invalid username or password.");
return;
} catch (AccountException e) {
log.warn("'{}' failed to sign in", username, e);
sendForm(req, res, "Authentication failed.");
return;
} catch (RuntimeException e) {
log.error("LDAP authentication failed", e);
sendForm(req, res, "Authentication unavailable at this time.");

3
gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java
View File

@ -30,7 +30,6 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class AllProjectsConfig extends VersionedMetaDataOnInit {
private static final Logger log = LoggerFactory.getLogger(AllProjectsConfig.class);
private Config cfg;
@ -65,7 +64,7 @@ public class AllProjectsConfig extends VersionedMetaDataOnInit {
return GroupList.parse(
new Project.NameKey(project),
readUTF8(GroupList.FILE_NAME),
GroupList.createLoggerSink(GroupList.FILE_NAME, log));
error -> log.error("Error parsing file {}: {}", GroupList.FILE_NAME, error.getMessage()));
}
public void save(String pluginName, String message) throws IOException, ConfigInvalidException {

19
gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java
View File

@ -344,6 +344,25 @@ public class DropWizardMetricMaker extends MetricMaker {
METRIC_NAME_PATTERN.pattern());
}
public static String sanitizeMetricName(String input) {
if (METRIC_NAME_PATTERN.matcher(input).matches()) {
return input;
}
String first = input.substring(0, 1).replaceFirst("[^\\w-]", "_");
if (input.length() == 1) {
return first;
}
String result = first + input.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_");
if (result.endsWith("/")) {
result = result.substring(0, result.length() - 1);
}
return result;
}
static String name(Description.FieldOrdering ordering, String codeName, String fieldValues) {
if (ordering == FieldOrdering.PREFIX_FIELDS_BASENAME) {
int s = codeName.lastIndexOf('/');

2
gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java
View File

@ -41,7 +41,7 @@ class OperatingSystemMXBeanProvider {
return new OperatingSystemMXBeanProvider(sys);
}
} catch (ReflectiveOperationException e) {
log.debug(String.format("No implementation for %s: %s", name, e.getMessage()));
log.debug("No implementation for {}", name, e);
}
}
log.warn("No implementation of UnixOperatingSystemMXBean found");

8
gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java
View File

@ -17,8 +17,6 @@ package com.google.gerrit.server.account;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.git.DestinationList;
import com.google.gerrit.server.git.TabFile;
import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.git.VersionedMetaData;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
@ -62,8 +60,10 @@ public class VersionedAccountDestinations extends VersionedMetaData {
String path = p.path;
if (path.startsWith(prefix)) {
String label = path.substring(prefix.length());
ValidationError.Sink errors = TabFile.createLoggerSink(path, log);
destinations.parseLabel(label, readUTF8(path), errors);
destinations.parseLabel(
label,
readUTF8(path),
error -> log.error("Error parsing file {}: {}", path, error.getMessage()));
}
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java
View File

@ -52,7 +52,9 @@ public class VersionedAccountQueries extends VersionedMetaData {
protected void onLoad() throws IOException, ConfigInvalidException {
queryList =
QueryList.parse(
readUTF8(QueryList.FILE_NAME), QueryList.createLoggerSink(QueryList.FILE_NAME, log));
readUTF8(QueryList.FILE_NAME),
error ->
log.error("Error parsing file {}: {}", QueryList.FILE_NAME, error.getMessage()));
}
@Override

16
gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
View File

@ -52,9 +52,13 @@ import javax.security.auth.Subject;
import javax.security.auth.login.LoginContext;
import javax.security.auth.login.LoginException;
import org.eclipse.jgit.lib.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
class Helper {
private static final Logger log = LoggerFactory.getLogger(Helper.class);
static final String LDAP_UUID = "ldap:";
private final Cache<String, ImmutableSet<String>> parentGroups;
@ -151,7 +155,7 @@ class Helper {
} catch (PrivilegedActionException e) {
Throwables.throwIfInstanceOf(e.getException(), NamingException.class);
Throwables.throwIfInstanceOf(e.getException(), RuntimeException.class);
LdapRealm.log.warn("Internal error", e.getException());
log.warn("Internal error", e.getException());
return null;
} finally {
ctx.logout();
@ -296,7 +300,7 @@ class Helper {
}
}
} catch (NamingException e) {
LdapRealm.log.warn("Could not find group " + groupDN, e);
log.warn("Could not find group {}", groupDN, e);
}
cachedParentsDNs = dns.build();
parentGroups.put(groupDN, cachedParentsDNs);
@ -429,10 +433,10 @@ class Helper {
try {
return LdapType.guessType(ctx);
} catch (NamingException e) {
LdapRealm.log.warn(
"Cannot discover type of LDAP server at "
+ server
+ ", assuming the server is RFC 2307 compliant.",
log.warn(
"Cannot discover type of LDAP server at {},"
+ " assuming the server is RFC 2307 compliant.",
server,
e);
return LdapType.RFC_2307;
}

3
gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java
View File

@ -60,7 +60,8 @@ import org.slf4j.LoggerFactory;
@Singleton
class LdapRealm extends AbstractRealm {
static final Logger log = LoggerFactory.getLogger(LdapRealm.class);
private static final Logger log = LoggerFactory.getLogger(LdapRealm.class);
static final String LDAP = "com.sun.jndi.ldap.LdapCtxFactory";
static final String USERNAME = "username";

5
gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java
View File

@ -23,7 +23,6 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.slf4j.Logger;
public class TabFile {
public interface Parser {
@ -145,8 +144,4 @@ public class TabFile {
}
return r.toString();
}
public static ValidationError.Sink createLoggerSink(String file, Logger log) {
return ValidationError.createLoggerSink("Error parsing file " + file + ": ", log);
}
}

11
gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java
View File

@ -14,8 +14,6 @@
package com.google.gerrit.server.git;
import org.slf4j.Logger;
/** Indicates a problem with Git based data. */
public class ValidationError {
private final String message;
@ -44,13 +42,4 @@ public class ValidationError {
public interface Sink {
void error(ValidationError error);
}
public static Sink createLoggerSink(String message, Logger log) {
return new ValidationError.Sink() {
@Override
public void error(ValidationError error) {
log.error(message + error.getMessage());
}
};
}
}

14
gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
View File

@ -14,6 +14,8 @@
package com.google.gerrit.server.git;
import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName;
import com.google.common.base.CaseFormat;
import com.google.common.base.Supplier;
import com.google.gerrit.extensions.events.LifecycleListener;
@ -220,7 +222,15 @@ public class WorkQueue {
corePoolSize + 4 // concurrency level
);
queueName = prefix;
buildMetrics(queueName, metrics);
try {
buildMetrics(queueName, metrics);
} catch (IllegalArgumentException e) {
if (e.getMessage().contains("already")) {
log.warn("Not creating metrics for queue '{}': already exists", queueName);
} else {
throw e;
}
}
}
private void buildMetrics(String queueName, MetricMaker metric) {
@ -299,7 +309,7 @@ public class WorkQueue {
CaseFormat.UPPER_CAMEL.to(
CaseFormat.LOWER_UNDERSCORE,
queueName.replaceFirst("SSH", "Ssh").replaceAll("-", ""));
return String.format("queue/%s/%s", name, metricName);
return sanitizeMetricName(String.format("queue/%s/%s", name, metricName));
}
@Override

42
gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java Normal file
View File

@ -0,0 +1,42 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed 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 com.google.gerrit.metrics.dropwizard;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName;
import org.junit.Test;
public class DropWizardMetricMakerTest {
@Test
public void shouldSanitizeUnwantedChars() throws Exception {
assertThat(sanitizeMetricName("very+confusing$long#metric@net/name^1"))
.isEqualTo("very_confusing_long_metric_net/name_1");
assertThat(sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric");
}
@Test
public void shouldReduceConsecutiveSlashesToOne() throws Exception {
assertThat(sanitizeMetricName("/metric//submetric1///submetric2/submetric3"))
.isEqualTo("_metric/submetric1/submetric2/submetric3");
}
@Test
public void shouldNotFinishWithSlash() throws Exception {
assertThat(sanitizeMetricName("metric/")).isEqualTo("metric");
assertThat(sanitizeMetricName("metric//")).isEqualTo("metric");
assertThat(sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric");
}
}