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: container outputs and dynamic environments #591

Merged
merged 21 commits into from
Sep 11, 2024
Merged

Conversation

ecrupper
Copy link
Contributor

This PR introduces dynamic environments using a sidecar outputs container.

Proposal ref: go-vela/community#988

Outputs Container Image

When not set, the build will proceed just as it does today, making this feature completely opt-in and easy to turn off.

The input for VELA_EXECUTOR_OUTPUTS_IMAGE will be the image used for the sidecar container that is spun up next to the Vela build. At some point, this could expand to be a programmatic/plugin implementation rather than what it is in this PR — a sleeping container plugged into the volume that is polled after the conclusion of each step.

In the docker-compose.yml, I decided to just go with alpine:latest.

Substitution and Injection relocation

		// perform any substitution on dynamic variables
		err = _step.Substitute()
		if err != nil {
			return err
		}

		c.Logger.Debug("injecting non-substituted secrets")
		// inject no-substitution secrets for container
		err = injectSecrets(_step, c.NoSubSecrets)
		if err != nil {
			return err
		}

This block has moved from CreateStep to ExecBuild for the Docker runtime and will now occur after polling the outputs container. This will allow for dynamic environments such as this:

  - name: write to outputs
    image: alpine:latest
    commands:
      - echo "GREETING=hello" >> /vela/outputs/.env
      - echo "SECRET=whisper" >> /vela/outputs/masked.env
      - echo "IMAGE=ubuntu" >> /vela/outputs/.env
  
  - name: read from outputs
    image: ${IMAGE}       # resolves to `ubuntu`
    pull: on_start
    commands:
      - echo $GREETING  # prints hello
      - echo $SECRET      # prints ***

Updates to privileged image checking

To protect against dynamic invocations of privileged images using this new feature, parsing images to determine whether they should run will now be at runtime. The logic has moved from AssembleBuild to ExecStep/ExecService.

Unfortunately this means that builds will run up until that step is set to execute before denying it, which is a slightly worse UX but I think the tradeoff is worth it.

@ecrupper ecrupper requested a review from a team as a code owner July 25, 2024 18:43
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 57.87037% with 91 lines in your changes missing coverage. Please review.

Project coverage is 57.50%. Comparing base (66403e5) to head (4c1eca2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
executor/linux/build.go 33.33% 14 Missing and 8 partials ⚠️
runtime/docker/container.go 51.51% 9 Missing and 7 partials ⚠️
executor/linux/outputs.go 80.64% 6 Missing and 6 partials ⚠️
executor/linux/stage.go 37.50% 6 Missing and 4 partials ⚠️
cmd/vela-worker/run.go 0.00% 8 Missing ⚠️
executor/linux/step.go 66.66% 3 Missing and 3 partials ⚠️
runtime/kubernetes/container.go 28.57% 4 Missing and 1 partial ⚠️
internal/image/image.go 73.33% 2 Missing and 2 partials ⚠️
runtime/docker/image.go 0.00% 3 Missing and 1 partial ⚠️
cmd/vela-worker/exec.go 0.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
+ Coverage   57.44%   57.50%   +0.06%     
==========================================
  Files         120      121       +1     
  Lines        5055     5201     +146     
==========================================
+ Hits         2904     2991      +87     
- Misses       1948     1984      +36     
- Partials      203      226      +23     
Files with missing lines Coverage Δ
executor/linux/linux.go 100.00% <100.00%> (ø)
executor/linux/opts.go 100.00% <100.00%> (ø)
executor/linux/secret.go 73.50% <100.00%> (ø)
executor/setup.go 100.00% <100.00%> (ø)
internal/step/environment.go 78.57% <100.00%> (+1.64%) ⬆️
...ernetes/generated/clientset/versioned/clientset.go 29.72% <ø> (ø)
...ed/clientset/versioned/fake/clientset_generated.go 40.90% <ø> (ø)
...tes/generated/clientset/versioned/fake/register.go 100.00% <ø> (ø)
...s/generated/clientset/versioned/scheme/register.go 100.00% <ø> (ø)
...ed/vela/v1alpha1/fake/fake_pipelinepodstemplate.go 12.50% <ø> (ø)
... and 14 more

... and 1 file with indirect coverage changes

Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

was doing some functional testing this morning and was not able to get it working with stages. i see the container being spun up, but it doesn't look like /vela/outputs/.env is being injected. i can cat it, of course.

that's not an intentional limitation, right? didn't see anything on that in the proposal or here anyway.

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

just a question/suggestion

executor/linux/outputs.go Outdated Show resolved Hide resolved
wass3rw3rk
wass3rw3rk previously approved these changes Sep 6, 2024
executor/linux/outputs_test.go Outdated Show resolved Hide resolved
@wass3rw3rk
Copy link
Member

wass3rw3rk commented Sep 6, 2024

friendly reminder to also add an accompanying docs PR :D

executor/linux/outputs.go Outdated Show resolved Hide resolved
@@ -185,6 +185,17 @@ func WithVersion(version string) Opt {
}
}

func WithOutputCtn(ctn *pipeline.Container) Opt {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func WithOutputCtn(ctn *pipeline.Container) Opt {
// WithOutputCtn sets the outputs container in the executor client for Linux.
func WithOutputCtn(ctn *pipeline.Container) Opt {

return nil
}

// update engine logger with secret metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// update engine logger with secret metadata
// update engine logger

@plyr4
Copy link
Contributor

plyr4 commented Sep 11, 2024

love it.
any thoughts on letting platform admins adjust the amount of resources allocated to the outputs container? (not blocking)

@ecrupper
Copy link
Contributor Author

any thoughts on letting platform admins adjust the amount of resources allocated to the outputs container? (not blocking)

I will open up a follow up issue for this. And for the comments I will open up a follow up PR shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants