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

Allow specifying environment variables when installing an engine plugin as a Swarm service #38441

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@sirlatrom
Copy link

sirlatrom commented Dec 27, 2018

- What I did
I enabled specifying environment variables when installing a plugin through a Swarm service.

- How I did it

  1. I added an Env []string field to the PluginSpec type by modifying its protobuf file, allowing environment variables (and values) to be specified when a plugin is to be installed as a Swarm service.
  2. I added logic to transfer the environment variables to the resulting plugin task.

- How to verify it
a) Specify Env: []string{"MY_ENV_VAR=my-env-value"} in the TaskTemplate.PluginSpec object of the swarm.ServiceSpec when calling the service/create endpoint.
Example:

...
Env: []string{
    "policy-template={{ .ServiceName }},{{ .TaskImage }},{{ ServiceLabel \"com.docker.ucp.access.label\" }}",
...
},

b) The test TestServicePlugin in integration/service/plugin_test.go now covers the changed code.

- Description for the changelog

Allow specifying environment variables when installing an engine plugin as a Swarm service.

- A picture of a cute animal (not mandatory but encouraged)
68f39a73-bcad-4998-a7c8-ada5e1e8b8ff_1 ecf2088920bbbf3577bfce7402ad521f

@sirlatrom sirlatrom requested a review from cpuguy83 as a code owner Dec 27, 2018

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch 3 times, most recently from b3df338 to 37c3828 Dec 27, 2018

@sirlatrom sirlatrom changed the title WIP: Swarm plugin environment variables passing WIP: Allow specifying environment variables when installing an engine plugin as a Swarm service Dec 27, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 27, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@0dc5312). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38441   +/-   ##
=========================================
  Coverage          ?   36.59%           
=========================================
  Files             ?      608           
  Lines             ?    45235           
  Branches          ?        0           
=========================================
  Hits              ?    16556           
  Misses            ?    26395           
  Partials          ?     2284
Show resolved Hide resolved plugin/defs.go Outdated

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch from ae17882 to cf4c4df Jan 9, 2019

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 9, 2019

@thaJeztah I've incorporated your suggested change. I could not find any existing tests covering the WithSwarmService adjacent to my WithEnv function.

@sirlatrom sirlatrom changed the title WIP: Allow specifying environment variables when installing an engine plugin as a Swarm service Allow specifying environment variables when installing an engine plugin as a Swarm service Jan 9, 2019

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jan 9, 2019

Can you add a test for this (or modify the swarm plugin create test?)

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 9, 2019

@cpuguy Sure thing, thanks for the pointer, I'll see if I can add it to the right place in that test.

@sirlatrom sirlatrom requested a review from vdemeester as a code owner Jan 9, 2019

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 10, 2019

@cpuguy83 Currently there's no way for the Docker engine to tell what tasks exist for a plugin Swarm service, since they are in a different namespace. Please advise on how to continue writing a test in integration/service/plugin_test.go when the goal should be to verify the task/container for the plugin has the correct environment variables set.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jan 10, 2019

@sirlatrom You should be able to query the tasks API with the service ID as the filter.

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 11, 2019

@cpuguy83 That works for regular Docker tasks, but the /tasks endpoint only returns tasks that have a ContainerSpec. So I can inspect the plugin service, since that endpoint does not hide plugin services, but I cannot get its tasks, because the tasks endpoint hides those.

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 14, 2019

More specifically, it is this bit of code that restricts the runtime of the returned tasks:

if !filter.Contains("runtime") {
// default to only showing container tasks
filter.Add("runtime", "container")
filter.Add("runtime", "")
}

I'll extend the test helper for task listing to allow additional filters, including the "runtime" filter.

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch 3 times, most recently from e16a0de to 11fb7d1 Jan 14, 2019

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 14, 2019

Flaky test is flaky. DockerHubPullSuite.TestPullFromCentralRegistry passed in experimental, powerpc and z, but failed on janky. DockerSuite.TestRunStoppedLoggingDriverNoLeak failed on windowsRS1.

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch from 11fb7d1 to 7f6a6d3 Jan 15, 2019

@sirlatrom

This comment has been minimized.

Copy link

sirlatrom commented Jan 15, 2019

I force pushed to get checks to pass, but windowsRS1 still fails. @cpuguy83 Is that test unrelated? I feel like it is.

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch from 7f6a6d3 to ab32aec Jan 15, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 15, 2019

(reserved for my derek commands)

@derek derek bot added the rebuild/z label Jan 15, 2019

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jan 15, 2019

@sirlatrom That's a default filter. Should be able to add a filter for the plugin task, I think?

Show resolved Hide resolved plugin/defs.go
Show resolved Hide resolved plugin/defs.go
Show resolved Hide resolved plugin/defs.go

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch from ab32aec to 6c8266f Jan 15, 2019

Support environment vars in Swarm plugins services
Allow specifying environment variables when installing an engine plugin
as a Swarm service. Invalid environment variable entries (without an
equals (`=`) char) will be ignored.

Signed-off-by: Sune Keller <absukl@almbrand.dk>

@sirlatrom sirlatrom force-pushed the sirlatrom:swarm_plugin_env branch from 6c8266f to 8908c31 Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment