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

Consider adding an equivalence of EXPOSE #472

Open
tcnghia opened this issue Oct 11, 2021 · 10 comments
Open

Consider adding an equivalence of EXPOSE #472

tcnghia opened this issue Oct 11, 2021 · 10 comments

Comments

@tcnghia
Copy link
Contributor

tcnghia commented Oct 11, 2021

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

@tcnghia tcnghia changed the title Consider adding equivalence of EXPOSE Consider adding an equivalence of EXPOSE Oct 11, 2021
@imjasonh
Copy link
Member

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
Copy link
Contributor Author

tcnghia commented Oct 11, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

jonjohnsonjr commented Oct 11, 2021

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 #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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link

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
Copy link

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.

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

No branches or pull requests

5 participants