Skip to content
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.

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

feat(plugins): Download remote plugins #358

Merged
merged 7 commits into from
Aug 19, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
import com.google.common.annotations.Beta;
import com.netflix.spinnaker.kork.plugins.spring.configs.PluginConfiguration;
import com.netflix.spinnaker.kork.plugins.spring.configs.PluginProperties;
import com.netflix.spinnaker.kork.plugins.spring.configs.PluginPropertyDetails;
import java.io.*;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.*;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -67,10 +69,10 @@ public void loadPlugins(Class source) {
return;
}
try (InputStream inputStream = openFromFile()) {
PluginProperties props = parsePluginConfigs(inputStream);
List<PluginProperties.PluginConfiguration> pluginConfigurations = getEnabledJars(props);
PluginPropertyDetails props = parsePluginConfigs(inputStream);
List<PluginConfiguration> pluginConfigurations = getEnabledJars(props);
logPlugins(pluginConfigurations);
urls = getJarPathsFromPluginConfigurations(pluginConfigurations);
urls = getJarPathsFromPluginConfigurations(pluginConfigurations, props.downloadingEnabled);
} catch (IOException e) {
throw new MissingPluginConfigurationException(
String.format(
Expand All @@ -84,14 +86,15 @@ public void loadPlugins(Class source) {
* Convert inputStream to pluginProperties object
*
* @param inputStream The list of plugins
* @return PluginProperties
* @return PluginPropertyDetails
*/
private PluginProperties parsePluginConfigs(InputStream inputStream) {
private PluginPropertyDetails parsePluginConfigs(InputStream inputStream) {
ObjectMapper objectMapper = new ObjectMapper(new YAMLFactory());
try {
PluginProperties pluginConfigs = objectMapper.readValue(inputStream, PluginProperties.class);
pluginConfigs.validate();
return pluginConfigs;
PluginPropertyDetails pluginPropertyDetails =
objectMapper.readValue(inputStream, PluginProperties.class).getPluginsPropertyDetails();
pluginPropertyDetails.validate();
return pluginPropertyDetails;
} catch (IOException e) {
throw new MalformedPluginConfigurationException("Unable to parse plugin configurations", e);
}
Expand All @@ -114,12 +117,11 @@ private boolean checkFileExists() {
/**
* This method returns a list of Plugin Configurations that are enabled
*
* @param pluginProperties Plugin configs, deserialized
* @param pluginPropertyDetails Plugin configs, deserialized
* @return List<PluginConfiguration> List of pluginConfigurations where enabled is true
*/
private List<PluginProperties.PluginConfiguration> getEnabledJars(
PluginProperties pluginProperties) {
return pluginProperties.pluginConfigurationList.stream()
private List<PluginConfiguration> getEnabledJars(PluginPropertyDetails pluginPropertyDetails) {
return pluginPropertyDetails.getPluginConfigurations().stream()
.filter(p -> p.enabled == true)
.collect(Collectors.toList());
}
Expand All @@ -129,23 +131,29 @@ private List<PluginProperties.PluginConfiguration> getEnabledJars(
* @return List<URL> List of paths to jars, where enabled is true
*/
private URL[] getJarPathsFromPluginConfigurations(
List<PluginProperties.PluginConfiguration> pluginConfigurations) {
List<PluginConfiguration> pluginConfigurations, boolean downloadingEnabled) {
return pluginConfigurations.stream()
.map(PluginProperties.PluginConfiguration::getJars)
.map(PluginConfiguration::getJars)
.flatMap(Collection::stream)
.map(Paths::get)
.map(this::getUrlFromPath)
.map(s -> convertToUrl(s, downloadingEnabled))
.distinct()
.toArray(URL[]::new);
}

/**
* @param path
* @param jarLocation
* @return path as a URL
*/
private URL getUrlFromPath(Path path) {
private URL convertToUrl(String jarLocation, boolean downloadingEnabled) {
try {
return path.toUri().toURL();
if (jarLocation.startsWith("/")) {
return Paths.get(jarLocation).toUri().toURL();
} else if (jarLocation.startsWith("file://") || downloadingEnabled) {
return new URL(jarLocation);
} else {
throw new MalformedPluginConfigurationException(
"Attempting to download jar " + jarLocation + " but downloading is disabled");
}
} catch (MalformedURLException e) {
throw new MalformedPluginConfigurationException(e);
}
Expand All @@ -156,9 +164,9 @@ private URL getUrlFromPath(Path path) {
*
* @param pluginConfigurations
*/
private void logPlugins(List<PluginProperties.PluginConfiguration> pluginConfigurations) {
private void logPlugins(List<PluginConfiguration> pluginConfigurations) {
if (!pluginConfigurations.isEmpty()) {
for (PluginProperties.PluginConfiguration pluginConfiguration : pluginConfigurations) {
for (PluginConfiguration pluginConfiguration : pluginConfigurations) {
log.info("Loading {}", pluginConfiguration);
}
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2019 Armory, Inc.
*
* 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.netflix.spinnaker.kork.plugins.spring.configs;

import com.netflix.spinnaker.kork.plugins.spring.MalformedPluginConfigurationException;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@AllArgsConstructor
@NoArgsConstructor
public class PluginConfiguration {

public String name;
List<String> jars;
public boolean enabled;

static final String regex = "^[a-zA-Z0-9]+\\/[\\w-]+$";
static final Pattern pattern = Pattern.compile(regex);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Get rid of the regex property, since it's only used in one place.

Also, constants should be YELLING_SNAKE_CASE

Copy link
Member Author

Choose a reason for hiding this comment

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

removed these variables, now getting the matcher is just one line in the function 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually left pattern as a constant and updated its name, I think it helps readability

Copy link
Member

Choose a reason for hiding this comment

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

Not only readability, but compiling regex isn't free (and can produce exceptions) - which is why people assign its compiled into a static: If the compilation fails, it'll only fail once and at class creation, which is way better than at each invocation time.


public void validate() {
Matcher matcher = pattern.matcher(name);
if (!matcher.find()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could use matcher.matches here I believe

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed to use matches() instead of find(), they're similar, but by using matches(), I'm able to simplify the pattern 👍

throw new MalformedPluginConfigurationException(
String.format("Invalid plugin name: %s", name));
}
}

@Override
public String toString() {
return new StringBuilder()
.append("Plugin: " + name + ", ")
.append("Jars: " + String.join(", ", jars))
.toString();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright 2019 Armory, Inc.
*
* 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.netflix.spinnaker.kork.plugins.spring.configs;

import com.fasterxml.jackson.annotation.JsonProperty;
import java.util.List;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@AllArgsConstructor
@NoArgsConstructor
public class PluginConfigurationList {

@JsonProperty("pluginConfigurationList")
Copy link
Member

Choose a reason for hiding this comment

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

Personal preference, this: Remove List from the property name: It's just noise. Plural form for collections is preferable. No one in conversation says, "Check out my widget list," but instead they say, "Check out my widgets."

Copy link
Member

Choose a reason for hiding this comment

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

Also... is this class used anywhere? I don't see it being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

heh, I had that same thought. I had refactored it out, but didn't remove the file from the PR, deleting now 👍

List<PluginConfiguration> pluginConfigurationList;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright 2019 Armory, Inc.
*
* 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.netflix.spinnaker.kork.plugins.spring.configs;

import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@AllArgsConstructor
@NoArgsConstructor
public class PluginProperties {

@JsonProperty("plugins")
PluginPropertyDetails pluginsPropertyDetails;
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.netflix.spinnaker.kork.plugins.spring;

package com.netflix.spinnaker.kork.plugins.spring.configs;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.netflix.spinnaker.kork.plugins.spring.MalformedPluginConfigurationException;
import java.util.Collections;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import lombok.AllArgsConstructor;
import lombok.Data;
Expand All @@ -28,22 +28,25 @@
@Data
@AllArgsConstructor
@NoArgsConstructor
public class PluginProperties {
public class PluginPropertyDetails {

@JsonProperty("pluginConfigurations")
List<PluginConfiguration> pluginConfigurations;

@JsonProperty("plugins")
List<PluginConfiguration> pluginConfigurationList;
@JsonProperty("downloadingEnabled")
public Boolean downloadingEnabled;

public void validate() {
validateUniquePlugins();
for (PluginConfiguration pluginConfiguration : pluginConfigurationList) {
for (PluginConfiguration pluginConfiguration : getPluginConfigurations()) {
pluginConfiguration.validate();
}
}

public void validateUniquePlugins() {

List<String> pluginNames =
pluginConfigurationList.stream()
getPluginConfigurations().stream()
.map(PluginConfiguration::getName)
.collect(Collectors.toList());

Expand All @@ -57,33 +60,4 @@ public void validateUniquePlugins() {
String.format("The following plugins have been defined multiple times: %s", duplicates));
}
}

@Data
@AllArgsConstructor
@NoArgsConstructor
public static class PluginConfiguration {

public String name;
List<String> jars;
public boolean enabled;

static final String regex = "^[a-zA-Z0-9]+\\/[\\w-]+$";
static final Pattern pattern = Pattern.compile(regex);

public void validate() {
Matcher matcher = pattern.matcher(name);
if (!matcher.find()) {
throw new MalformedPluginConfigurationException(
String.format("Invalid plugin name: %s", name));
}
}

@Override
public String toString() {
return new StringBuilder()
.append("Plugin: " + name + ", ")
.append("Jars: " + String.join(", ", jars))
.toString();
}
}
}
Loading