feat(plugins): Download remote plugins #358
Conversation
link108
force-pushed
the
download-remote-plugins
branch
2 times, most recently
from
9c3cdc4
to
8b21165
Compare
link108
force-pushed
the
download-remote-plugins
branch
from
8b21165
to
6f17ecd
Compare
link108
changed the title
| public boolean enabled; | ||
|
|
||
| static final String regex = "^[a-zA-Z0-9]+\\/[\\w-]+$"; | ||
| static final Pattern pattern = Pattern.compile(regex); |
There was a problem hiding this comment.
Nit: Get rid of the regex property, since it's only used in one place.
Also, constants should be YELLING_SNAKE_CASE
There was a problem hiding this comment.
removed these variables, now getting the matcher is just one line in the function 👍
There was a problem hiding this comment.
Actually left pattern as a constant and updated its name, I think it helps readability
There was a problem hiding this comment.
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.
| @NoArgsConstructor | ||
| public class PluginConfigurationList { | ||
|
|
||
| @JsonProperty("pluginConfigurationList") |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
Also... is this class used anywhere? I don't see it being used.
There was a problem hiding this comment.
heh, I had that same thought. I had refactored it out, but didn't remove the file from the PR, deleting now 👍
| jarLocation | expected | ||
| "/opt/spinnaker/plugin/foo-bar-1.2.3.jar" | Paths.get("/opt/spinnaker/plugin/foo-bar-1.2.3.jar").toUri().toURL() | ||
| "file://example/com/foo-bar-1.2.3.jar" | new URL("file://example/com/foo-bar-1.2.3.jar") | ||
| "http://example.com/foo-bar-1.2.3.jar" | new URL("http://example.com/foo-bar-1.2.3.jar") |
There was a problem hiding this comment.
Would like to see a negative case in these tests as well.
There was a problem hiding this comment.
Added in another test. I was thinking of just having one or two test functions here, but I think having descriptive test names is good to show intent
There was a problem hiding this comment.
Can you elaborate a bit on the use case for this? Adding plugins feels like an installation time concern, and the ability to via misconfiguration or malicious configuration fetch and execute remote code feels like it would need some pretty hefty justification to counter the potential for security vulnerabilities that it could introduce.
|
|
||
| public void validate() { | ||
| Matcher matcher = pattern.matcher(name); | ||
| if (!matcher.find()) { |
There was a problem hiding this comment.
could use matcher.matches here I believe
There was a problem hiding this comment.
I changed to use matches() instead of find(), they're similar, but by using matches(), I'm able to simplify the pattern 👍
link108
force-pushed
the
download-remote-plugins
branch
2 times, most recently
from
9e615e0
to
52f2140
Compare
link108
force-pushed
the
download-remote-plugins
branch
from
52f2140
to
b70ac25
Compare
|
@cfieber Of course, happy to do so 👍 Most of the Spinnaker administrators I've talked to run, or want to run, Spinnaker in Kubernetes, where each microservice is one or more pods running the same Docker image. A pod is a grouping of docker images that all run on the same host. Spinnaker micro services run with one docker image per pod, the application itself, eg Orca, Clouddriver, etc. In order to get a Docker image onto a pod, Kubernetes will download it from a remote source. While the remote source could be Docker hub or another public registry, in practice, Docker images are hosted in a secure artifact store that Kubernetes has access to (such as Artifactory.) Since we need some way of getting the plugin code onto the pod, we can mimic the approach of downloading assets from a location that matches the administrators’ security preference. Requiring Spinnaker administrators to create their own custom docker images that contain plugins raises the barrier of entry to use plugins at all. They would still have to host their custom Docker images somewhere for Kubernetes to download and run. Since downloading assets is common when running Kubernetes and most Spinnaker administrators I've spoken with already host a secure artifact store, it would be a natural location to host plugin artifacts as well. At most sites, Spinnaker administrators are trusted to ensure configuration is valid and not malicious. Additionally, ingress/egress is most often controlled at the infrastructure level with security groups, firewall rules, etc. Taking all of that into consideration and combining it with this feature being disabled by default, the administrator would have to purposefully introduce risk to their network. Looking at other projects in the industry, this seems like an accepted practice. For example, in Jenkins, administrators can download plugins from a remote source at runtime. Ideally Halyard would deliver plugins to Kubernetes directly, however there is a 1 MB limit on the Kubernetes side, so it won't work as plugins may be larger than 1 MB. — Do note, this is opt in only, so you have to manually enable the feature. The Halyard command to configure this defaults to disabled (spinnaker/halyard#1393) |
|
@link108 Thanks for the details - this feels okayish for now. |
This PR is related to spinnaker/spinnaker#4181.
Different environments used to run Spinnaker have varying constraints, in order to ensure plugins can be used in all environments, we added the ability to download plugins via Kork at runtime. By default, downloading of plugin jars will be disabled, but if the Spinnaker administrator trusts the download location, they can enable plugin jar downloading. Spinnaker users (application owners) will not have the ability to add plugins.
To ensure Spinnaker is running as expected, a Spinnaker service will fail to start if it fails to download the plugin JARs.