diff options
33 files changed, 397 insertions, 253 deletions
diff --git a/bundle-plugin-test/src/test/java/com/yahoo/BundleIT.java b/bundle-plugin-test/src/test/java/com/yahoo/BundleIT.java index 5b4fdda06f7..932b78a5c8d 100644 --- a/bundle-plugin-test/src/test/java/com/yahoo/BundleIT.java +++ b/bundle-plugin-test/src/test/java/com/yahoo/BundleIT.java @@ -18,6 +18,7 @@ import java.util.jar.JarFile; import java.util.jar.Manifest; import java.util.zip.ZipEntry; +import static com.yahoo.osgi.maven.ProjectBundleClassPaths.CLASSPATH_MAPPINGS_FILENAME; import static org.hamcrest.CoreMatchers.allOf; import static org.hamcrest.CoreMatchers.anyOf; import static org.hamcrest.CoreMatchers.containsString; @@ -143,9 +144,9 @@ public class BundleIT { @SuppressWarnings("unchecked") @Test public void bundle_class_path_mappings_are_generated() throws URISyntaxException, IOException { - URL mappingsUrl = getClass().getResource("/" + com.yahoo.osgi.maven.ProjectBundleClassPaths.CLASSPATH_MAPPINGS_FILENAME); + URL mappingsUrl = getClass().getResource("/" + CLASSPATH_MAPPINGS_FILENAME); assertNotNull( - "Could not find " + ProjectBundleClassPaths.CLASSPATH_MAPPINGS_FILENAME + " in the test output directory", + "Could not find " + CLASSPATH_MAPPINGS_FILENAME + " in the test output directory", mappingsUrl); ProjectBundleClassPaths bundleClassPaths = ProjectBundleClassPaths.load(Paths.get(mappingsUrl.toURI())); diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java index 798fea2644e..0626c786822 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java @@ -11,10 +11,14 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.jar.Manifest; import java.util.stream.Collectors; /** + * "Public package" in this context means a package that is either exported or declared as "Global-Package" + * in a bundle's manifest. + * * @author Tony Vaagenes * @author ollivir */ @@ -23,10 +27,18 @@ public class AnalyzeBundle { public final List<Export> exports; public final List<String> globals; - public PublicPackages(List<Export> exports, List<String> globals) { + PublicPackages(List<Export> exports, List<String> globals) { this.exports = exports; this.globals = globals; } + + public Set<String> exportedPackageNames() { + return exports.stream() + .map(Export::getPackageNames) + .flatMap(Collection::stream) + .collect(Collectors.toSet()); + } + } public static PublicPackages publicPackagesAggregated(Collection<File> jarFiles) { diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java index 13bbc63192c..9f5fd2236e7 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java @@ -1,6 +1,7 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.container.plugin.classanalysis; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Sets; import com.yahoo.container.plugin.util.Maps; @@ -10,6 +11,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; /** * @author Tony Vaagenes @@ -19,7 +21,8 @@ public class PackageTally { private final Map<String, Optional<ExportPackageAnnotation>> definedPackagesMap; private final Set<String> referencedPackagesUnfiltered; - public PackageTally(Map<String, Optional<ExportPackageAnnotation>> definedPackagesMap, Set<String> referencedPackagesUnfiltered) { + @VisibleForTesting + PackageTally(Map<String, Optional<ExportPackageAnnotation>> definedPackagesMap, Set<String> referencedPackagesUnfiltered) { this.definedPackagesMap = definedPackagesMap; this.referencedPackagesUnfiltered = referencedPackagesUnfiltered; } @@ -41,6 +44,19 @@ public class PackageTally { } /** + * Returns the set of packages that is referenced from this tally, but not included in the given set of available packages. + * + * @param definedAndExportedPackages Set of available packages (usually all packages defined in the generated bundle's project + all exported packages of dependencies) + * @return The set of missing packages, that may cause problem when the bundle is deployed in an OSGi container runtime. + */ + public Set<String> referencedPackagesMissingFrom(Set<String> definedAndExportedPackages) { + return Sets.difference(referencedPackages(), definedAndExportedPackages).stream() + .filter(pkg -> !pkg.startsWith("java.")) + .filter(pkg -> !pkg.equals(com.yahoo.api.annotations.PublicApi.class.getPackageName())) + .collect(Collectors.toSet()); + } + + /** * Represents the classes for two package tallies that are deployed as a single unit. * <p> * ExportPackageAnnotations from this has precedence over the other. diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/Artifacts.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/Artifacts.java index fff88d413d0..a0d0e143724 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/Artifacts.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/Artifacts.java @@ -12,8 +12,8 @@ import java.util.List; * @author Tony Vaagenes * @author ollivir */ -public class Artifacts { - public static class ArtifactSet { +class Artifacts { + static class ArtifactSet { private final List<Artifact> jarArtifactsToInclude; private final List<Artifact> jarArtifactsProvided; private final List<Artifact> nonJarArtifacts; @@ -24,20 +24,20 @@ public class Artifacts { this.nonJarArtifacts = nonJarArtifacts; } - public List<Artifact> getJarArtifactsToInclude() { + List<Artifact> getJarArtifactsToInclude() { return jarArtifactsToInclude; } - public List<Artifact> getJarArtifactsProvided() { + List<Artifact> getJarArtifactsProvided() { return jarArtifactsProvided; } - public List<Artifact> getNonJarArtifacts() { + List<Artifact> getNonJarArtifacts() { return nonJarArtifacts; } } - public static ArtifactSet getArtifacts(MavenProject project) { + static ArtifactSet getArtifacts(MavenProject project) { List<Artifact> jarArtifactsToInclude = new ArrayList<>(); List<Artifact> jarArtifactsProvided = new ArrayList<>(); @@ -63,7 +63,7 @@ public class Artifacts { return new ArtifactSet(jarArtifactsToInclude, jarArtifactsProvided, nonJarArtifactsToInclude); } - public static Collection<Artifact> getArtifactsToInclude(MavenProject project) { + static Collection<Artifact> getArtifactsToInclude(MavenProject project) { return getArtifacts(project).getJarArtifactsToInclude(); } } diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java index 9c9957ae718..04f2428d284 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java @@ -10,14 +10,12 @@ import com.yahoo.container.plugin.classanalysis.PackageTally; import com.yahoo.container.plugin.osgi.ExportPackageParser; import com.yahoo.container.plugin.osgi.ExportPackages; import com.yahoo.container.plugin.osgi.ExportPackages.Export; -import com.yahoo.container.plugin.osgi.ImportPackages; import com.yahoo.container.plugin.osgi.ImportPackages.Import; import com.yahoo.container.plugin.util.Strings; import org.apache.commons.lang3.tuple.Pair; import org.apache.maven.artifact.Artifact; import org.apache.maven.plugin.AbstractMojo; import org.apache.maven.plugin.MojoExecutionException; -import org.apache.maven.plugin.MojoFailureException; import org.apache.maven.plugins.annotations.Mojo; import org.apache.maven.plugins.annotations.Parameter; import org.apache.maven.plugins.annotations.ResolutionScope; @@ -41,6 +39,9 @@ import java.util.jar.Manifest; import java.util.stream.Collectors; import java.util.stream.Stream; +import static com.yahoo.container.plugin.bundle.AnalyzeBundle.publicPackagesAggregated; +import static com.yahoo.container.plugin.osgi.ExportPackages.exportsByPackageName; +import static com.yahoo.container.plugin.osgi.ImportPackages.calculateImports; import static com.yahoo.container.plugin.util.Files.allDescendantFiles; import static com.yahoo.container.plugin.util.IO.withFileOutputStream; import static com.yahoo.container.plugin.util.JarFiles.withInputStream; @@ -90,32 +91,43 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { @Parameter(alias = "X-Jersey-Binding") private String jerseyBinding = null; - public void execute() throws MojoExecutionException, MojoFailureException { + public void execute() throws MojoExecutionException { try { Artifacts.ArtifactSet artifactSet = Artifacts.getArtifacts(project); warnOnUnsupportedArtifacts(artifactSet.getNonJarArtifacts()); - AnalyzeBundle.PublicPackages publicPackagesFromProvidedJars = AnalyzeBundle.publicPackagesAggregated( + // Packages from Export-Package and Global-Package headers in provided scoped jars + AnalyzeBundle.PublicPackages publicPackagesFromProvidedJars = publicPackagesAggregated( artifactSet.getJarArtifactsProvided().stream().map(Artifact::getFile).collect(Collectors.toList())); - PackageTally includedJarPackageTally = definedPackages(artifactSet.getJarArtifactsToInclude()); - PackageTally projectPackageTally = analyzeProjectClasses(); - PackageTally pluginPackageTally = projectPackageTally.combine(includedJarPackageTally); + // Packages from Export-Package headers in provided scoped jars + Set<String> exportedPackagesFromProvidedDeps = publicPackagesFromProvidedJars.exportedPackageNames(); - Set<String> definedPackages = new HashSet<>(projectPackageTally.definedPackages()); - definedPackages.addAll(includedJarPackageTally.definedPackages()); + // Packaged defined in this project's code + PackageTally projectPackages = getProjectClassesTally(); - warnIfPackagesDefinedOverlapsGlobalPackages(definedPackages, publicPackagesFromProvidedJars.globals); + // Packages defined in compile scoped jars + PackageTally compileJarsPackages = definedPackages(artifactSet.getJarArtifactsToInclude()); - if (getLog().isDebugEnabled()) { - getLog().debug("Referenced packages = " + pluginPackageTally.referencedPackages()); - getLog().debug("Defined packages = " + pluginPackageTally.definedPackages()); - getLog().debug("Exported packages of dependencies = " + publicPackagesFromProvidedJars.exports.stream() - .map(e -> "(" + e.getPackageNames().toString() + ", " + e.version().orElse("")).collect(Collectors.joining(", "))); + // The union of packages in the project and compile scoped jars + PackageTally includedPackages = projectPackages.combine(compileJarsPackages); + + warnIfPackagesDefinedOverlapsGlobalPackages(includedPackages.definedPackages(), publicPackagesFromProvidedJars.globals); + logDebugPackageSets(publicPackagesFromProvidedJars, includedPackages); + + if (hasJdiscCoreProvided(artifactSet.getJarArtifactsProvided())) { + // jdisc_core being provided guarantees that log output does not contain its exported packages + logMissingPackages(exportedPackagesFromProvidedDeps, projectPackages, compileJarsPackages, includedPackages); + } else { + getLog().warn("This project does not have jdisc_core as provided dependency, so the " + + "generated 'Import-Package' OSGi header may be missing important packages."); } + logOverlappingPackages(projectPackages, exportedPackagesFromProvidedDeps); + logUnnecessaryPackages(compileJarsPackages, exportedPackagesFromProvidedDeps); - Map<String, Import> calculatedImports = ImportPackages.calculateImports(pluginPackageTally.referencedPackages(), - pluginPackageTally.definedPackages(), ExportPackages.exportsByPackageName(publicPackagesFromProvidedJars.exports)); + Map<String, Import> calculatedImports = calculateImports(includedPackages.referencedPackages(), + includedPackages.definedPackages(), + exportsByPackageName(publicPackagesFromProvidedJars.exports)); Map<String, Optional<String>> manualImports = emptyToNone(importPackage).map(GenerateOsgiManifestMojo::getManualImports) .orElseGet(HashMap::new); @@ -123,17 +135,74 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { calculatedImports.remove(packageName); } createManifestFile(new File(project.getBuild().getOutputDirectory()), manifestContent(project, - artifactSet.getJarArtifactsToInclude(), manualImports, calculatedImports.values(), pluginPackageTally)); + artifactSet.getJarArtifactsToInclude(), manualImports, calculatedImports.values(), includedPackages)); } catch (Exception e) { - throw new MojoExecutionException("Failed generating osgi manifest.", e); + throw new MojoExecutionException("Failed generating osgi manifest", e); + } + } + + private void logDebugPackageSets(AnalyzeBundle.PublicPackages publicPackagesFromProvidedJars, PackageTally includedPackages) { + if (getLog().isDebugEnabled()) { + getLog().debug("Referenced packages = " + includedPackages.referencedPackages()); + getLog().debug("Defined packages = " + includedPackages.definedPackages()); + getLog().debug("Exported packages of dependencies = " + publicPackagesFromProvidedJars.exports.stream() + .map(e -> "(" + e.getPackageNames().toString() + ", " + e.version().orElse("")).collect(Collectors.joining(", "))); + } + } + + private boolean hasJdiscCoreProvided(List<Artifact> providedArtifacts) { + return providedArtifacts.stream().anyMatch(artifact -> artifact.getArtifactId().equals("jdisc_core")); + } + + private void logMissingPackages(Set<String> exportedPackagesFromProvidedJars, + PackageTally projectPackages, + PackageTally compileJarPackages, + PackageTally includedPackages) { + + Set<String> definedAndExportedPackages = Sets.union(includedPackages.definedPackages(), exportedPackagesFromProvidedJars); + + Set<String> missingProjectPackages = projectPackages.referencedPackagesMissingFrom(definedAndExportedPackages); + if (! missingProjectPackages.isEmpty()) { + getLog().warn("Packages unavailable runtime are referenced from project classes " + + "(annotations can usually be ignored): " + missingProjectPackages); + } + + Set<String> missingCompilePackages = compileJarPackages.referencedPackagesMissingFrom(definedAndExportedPackages); + if (! missingCompilePackages.isEmpty()) { + getLog().info("Packages unavailable runtime are referenced from compile scoped jars " + + "(annotations can usually be ignored): " + missingCompilePackages); + } + } + + private void logOverlappingPackages(PackageTally projectPackages, + Set<String> exportedPackagesFromProvidedDeps) { + Set<String> overlappingProjectPackages = Sets.intersection(projectPackages.definedPackages(), exportedPackagesFromProvidedDeps); + if (! overlappingProjectPackages.isEmpty()) { + getLog().warn("Project classes use the following packages that are already defined in provided scoped dependencies: " + + overlappingProjectPackages); + } + } + + /* + * This mostly detects packages re-exported via composite bundles like jdisc_core and container-disc. + * An artifact can only be represented once, either in compile or provided scope. So if the project + * adds an artifact in compile scope that we deploy as a pre-installed bundle, we won't see the same + * artifact as provided via container-dev and hence can't detect the duplicate packages. + */ + private void logUnnecessaryPackages(PackageTally compileJarsPackages, + Set<String> exportedPackagesFromProvidedDeps) { + Set<String> unnecessaryPackages = Sets.intersection(compileJarsPackages.definedPackages(), exportedPackagesFromProvidedDeps); + if (! unnecessaryPackages.isEmpty()) { + getLog().info("Compile scoped jars contain the following packages that are most likely " + + "available from jdisc runtime: " + unnecessaryPackages); } } private static void warnIfPackagesDefinedOverlapsGlobalPackages(Set<String> internalPackages, List<String> globalPackages) throws MojoExecutionException { Set<String> overlap = Sets.intersection(internalPackages, new HashSet<>(globalPackages)); - if (overlap.isEmpty() == false) { + if (! overlap.isEmpty()) { throw new MojoExecutionException( "The following packages are both global and included in the bundle:\n " + String.join("\n ", overlap)); } @@ -173,7 +242,7 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { Pair.of("WebInfUrl", webInfUrl), // Pair.of("Import-Package", importPackage), // Pair.of("Export-Package", exportPackage))) { - if (element.getValue() != null && element.getValue().isEmpty() == false) { + if (element.getValue() != null && ! element.getValue().isEmpty()) { ret.put(element.getKey(), element.getValue()); } } @@ -232,7 +301,7 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { } private void warnOnUnsupportedArtifacts(Collection<Artifact> nonJarArtifacts) { - List<Artifact> unsupportedArtifacts = nonJarArtifacts.stream().filter(a -> "pom".equals(a.getType()) == false) + List<Artifact> unsupportedArtifacts = nonJarArtifacts.stream().filter(a -> ! a.getType().equals("pom")) .collect(Collectors.toList()); unsupportedArtifacts.forEach(artifact -> getLog() @@ -240,7 +309,7 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { artifact.getId(), artifact.getType()))); } - private PackageTally analyzeProjectClasses() { + private PackageTally getProjectClassesTally() { File outputDirectory = new File(project.getBuild().getOutputDirectory()); List<ClassFileMetaData> analyzedClasses = allDescendantFiles(outputDirectory).filter(file -> file.getName().endsWith(".class")) @@ -258,7 +327,7 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { List<ClassFileMetaData> analyzedClasses = new ArrayList<>(); for (Enumeration<JarEntry> entries = jarFile.entries(); entries.hasMoreElements();) { JarEntry entry = entries.nextElement(); - if (entry.isDirectory() == false && entry.getName().endsWith(".class")) { + if (! entry.isDirectory() && entry.getName().endsWith(".class")) { analyzedClasses.add(analyzeClass(jarFile, entry)); } } @@ -305,10 +374,7 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { } private static Optional<String> emptyToNone(String str) { - return Optional.ofNullable(str).map(String::trim).filter(s -> s.isEmpty() == false); + return Optional.ofNullable(str).map(String::trim).filter(s -> ! s.isEmpty()); } - private static boolean isClassToAnalyze(String name) { - return name.endsWith(".class") && name.endsWith("module-info.class") == false; - } } diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/osgi/ImportPackages.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/osgi/ImportPackages.java index b58248ec4a6..a3031cb7ad7 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/osgi/ImportPackages.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/osgi/ImportPackages.java @@ -74,8 +74,9 @@ public class ImportPackages { } } - public static Map<String, Import> calculateImports(Set<String> referencedPackages, Set<String> implementedPackages, - Map<String, ExportPackages.Export> exportedPackages) { + public static Map<String, Import> calculateImports(Set<String> referencedPackages, + Set<String> implementedPackages, + Map<String, ExportPackages.Export> exportedPackages) { Map<String, Import> ret = new HashMap<>(); for (String undefinedPackage : Sets.difference(referencedPackages, implementedPackages)) { ExportPackages.Export export = exportedPackages.get(undefinedPackage); diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/JdkPackages.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/JdkPackages.java new file mode 100644 index 00000000000..0786272bc70 --- /dev/null +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/JdkPackages.java @@ -0,0 +1,39 @@ +package com.yahoo.container.plugin.util; + +import java.net.URL; + +/** + * @author gjoranv + */ +public class JdkPackages { + + /** + * Returns a boolean indicating (via best effort) if the given package is part of the JDK. + */ + public static boolean isJdkPackage(String pkg) { + return hasJdkExclusivePrefix(pkg) + || isResourceInPlatformClassLoader(pkg); // TODO: must be a class, not a package, due to module encapsulation + } + + private static boolean isResourceInPlatformClassLoader(String klass) { + String klassAsPath = klass.replaceAll("\\.", "/") + ".class"; + URL resource = getPlatformClassLoader().getResource(klassAsPath); + return !(resource == null); + } + + private static ClassLoader getPlatformClassLoader() { + ClassLoader platform = JdkPackages.class.getClassLoader().getParent(); + + // Will fail upon changes in classloader hierarchy between JDK versions + assert (platform.getName().equals("platform")); + + return platform; + } + + private static boolean hasJdkExclusivePrefix(String pkg) { + return pkg.startsWith("java.") + || pkg.startsWith("sun."); + } + +} + diff --git a/bundle-plugin/src/test/java/com/yahoo/container/plugin/bundle/AnalyzeBundleTest.java b/bundle-plugin/src/test/java/com/yahoo/container/plugin/bundle/AnalyzeBundleTest.java index 8cccf0598ab..a09604b842b 100644 --- a/bundle-plugin/src/test/java/com/yahoo/container/plugin/bundle/AnalyzeBundleTest.java +++ b/bundle-plugin/src/test/java/com/yahoo/container/plugin/bundle/AnalyzeBundleTest.java @@ -11,8 +11,10 @@ import org.junit.rules.ExpectedException; import java.io.File; import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import static com.yahoo.container.plugin.classanalysis.TestUtilities.throwableMessage; import static org.hamcrest.CoreMatchers.containsString; @@ -28,6 +30,7 @@ import static org.junit.Assert.assertThat; */ public class AnalyzeBundleTest { private final List<ExportPackages.Export> exports; + private final Set<String> exportedPackageNames; private final Map<String, ExportPackages.Export> exportsByPackageName; File jarDir = new File("src/test/resources/jar"); @@ -37,6 +40,7 @@ public class AnalyzeBundleTest { File simple = new File(jarDir, "simple1.jar"); PublicPackages pp = AnalyzeBundle.publicPackagesAggregated(Arrays.asList(notOsgi, simple)); this.exports = pp.exports; + this.exportedPackageNames = pp.exportedPackageNames(); this.exportsByPackageName = ExportPackages.exportsByPackageName(exports); } @@ -55,6 +59,11 @@ public class AnalyzeBundleTest { assertThat(exportsByPackageName.keySet(), hasItem("com.yahoo.sample.exported.package")); } + @Test + public void exported_class_names_can_be_retrieved() { + assertThat(exportedPackageNames, is(new HashSet<>(exports.get(0).getPackageNames()))); + } + @Rule public ExpectedException exception = ExpectedException.none(); diff --git a/bundle-plugin/src/test/java/com/yahoo/container/plugin/classanalysis/PackageTallyTest.java b/bundle-plugin/src/test/java/com/yahoo/container/plugin/classanalysis/PackageTallyTest.java new file mode 100644 index 00000000000..e80562c2c13 --- /dev/null +++ b/bundle-plugin/src/test/java/com/yahoo/container/plugin/classanalysis/PackageTallyTest.java @@ -0,0 +1,24 @@ +// Copyright 2019 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.container.plugin.classanalysis; + +import org.junit.Test; + +import java.util.Set; + +import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +/** + * @author gjoranv + */ +public class PackageTallyTest { + + @Test + public void referenced_packages_missing_from_are_detected() { + PackageTally tally = new PackageTally(emptyMap(), Set.of("p1", "java.util", "com.yahoo.api.annotations", "missing")); + Set<String> missingPackages = tally.referencedPackagesMissingFrom(Set.of("p1")); + assertThat(missingPackages, is(Set.of("missing"))); + } + +} diff --git a/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ImportPackageTest.java b/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ImportPackageTest.java index 23960323a31..b76151c790e 100644 --- a/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ImportPackageTest.java +++ b/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ImportPackageTest.java @@ -109,8 +109,9 @@ public class ImportPackageTest { is("com.yahoo.exported;version=\"[1.2.3,2)\"")); } - private static Set<Import> calculateImports(Set<String> referencedPackages, Set<String> implementedPackages, - Map<String, Export> exportedPackages) { + private static Set<Import> calculateImports(Set<String> referencedPackages, + Set<String> implementedPackages, + Map<String, Export> exportedPackages) { return new HashSet<>(ImportPackages.calculateImports(referencedPackages, implementedPackages, exportedPackages).values()); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java index db7b4a88721..a3099fe3ed4 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java @@ -306,7 +306,7 @@ public class ContentSearchCluster extends AbstractConfigProducer implements Prot @Override public void getConfig(ProtonConfig.Builder builder) { double visibilityDelay = hasIndexedCluster() ? getIndexed().getVisibilityDelay() : 0.0; - builder.feeding.concurrency(0.25); // As if specified 0.5 in services.xml + builder.feeding.concurrency(0.30); // As if specified 0.6 in services.xml boolean hasAnyNonIndexedCluster = false; for (NewDocumentType type : TopologicalDocumentTypeSorter.sort(documentDefinitions.values())) { ProtonConfig.Documentdb.Builder ddbB = new ProtonConfig.Documentdb.Builder(); diff --git a/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java index c43787b37e0..2415aadc8a3 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java @@ -118,13 +118,13 @@ public class DocumentDatabaseTestCase { @Test public void requireThatConcurrencyIsReflectedCorrectlyForDefault() { - verifyConcurrency("index", "", 0.25, 0.25); - verifyConcurrency("streaming", "", 0.5, 0.0); - verifyConcurrency("store-only", "", 0.5, 0.0); + verifyConcurrency("index", "", 0.30, 0.30); + verifyConcurrency("streaming", "", 0.6, 0.0); + verifyConcurrency("store-only", "", 0.6, 0.0); } @Test public void requireThatMixedModeConcurrencyIsReflectedCorrectlyForDefault() { - verifyConcurrency(Arrays.asList(DocType.create("a", "index"), DocType.create("b", "streaming")), "", 0.5, Arrays.asList(0.25, 0.0)); + verifyConcurrency(Arrays.asList(DocType.create("a", "index"), DocType.create("b", "streaming")), "", 0.6, Arrays.asList(0.30, 0.0)); } @Test public void requireThatMixedModeConcurrencyIsReflected() { diff --git a/config-provisioning/abi-spec.json b/config-provisioning/abi-spec.json index 64114389751..33c02811318 100644 --- a/config-provisioning/abi-spec.json +++ b/config-provisioning/abi-spec.json @@ -737,25 +737,6 @@ ], "fields": [] }, - "com.yahoo.config.provision.RotationName": { - "superClass": "java.lang.Object", - "interfaces": [ - "java.lang.Comparable" - ], - "attributes": [ - "public" - ], - "methods": [ - "public java.lang.String value()", - "public boolean equals(java.lang.Object)", - "public int hashCode()", - "public int compareTo(com.yahoo.config.provision.RotationName)", - "public java.lang.String toString()", - "public static com.yahoo.config.provision.RotationName from(java.lang.String)", - "public bridge synthetic int compareTo(java.lang.Object)" - ], - "fields": [] - }, "com.yahoo.config.provision.SystemName": { "superClass": "java.lang.Enum", "interfaces": [], diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/RotationName.java b/config-provisioning/src/main/java/com/yahoo/config/provision/RotationName.java deleted file mode 100644 index fb6d9dc09e6..00000000000 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/RotationName.java +++ /dev/null @@ -1,58 +0,0 @@ -// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.config.provision; - -import java.util.Objects; - -/** - * Represents a rotation name for a container cluster. Typically created from the rotation element in services.xml. - * - * @author mpolden - */ -// TODO(mpolden): Remove this once all usages have been replaced -public class RotationName implements Comparable<RotationName> { - - private final String name; - - private RotationName(String name) { - this.name = requireNonBlank(name, "name must be non-empty"); - } - - public String value() { - return name; - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - RotationName that = (RotationName) o; - return name.equals(that.name); - } - - @Override - public int hashCode() { - return Objects.hash(name); - } - - @Override - public int compareTo(RotationName o) { - return name.compareTo(o.name); - } - - @Override - public String toString() { - return "rotation '" + name + "'"; - } - - public static RotationName from(String name) { - return new RotationName(name); - } - - private static String requireNonBlank(String s, String message) { - if (s == null || s.isBlank()) { - throw new IllegalArgumentException(message); - } - return s; - } - -} diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionStateWatcher.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionStateWatcher.java index 53a472c2b67..d5a87b3c45e 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionStateWatcher.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionStateWatcher.java @@ -63,9 +63,9 @@ public class LocalSessionStateWatcher { public void nodeChanged() { zkWatcherExecutor.execute(() -> { try { - ChildData data = fileCache.getCurrentData(); - if (data != null) { - sessionChanged(Session.Status.parse(Utf8.toString(fileCache.getCurrentData().getData()))); + ChildData node = fileCache.getCurrentData(); + if (node != null) { + sessionChanged(Session.Status.parse(Utf8.toString(node.getData()))); } } catch (Exception e) { log.log(LogLevel.WARNING, session.logPre() + "Error handling session changed for session " + getSessionId(), e); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionStateWatcher.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionStateWatcher.java index 653a2616cbe..a3fad3d7322 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionStateWatcher.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionStateWatcher.java @@ -1,8 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.session; -import com.yahoo.concurrent.StripedExecutor; -import com.yahoo.config.provision.TenantName; import com.yahoo.log.LogLevel; import com.yahoo.text.Utf8; import com.yahoo.vespa.config.server.ReloadHandler; @@ -72,12 +70,12 @@ public class RemoteSessionStateWatcher { } } - public void nodeChanged() { + private void nodeChanged() { zkWatcherExecutor.execute(() -> { try { - ChildData data = fileCache.getCurrentData(); - if (data != null) { - sessionChanged(Session.Status.parse(Utf8.toString(fileCache.getCurrentData().getData()))); + ChildData node = fileCache.getCurrentData(); + if (node != null) { + sessionChanged(Session.Status.parse(Utf8.toString(node.getData()))); } } catch (Exception e) { log.log(LogLevel.WARNING, session.logPre() + "Error handling session changed for session " + getSessionId(), e); diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java index 1b0c0b9d982..0ad15e9cabe 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java @@ -1,15 +1,12 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.configserver; -import com.google.common.collect.ImmutableSortedSet; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.HostName; -import com.yahoo.config.provision.RotationName; import java.util.Objects; import java.util.Optional; -import java.util.Set; /** * Represents an exclusive load balancer, assigned to an application's cluster. @@ -23,19 +20,6 @@ public class LoadBalancer { private final ClusterSpec.Id cluster; private final HostName hostname; private final Optional<String> dnsZone; - private final Set<RotationName> rotations; - - // TODO(mpolden): Kept for API compatibility with internal code. This constructor can be removed when all usages are - // TODO(mpolden): removed - public LoadBalancer(String id, ApplicationId application, ClusterSpec.Id cluster, HostName hostname, - Optional<String> dnsZone, Set<RotationName> rotations) { - this.id = Objects.requireNonNull(id, "id must be non-null"); - this.application = Objects.requireNonNull(application, "application must be non-null"); - this.cluster = Objects.requireNonNull(cluster, "cluster must be non-null"); - this.hostname = Objects.requireNonNull(hostname, "hostname must be non-null"); - this.dnsZone = Objects.requireNonNull(dnsZone, "dnsZone must be non-null"); - this.rotations = ImmutableSortedSet.copyOf(Objects.requireNonNull(rotations, "rotations must be non-null")); - } public LoadBalancer(String id, ApplicationId application, ClusterSpec.Id cluster, HostName hostname, Optional<String> dnsZone) { @@ -44,7 +28,6 @@ public class LoadBalancer { this.cluster = Objects.requireNonNull(cluster, "cluster must be non-null"); this.hostname = Objects.requireNonNull(hostname, "hostname must be non-null"); this.dnsZone = Objects.requireNonNull(dnsZone, "dnsZone must be non-null"); - this.rotations = Set.of(); } public String id() { @@ -67,8 +50,4 @@ public class LoadBalancer { return dnsZone; } - public Set<RotationName> rotations() { - return rotations; - } - } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java index db9291cd651..475671181e3 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.api.integration.zone; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; @@ -10,7 +11,6 @@ import com.yahoo.config.provision.zone.UpgradePolicy; import com.yahoo.config.provision.zone.ZoneFilter; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.athenz.api.AthenzIdentity; -import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; @@ -56,6 +56,9 @@ public interface ZoneRegistry { /** Return the configserver's Athenz service identity */ AthenzIdentity getConfigServerAthenzIdentity(ZoneId zoneId); + /** Return the system Athenz domain */ + AthenzDomain accessControlDomain(); + /** Returns the Vespa upgrade policy to use for zones in this registry */ UpgradePolicy upgradePolicy(); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index 45620c262cb..d2942d77b7a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -12,7 +12,6 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.AthenzService; import com.yahoo.config.provision.ClusterSpec; -import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.io.IOUtils; import com.yahoo.log.LogLevel; @@ -587,7 +586,7 @@ public class InternalStepRunner implements StepRunner { ApplicationVersion version = controller.jobController().run(id).get().versions().targetApplication(); DeploymentSpec spec = controller.applications().require(id.application()).deploymentSpec(); - byte[] servicesXml = servicesXml(controller.system(), testerFlavorFor(id, spec)); + byte[] servicesXml = servicesXml(controller.zoneRegistry().accessControlDomain(), testerFlavorFor(id, spec)); byte[] testPackage = controller.applications().applicationStore().get(id.tester(), version); ZoneId zone = id.type().zone(controller.system()); @@ -619,9 +618,7 @@ public class InternalStepRunner implements StepRunner { } /** Returns the generated services.xml content for the tester application. */ - static byte[] servicesXml(SystemName systemName, Optional<String> testerFlavor) { - String domain = systemName == SystemName.main ? "vespa.vespa" : "vespa.vespa.cd"; - + static byte[] servicesXml(AthenzDomain domain, Optional<String> testerFlavor) { String flavor = testerFlavor.orElse("d-1-4-50"); int memoryGb = Integer.parseInt(flavor.split("-")[2]); // Memory available in tester container. int jdiscMemoryPercentage = (int) Math.ceil(200.0 / memoryGb); // 2Gb memory for tester application (excessive?). @@ -646,7 +643,7 @@ public class InternalStepRunner implements StepRunner { " <http>\n" + " <server id='default' port='4080'/>\n" + " <filtering>\n" + - " <access-control domain='" + domain + "'>\n" + // Set up dummy access control to pass validation :/ + " <access-control domain='" + domain.value() + "'>\n" + // Set up dummy access control to pass validation :/ " <exclude>\n" + " <binding>http://*/tester/v1/*</binding>\n" + " </exclude>\n" + @@ -659,7 +656,7 @@ public class InternalStepRunner implements StepRunner { " </config>\n" + " <component id=\"com.yahoo.jdisc.http.filter.security.athenz.StaticRequestResourceMapper\" bundle=\"jdisc-security-filters\">\n" + " <config name=\"jdisc.http.filter.security.athenz.static-request-resource-mapper\">\n" + - " <resourceName>" + domain + ":tester-application</resourceName>\n" + + " <resourceName>" + domain.value() + ":tester-application</resourceName>\n" + " <action>deploy</action>\n" + " </config>\n" + " </component>\n" + diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java index 095651df033..f8157680cfd 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java @@ -3,15 +3,12 @@ package com.yahoo.vespa.hosted.controller.deployment; import com.yahoo.component.Version; import com.yahoo.config.application.api.DeploymentSpec; -import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.HostName; -import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.slime.ArrayTraverser; import com.yahoo.slime.Inspector; -import com.yahoo.slime.JsonFormat; -import com.yahoo.slime.ObjectTraverser; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.ConfigChangeActions; import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.RefeedAction; @@ -28,7 +25,6 @@ import com.yahoo.vespa.hosted.controller.application.RoutingPolicy; import org.junit.Before; import org.junit.Test; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.UncheckedIOException; import java.net.URI; @@ -38,7 +34,6 @@ import java.nio.file.Paths; import java.time.Duration; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.Executors; @@ -404,7 +399,7 @@ public class InternalStepRunnerTest { @Test public void generates_correct_services_xml_test() { - assertFile("test_runner_services.xml-cd", new String(InternalStepRunner.servicesXml(SystemName.cd, Optional.of("d-2-12-75")))); + assertFile("test_runner_services.xml-cd", new String(InternalStepRunner.servicesXml(AthenzDomain.from("vespa.vespa.cd"), Optional.of("d-2-12-75")))); } private void assertFile(String resourceName, String actualContent) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java index f802a6af549..cff0f8da463 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java @@ -6,6 +6,7 @@ import com.google.inject.Inject; import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.component.AbstractComponent; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; @@ -27,7 +28,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.stream.Collectors; /** * @author mpolden @@ -108,6 +108,11 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry } @Override + public AthenzDomain accessControlDomain() { + return AthenzDomain.from("vespadomain"); + } + + @Override public UpgradePolicy upgradePolicy() { return upgradePolicy; } diff --git a/http-utils/src/main/java/ai/vespa/util/http/VespaHttpClientBuilder.java b/http-utils/src/main/java/ai/vespa/util/http/VespaHttpClientBuilder.java index 5e7a9441fc8..5ac6786ae30 100644 --- a/http-utils/src/main/java/ai/vespa/util/http/VespaHttpClientBuilder.java +++ b/http-utils/src/main/java/ai/vespa/util/http/VespaHttpClientBuilder.java @@ -4,24 +4,28 @@ package ai.vespa.util.http; import com.yahoo.security.tls.MixedMode; import com.yahoo.security.tls.TlsContext; import com.yahoo.security.tls.TransportSecurityUtils; +import org.apache.http.HttpException; +import org.apache.http.HttpHost; import org.apache.http.HttpRequest; -import org.apache.http.HttpRequestInterceptor; -import org.apache.http.client.methods.HttpRequestBase; -import org.apache.http.client.utils.URIBuilder; +import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.protocol.HttpClientContext; import org.apache.http.config.Registry; import org.apache.http.config.RegistryBuilder; import org.apache.http.conn.HttpClientConnectionManager; +import org.apache.http.conn.UnsupportedSchemeException; +import org.apache.http.conn.routing.HttpRoute; +import org.apache.http.conn.routing.HttpRoutePlanner; import org.apache.http.conn.socket.ConnectionSocketFactory; import org.apache.http.conn.socket.PlainConnectionSocketFactory; import org.apache.http.conn.ssl.NoopHostnameVerifier; import org.apache.http.conn.ssl.SSLConnectionSocketFactory; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.impl.conn.BasicHttpClientConnectionManager; +import org.apache.http.impl.conn.DefaultSchemePortResolver; import org.apache.http.protocol.HttpContext; import javax.net.ssl.SSLParameters; -import java.net.URI; -import java.net.URISyntaxException; +import java.net.InetAddress; import java.util.logging.Level; import java.util.logging.Logger; @@ -69,7 +73,7 @@ public class VespaHttpClientBuilder { private static HttpClientBuilder createBuilder(ConnectionManagerFactory connectionManagerFactory) { var builder = HttpClientBuilder.create(); addSslSocketFactory(builder, connectionManagerFactory); - addTlsAwareRequestInterceptor(builder); + addHttpsRewritingRoutePlanner(builder); return builder; } @@ -86,11 +90,10 @@ public class VespaHttpClientBuilder { }); } - private static void addTlsAwareRequestInterceptor(HttpClientBuilder builder) { + private static void addHttpsRewritingRoutePlanner(HttpClientBuilder builder) { if (TransportSecurityUtils.isTransportSecurityEnabled() && TransportSecurityUtils.getInsecureMixedMode() != MixedMode.PLAINTEXT_CLIENT_MIXED_SERVER) { - log.log(Level.FINE, "Adding request interceptor to client"); - builder.addInterceptorFirst(new HttpToHttpsRewritingRequestInterceptor()); + builder.setRoutePlanner(new HttpToHttpsRoutePlanner()); } } @@ -106,29 +109,32 @@ public class VespaHttpClientBuilder { .build(); } - static class HttpToHttpsRewritingRequestInterceptor implements HttpRequestInterceptor { + + /** + * Reroutes requests using 'http' to 'https'. + * Implementation inspired by {@link org.apache.http.impl.conn.DefaultRoutePlanner}, but without proxy support. + */ + static class HttpToHttpsRoutePlanner implements HttpRoutePlanner { + @Override - public void process(HttpRequest request, HttpContext context) { - if (request instanceof HttpRequestBase) { - HttpRequestBase httpUriRequest = (HttpRequestBase) request; - httpUriRequest.setURI(rewriteUri(httpUriRequest.getURI())); - } else { - log.log(Level.FINE, () -> "Not a HttpRequestBase - skipping URI rewriting: " + request.getClass().getName()); - } + public HttpRoute determineRoute(HttpHost host, HttpRequest request, HttpContext context) throws HttpException { + HttpClientContext clientContext = HttpClientContext.adapt(context); + RequestConfig config = clientContext.getRequestConfig(); + InetAddress local = config.getLocalAddress(); + + HttpHost target = resolveTarget(host); + boolean secure = target.getSchemeName().equalsIgnoreCase("https"); + return new HttpRoute(target, local, secure); } - private static URI rewriteUri(URI originalUri) { - if (!originalUri.getScheme().equals("http")) { - return originalUri; - } - int port = originalUri.getPort(); - int rewrittenPort = port != -1 ? port : 80; + private HttpHost resolveTarget(HttpHost host) throws HttpException { try { - URI rewrittenUri = new URIBuilder(originalUri).setScheme("https").setPort(rewrittenPort).build(); - log.log(Level.FINE, () -> String.format("Uri rewritten from '%s' to '%s'", originalUri, rewrittenUri)); - return rewrittenUri; - } catch (URISyntaxException e) { - throw new RuntimeException(e); + String originalScheme = host.getSchemeName(); + String scheme = originalScheme.equalsIgnoreCase("http") ? "https" : originalScheme; + int port = DefaultSchemePortResolver.INSTANCE.resolve(host); + return new HttpHost(host.getHostName(), port, scheme); + } catch (UnsupportedSchemeException e) { + throw new HttpException(e.getMessage(), e); } } } diff --git a/http-utils/src/test/java/ai/vespa/util/http/VespaHttpClientBuilderTest.java b/http-utils/src/test/java/ai/vespa/util/http/VespaHttpClientBuilderTest.java index 7ffd0e459b0..85ee0913c58 100644 --- a/http-utils/src/test/java/ai/vespa/util/http/VespaHttpClientBuilderTest.java +++ b/http-utils/src/test/java/ai/vespa/util/http/VespaHttpClientBuilderTest.java @@ -1,14 +1,15 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package ai.vespa.util.http; -import ai.vespa.util.http.VespaHttpClientBuilder.HttpToHttpsRewritingRequestInterceptor; -import org.apache.http.client.methods.HttpGet; -import org.apache.http.protocol.BasicHttpContext; +import org.apache.http.HttpException; +import org.apache.http.HttpHost; +import org.apache.http.HttpRequest; +import org.apache.http.client.protocol.HttpClientContext; +import org.apache.http.conn.routing.HttpRoute; import org.junit.Test; -import java.net.URI; - -import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; /** * @author bjorncs @@ -16,24 +17,25 @@ import static org.assertj.core.api.AssertionsForClassTypes.assertThat; public class VespaHttpClientBuilderTest { @Test - public void request_interceptor_modifies_scheme_of_requests() { - verifyProcessedUriMatchesExpectedOutput("http://dummyhostname:8080/a/path/to/resource?query=value", - "https://dummyhostname:8080/a/path/to/resource?query=value"); + public void route_planner_modifies_scheme_of_requests() throws HttpException { + verifyProcessedUriMatchesExpectedOutput("http://dummyhostname:8080", "https://dummyhostname:8080"); + } + + @Test + public void route_planer_handles_implicit_http_port() throws HttpException { + verifyProcessedUriMatchesExpectedOutput("http://dummyhostname", "https://dummyhostname:80"); } @Test - public void request_interceptor_add_handles_implicit_http_port() { - verifyProcessedUriMatchesExpectedOutput("http://dummyhostname/a/path/to/resource?query=value", - "https://dummyhostname:80/a/path/to/resource?query=value"); + public void route_planer_handles_https_port() throws HttpException { + verifyProcessedUriMatchesExpectedOutput("http://dummyhostname:443", "https://dummyhostname:443"); } - private static void verifyProcessedUriMatchesExpectedOutput(String inputUri, String expectedOutputUri) { - var interceptor = new HttpToHttpsRewritingRequestInterceptor(); - HttpGet request = new HttpGet(inputUri); - interceptor.process(request, new BasicHttpContext()); - URI modifiedUri = request.getURI(); - URI expectedUri = URI.create(expectedOutputUri); - assertThat(modifiedUri).isEqualTo(expectedUri); + private static void verifyProcessedUriMatchesExpectedOutput(String inputHostString, String expectedHostString) throws HttpException { + var routePlanner = new VespaHttpClientBuilder.HttpToHttpsRoutePlanner(); + HttpRoute newRoute = routePlanner.determineRoute(HttpHost.create(inputHostString), mock(HttpRequest.class), new HttpClientContext()); + HttpHost target = newRoute.getTargetHost(); + assertEquals(expectedHostString, target.toURI()); } }
\ No newline at end of file diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/LoadBalancersResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/LoadBalancersResponse.java index bfbf7775031..9f8f4a804d1 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/LoadBalancersResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/LoadBalancersResponse.java @@ -77,10 +77,6 @@ public class LoadBalancersResponse extends HttpResponse { realObject.setString("ipAddress", real.ipAddress()); realObject.setLong("port", real.port()); }); - - // TODO(mpolden): The following fields preserves API compatibility. These can be removed once clients stop expecting them - lbObject.setArray("rotations"); - lbObject.setBool("inactive", lb.state() == LoadBalancer.State.inactive); }); new JsonFormat(true).encode(stream, slime); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers-single.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers-single.json index 67d2c3bfa4b..19e65c2fc25 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers-single.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers-single.json @@ -28,9 +28,7 @@ "ipAddress": "127.0.14.1", "port": 4080 } - ], - "rotations": [], - "inactive": false + ] } ] } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers.json index c9a45a9c3da..0b05a41af0a 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/load-balancers.json @@ -28,9 +28,7 @@ "ipAddress": "127.0.10.1", "port": 4080 } - ], - "rotations": [], - "inactive": false + ] }, { "id": "tenant4:application4:instance4:id4", @@ -60,9 +58,7 @@ "ipAddress": "127.0.14.1", "port": 4080 } - ], - "rotations": [], - "inactive": false + ] } ] } diff --git a/vespa-http-client/pom.xml b/vespa-http-client/pom.xml index 18dd34ee7be..96aa6defbe6 100644 --- a/vespa-http-client/pom.xml +++ b/vespa-http-client/pom.xml @@ -65,6 +65,22 @@ <artifactId>airline</artifactId> <version>0.6</version> </dependency> + <dependency> + <!-- Needed for Vespa TLS configuration. Standard jar artifact --> + <groupId>com.yahoo.vespa</groupId> + <artifactId>http-utils</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + <dependency> + <!-- Needed for Vespa TLS configuration. --> + <!-- This artifact is packaged as an OSGi bundle - make sure to manually include or exclude transitive dependencies as necessary --> + <!-- Note: includes BouncyCastle to compile scope transitively. --> + <groupId>com.yahoo.vespa</groupId> + <artifactId>security-utils</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> <!-- Test dependencies --> <dependency> @@ -140,6 +156,17 @@ </goals> <configuration> <outputFile>target/${project.artifactId}-jar-with-dependencies.jar</outputFile> + <filters> + <filter> + <!-- Don't include signature files in uber jar (most likely from bouncycastle). --> + <artifact>*:*</artifact> + <excludes> + <exclude>META-INF/*.SF</exclude> + <exclude>META-INF/*.DSA</exclude> + <exclude>META-INF/*.RSA</exclude> + </excludes> + </filter> + </filters> <transformers> <transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer"> <mainClass>com.yahoo.vespa.http.client.runner.Runner</mainClass> diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/ConnectionParams.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/ConnectionParams.java index f503190864b..adf61b124ab 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/ConnectionParams.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/ConnectionParams.java @@ -43,6 +43,16 @@ public final class ConnectionParams { private int traceLevel = 0; private int traceEveryXOperation = 0; private boolean printTraceToStdErr = true; + private boolean useTlsConfigFromEnvironment = false; + + /** + * Use TLS configuration through the standard Vespa environment variables. + * Setting this to 'true' will override any other TLS/HTTPS related configuration. + */ + public Builder setUseTlsConfigFromEnvironment(boolean useTlsConfigFromEnvironment) { + this.useTlsConfigFromEnvironment = useTlsConfigFromEnvironment; + return this; + } /** * Sets the SSLContext for the connection to the gateway when SSL is enabled for Endpoint. @@ -233,7 +243,8 @@ public final class ConnectionParams { dryRun, traceLevel, traceEveryXOperation, - printTraceToStdErr); + printTraceToStdErr, + useTlsConfigFromEnvironment); } public int getNumPersistentConnectionsPerEndpoint() { @@ -273,6 +284,10 @@ public final class ConnectionParams { public HostnameVerifier getHostnameVerifier() { return hostnameVerifier; } + + public boolean useTlsConfigFromEnvironment() { + return useTlsConfigFromEnvironment; + } } private final SSLContext sslContext; private final HostnameVerifier hostnameVerifier; @@ -288,6 +303,7 @@ public final class ConnectionParams { private final int traceLevel; private final int traceEveryXOperation; private final boolean printTraceToStdErr; + private final boolean useTlsConfigFromEnvironment; private ConnectionParams( SSLContext sslContext, @@ -303,9 +319,11 @@ public final class ConnectionParams { boolean dryRun, int traceLevel, int traceEveryXOperation, - boolean printTraceToStdErr) { + boolean printTraceToStdErr, + boolean useTlsConfigFromEnvironment) { this.sslContext = sslContext; this.hostnameVerifier = hostnameVerifier; + this.useTlsConfigFromEnvironment = useTlsConfigFromEnvironment; this.headers.putAll(headers); this.headerProviders.putAll(headerProviders); this.numPersistentConnectionsPerEndpoint = numPersistentConnectionsPerEndpoint; @@ -378,6 +396,10 @@ public final class ConnectionParams { return printTraceToStdErr; } + public boolean useTlsConfigFromEnvironment() { + return useTlsConfigFromEnvironment; + } + /** * A header provider that provides a header value. {@link #getHeaderValue()} is called each time a new HTTP request * is constructed by {@link com.yahoo.vespa.http.client.FeedClient}. diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java index 5289a7a562a..0b92427bcbe 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java @@ -17,14 +17,8 @@ import org.apache.http.StatusLine; import org.apache.http.client.HttpClient; import org.apache.http.client.config.RequestConfig; import org.apache.http.client.methods.HttpPost; -import org.apache.http.config.Registry; -import org.apache.http.config.RegistryBuilder; -import org.apache.http.conn.socket.ConnectionSocketFactory; -import org.apache.http.conn.socket.PlainConnectionSocketFactory; -import org.apache.http.conn.ssl.SSLConnectionSocketFactory; import org.apache.http.entity.InputStreamEntity; import org.apache.http.impl.client.HttpClientBuilder; -import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.apache.http.message.BasicHeader; import java.io.ByteArrayInputStream; @@ -400,25 +394,24 @@ class ApacheGatewayConnection implements GatewayConnection { } public HttpClient createClient() { - HttpClientBuilder clientBuilder = HttpClientBuilder.create(); - if (useSsl && connectionParams.getSslContext() != null) { - Registry<ConnectionSocketFactory> socketFactoryRegistry = RegistryBuilder.<ConnectionSocketFactory>create() - .register("https", new SSLConnectionSocketFactory( - connectionParams.getSslContext(), connectionParams.getHostnameVerifier())) - .register("http", PlainConnectionSocketFactory.INSTANCE) - .build(); - PoolingHttpClientConnectionManager connMgr = new PoolingHttpClientConnectionManager(socketFactoryRegistry); - clientBuilder.setConnectionManager(connMgr); - + HttpClientBuilder clientBuilder; + if (connectionParams.useTlsConfigFromEnvironment()) { + clientBuilder = VespaTlsAwareClientBuilder.createHttpClientBuilder(); + } else { + clientBuilder = HttpClientBuilder.create(); + if (useSsl && connectionParams.getSslContext() != null) { + clientBuilder.setSslcontext(connectionParams.getSslContext()); + clientBuilder.setSSLHostnameVerifier(connectionParams.getHostnameVerifier()); + } } - clientBuilder.setUserAgent(String.format("vespa-http-client (%s)", Vtag.currentVersion)); - clientBuilder.setDefaultHeaders(Collections.singletonList(new BasicHeader(Headers.CLIENT_VERSION, Vtag.currentVersion))); clientBuilder.setMaxConnPerRoute(1); clientBuilder.setMaxConnTotal(1); + clientBuilder.setConnectionTimeToLive(15, TimeUnit.SECONDS); + clientBuilder.setUserAgent(String.format("vespa-http-client (%s)", Vtag.currentVersion)); + clientBuilder.setDefaultHeaders(Collections.singletonList(new BasicHeader(Headers.CLIENT_VERSION, Vtag.currentVersion))); clientBuilder.disableContentCompression(); // Try to disable the disabling to see if system tests become stable again. // clientBuilder.disableAutomaticRetries(); - clientBuilder.setConnectionTimeToLive(15, TimeUnit.SECONDS); { RequestConfig.Builder requestConfigBuilder = RequestConfig.custom(); requestConfigBuilder.setSocketTimeout(0); diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/VespaTlsAwareClientBuilder.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/VespaTlsAwareClientBuilder.java new file mode 100644 index 00000000000..be67e11963e --- /dev/null +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/VespaTlsAwareClientBuilder.java @@ -0,0 +1,26 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.http.client.core.communication; + +import org.apache.http.impl.client.HttpClientBuilder; + +/** + * A static factory for VespaHttpClientBuilder. + * The main purpose of this class is to avoid references to classes not compiled with JDK8. + * + * @author bjorncs + */ +// TODO Remove use of reflection once vespa-http-client only targets JDK11 +// The VespaTlsAwareClientBuilder class refers to classes in security-utils / http-utils that targets JDK11+. +class VespaTlsAwareClientBuilder { + + private VespaTlsAwareClientBuilder() {} + + static HttpClientBuilder createHttpClientBuilder() { + try { + Class<?> builderClass = Class.forName("ai.vespa.util.http.VespaHttpClientBuilder"); + return (HttpClientBuilder) builderClass.getMethod("create").invoke(null); + } catch (ReflectiveOperationException e) { + throw new RuntimeException(e); + } + } +} diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/Vtag.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/Vtag.java index f21a86dfc1b..f44a30208e7 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/Vtag.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/Vtag.java @@ -3,5 +3,5 @@ package com.yahoo.vespa.http.client.core.communication; class Vtag { - static final String currentVersion = "7.0.0"; + static final String currentVersion = "7.1.0"; } diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/runner/CommandLineArguments.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/runner/CommandLineArguments.java index 4e2c8f1509e..ea0b3f29509 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/runner/CommandLineArguments.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/runner/CommandLineArguments.java @@ -209,6 +209,10 @@ public class CommandLineArguments { description = "Add http header to every request. Header must have the format '<Name>: <Value>'. Use this parameter multiple times for multiple headers") private List<String> headers = new ArrayList<>(); + @Option(name = {"--vespaTls"}, + description = "BETA! Use Vespa TLS configuration from environment if available. Other HTTPS/TLS configuration will be ignored if this is set.") + private boolean useTlsConfigFromEnvironment = false; + private final List<Header> parsedHeaders = new ArrayList<>(); int getWhenVerboseEnabledPrintMessageForEveryXDocuments() { @@ -252,6 +256,7 @@ public class CommandLineArguments { .setTraceEveryXOperation(traceEveryXOperation) .setPrintTraceToStdErr(traceArg > 0) .setNumPersistentConnectionsPerEndpoint(numPersistentConnectionsPerEndpoint) + .setUseTlsConfigFromEnvironment(useTlsConfigFromEnvironment) .build() ) // Enable dynamic throttling. diff --git a/vespaclient-container-plugin/pom.xml b/vespaclient-container-plugin/pom.xml index 959fb687692..53e708601d7 100644 --- a/vespaclient-container-plugin/pom.xml +++ b/vespaclient-container-plugin/pom.xml @@ -55,6 +55,10 @@ <groupId>org.apache.httpcomponents</groupId> <artifactId>httpclient</artifactId> </exclusion> + <exclusion> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpcore</artifactId> + </exclusion> </exclusions> </dependency> <dependency> |