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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: introduce dockerfile to containerize VU #77

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

Todai88
Copy link
Contributor

@Todai88 Todai88 commented Nov 26, 2021

Introduces a local dockerfile (multi-stage) that can be used to build and run a containerized version of VU (still simple).

Due to the container being built by docker, you'll have to share your private ssh_key in the image, or you won't have access to private/internal Snyk repositories.

To try this out:

  1. docker build . -t vu --build-arg github_user=Todai88 --build-arg ssh_key="$(cat ~/.ssh/id_rsa)"
  2. docker run -p 8080:8080 -e host="0.0.0.0" -p 8080:8080is used to bind port8080on the host's network adapter to8080on the running container.e host="0.0.0.0is used to share the container's address with the network context that Docker runs on. If not provided, you will receiveempty response from server`. 馃憥

@Todai88 Todai88 self-assigned this Nov 26, 2021
@CLAassistant
Copy link

CLAassistant commented Nov 26, 2021

CLA assistant check
All committers have signed the CLA.

@Todai88 Todai88 force-pushed the feat/introduce-docker-file branch 2 times, most recently from d82c9f0 to ee450f0 Compare November 29, 2021 12:23
@Todai88 Todai88 marked this pull request as ready for review November 29, 2021 12:50
@Todai88 Todai88 requested a review from a team as a code owner November 29, 2021 12:50
Copy link
Contributor

@cmars cmars left a comment

Choose a reason for hiding this comment

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

Given that this is an open source project, one should be able to build it without private dependencies.

vervet-underground/local.Dockerfile Outdated Show resolved Hide resolved
@Todai88
Copy link
Contributor Author

Todai88 commented Nov 30, 2021

Changes that's been made:

  • no longer using our private (for now) config library. Instead using a simple JSON decoder.
  • Dockerfile updated to no longer use any key material
  • server.go using the new, tiny lib/config.go file to decode the config.default.json code.

Copy link
Contributor

@cmars cmars left a comment

Choose a reason for hiding this comment

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

LGTM with a few thoughts

@@ -0,0 +1,20 @@
package lib
Copy link
Contributor

Choose a reason for hiding this comment

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

Package names in Go should be more descriptive. I'd recommend putting this in a package config instead that deals with all matters of VU config.

https://go.dev/blog/package-names for context.

vervet-underground/lib/config.go Outdated Show resolved Hide resolved
vervet-underground/lib/config.go Outdated Show resolved Hide resolved
vervet-underground/server.go Outdated Show resolved Hide resolved
Requires a correct SSH config set up that allows access to our private Github repositories:
>  docker build . -t vu

Run:
> docker run -p 8080:8080 vu
@Todai88 Todai88 requested a review from cmars November 30, 2021 14:19
"vervet-underground/lib"
)

func Load(configPath string) (*lib.ServerConfig, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc please, maybe something like

// Load reads the config file from configPath and returns it as a new ServerConfig instance.

ServerConfig should also come over to config. Ideally we should drop the lib package in favor of meaningful package names. (If there is other stuff in lib that isn't config-related, let's relocate those bits in followups.)

@genslein
Copy link
Contributor

@cmars @Todai88 maybe a question for a future piece: I would expect env vars to be used instead of a file/mount load for secrets in a container for an app. Is this not the recommended usage going forward in k8s? The registry mechanism of handling secrets seems a bit clunky

@cmars
Copy link
Contributor

cmars commented Nov 30, 2021

@cmars @Todai88 maybe a question for a future piece: I would expect env vars to be used instead of a file/mount load for secrets in a container for an app. Is this not the recommended usage going forward in k8s? The registry mechanism of handling secrets seems a bit clunky

A comma-separated string in an env var for the list of services to scrape would probably suffice for the initial version of this. Later versions will do service discovery from k8s direct. So, we could get rid of the config file... I don't think we need to configure the bind host in a container either.

Secrets I'd agree are best provided as env vars, unless they're large (like PEM-encoded private keys) in which case it makes more sense to mount them. Blending secrets with actual config in a monolithic JSON is an antipattern I think we can avoid here.

We might need a config file for expressing more complex configuration if there's lots of nesting (like a prometheus.yml). Let's see how the block storage and other configuration develops, but maybe YAGNI?

@Todai88 Todai88 merged commit 154f019 into main Nov 30, 2021
@Todai88 Todai88 deleted the feat/introduce-docker-file branch November 30, 2021 16:10
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.

None yet

4 participants