ko-build / ko

Build and deploy Go applications
https://ko.build
Apache License 2.0
7.54k stars 396 forks source link

Consider adding an equivalence of EXPOSE #472

Open tcnghia opened 2 years ago

tcnghia commented 2 years ago

Per https://docs.docker.com/engine/reference/builder/#expose:

The EXPOSE instruction does not actually publish the port. It functions as a type of documentation between the person who builds the image and the person who runs the container, about which ports are intended to be published.

Some container service (like Azure App Service) consumes this metadata to automatically configure the correct port-mapping to the container. Without this metadata, users of these service will need to set this on the deployment side.

cc @jonjohnsonjr

imjasonh commented 2 years ago

I have no particular problem with this.

I wonder how we'd like to let users configure this. In .ko.yaml probably, per-importpath?

tcnghia commented 2 years ago

The builds section currently has per-importpath configs, and I feel that it's the best place now. However I don't know how close we want to be to the GoReleaser format.

(adding another section with per-importpath configs seems error prone and may be confusing too)

jonjohnsonjr commented 2 years ago

I would like to get @caarlos0 to weigh in on some things, maybe.

IMO, if we can expose enough config like this to reasonably tell someone "you really don't need a Dockerfile", it wouldn't be a bad idea for goreleaser to use ko for image builds by default (maybe let you drop into docker if you need to do stupid things).

However, I'd like to avoid collisions within their config, and by borrowing their config format, it's on us to make sure we don't cause any collisions.

First thoughts that come to mind:

builds:
- id: foo
  main: ./foobar/foo
  container:
    annotations:
      best: image
    author: "ImJasonH"
    config:
      ExposedPorts:
        8080/tcp: {}
      Labels:
        foo: bar
      Env:
        -"foo=bar"

Basically just overlay the config file as yaml atop whatever ko builds so you can override anything. What's annoying is that this should maybe be:

builds:
- id: foo
  main: ./foobar/foo
  container:
    config:
      config:
        ...

Because the config file has a config field, and we might not want manifest-level things (annotations) to overlap with config-level things (author, created).

We could also have a parallel structure:

builds:
- id: foo
  main: ./foobar/foo
containers:
- id: foo
  manifest:
  config:
    config:

Or something?

caarlos0 commented 2 years ago

I'm not well versed in ko, but I would think that it depends on wether you want to allow to create an image with several builds in it...

if yes, I would go with something like:

builds:
- id: foo
  main: ./foobar/foo
- id: bar
  main: ./foobar/bar
containers:
- id: foobar
  builds: [foo, bar] # or maybe `ids`?
  manifest:
    # options here...

very inspired in how we currently link things in goreleaser...


fwiw, I was thinking in how to best integrate ko with goreleaser as well, and I think having another high level config + using the same syntax for builds would allow to basically "include" the ko.yaml into goreleaser.yaml, but that's maybe another thread... happy to discuss it somewhere else if it interests you as well :)

jonjohnsonjr commented 2 years ago

but I would think that it depends on wether you want to allow to create an image with several builds in it...

if yes, I would go with something like:

This might be a way to tackle https://github.com/google/ko/issues/251

very inspired in how we currently link things in goreleaser...

Can you elaborate on this a bit?

using the same syntax for builds would allow to basically "include" the ko.yaml into goreleaser.yaml, but that's maybe another thread...

Yeah that's definitely the overall goal in my mind -- just merge the two files (maybe a symlink) if at all possible.

happy to discuss it somewhere else if it interests you as well :)

100% yes, there's a #ko channel in knative slack and a #ko-project channel on k8s slack.

caarlos0 commented 2 years ago

Currently goreleaser builds the binaries locally already (and the build config is very similar to ko's)...

Let's say we go with another high-level entry for the EXPOSE et all (e.g. containers like in the example), the ko config would be something like this:

builds:
- id: foo
  main: ./foobar/foo
- id: bar
  main: ./foobar/bar
containers:
- id: foobar
  builds: [foo, bar] # or maybe `ids`?
  manifest:
    # options here...

Then, lets say you want to use goreleaser to manage other stuff as well, IF configs are compatible, it would be basically goreleaser release -f ko.yaml, or the user could use the includes feature to include ko.yaml into goreleaser.yaml and both would work...

at least, that was my general idea... 🤔

jonjohnsonjr commented 2 years ago

Yep yep, 100% this is the goal. Given that goreleaser does an excellent job of exposing everything you might need to configure for a go build, we just stole it rather than reinventing the wheel. As long as goreleaser is able to ignore unknown fields (e.g. the ko config), you should be able to use the same config file for both tools.

One kind of strange bit is that ko doesn't use goreleaser to drive the builds themselves -- we have implemented a bunch of this logic on our own. (Thinking out loud) I wonder if it would be possible to pull out part of goreleaser into a package such that ko could just depend on it directly and dispatch to goreleaser for the builds and only handle the container packaging bits.

I think this makes things a little weird, and also some ko-isms like bundling kodata/, but maybe that's fine if we can slice up the dependencies appropriately. I'm just trying to avoid circular dependencies without making this too complicated.

(Probably time to pull this into a separate issue...)

caarlos0 commented 2 years ago

the build is logic is already more or less isolated: https://github.com/goreleaser/goreleaser/tree/master/internal/builders/golang

there are a few internal dependencies (tmpl and buildtarget) that would need to be sorted out, but if you see that as being useful, I'm sure we can work something out...

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Keep fresh with the 'lifecycle/frozen' label.

mbrancato commented 1 year ago

Coming here to comment that when using tools like Knative without the EXPOSE functionality, we cannot hit things like pod metric endpoint directly. This means we get the proxy sidecar as the "container" label of what is reporting the metric, so it looks incorrect.