GoogleCloudPlatform / buildpacks

Builders and buildpacks designed to run on Google Cloud's container platforms
Apache License 2.0
985 stars 146 forks source link

Buildpack for dotnet should add a default listening URL/Port environment variable for ASPNET Core. #317

Open idofl opened 1 year ago

idofl commented 1 year ago

When using the dotnet runtime container images from Microsoft (mcr.microsoft.com/dotnet/runtime:7.0 and 6.0) the Dockerfile sets an environment variable, ASPNETCORE_URLS, to override the default listening port of ASPNET Core from 5000 to 80 (https://github.com/dotnet/dotnet-docker/blob/main/samples/aspnetapp/README.md#build-an-aspnet-core-image , https://github.com/dotnet/dotnet-docker/blob/a4d4929b8d6b7cdca35af6d71c9d5b79765aadc9/src/runtime-deps/6.0/bullseye-slim/amd64/Dockerfile#L5).

The buildpack for dotnet does not set the environment variable, and because of this, the default port remains 5000 (https://learn.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel/endpoints?view=aspnetcore-7.0).

This creates a difference in developer experience when developers use the buildpacks vs. when they create a Dockerfile with the standard MCR images. This difference in default behavior may lead to developers failing to host their apps in Cloud Run, if developers are not aware to the change, and keep their deployment configuration the same when switching from Dockerfile to buildpacks or vice versa.

Recommendation:

  1. Add the ASPNETCORE_URLS for runtime versions <= 7.x, and set the default port to 80.
  2. For runtime version >= 8.x, the new env variable is ASPNETCORE_HTTP_PORTS and its default value in the MCR images is set to 8080 (https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8#non-root-user , https://github.com/dotnet/dotnet-docker/blob/a4d4929b8d6b7cdca35af6d71c9d5b79765aadc9/src/runtime-deps/8.0/bookworm-slim/amd64/Dockerfile#LL7C24-L7C24).
jahjahbin commented 1 year ago

Hi, thanks for raising this. I can see how the difference might cause some difficulties switching between our buildpacks and MCR.

Just wondering, have you seen the Cloud Run docs on the PORT envar? https://cloud.google.com/run/docs/container-contract#port We also provide some guidance on the port in our Cloud Run quickstart guide for .NET: https://cloud.google.com/run/docs/quickstarts/build-and-deploy/deploy-dotnet-service#writing

I feel this is more of an enhancement or feature request, but happy to discuss things further.

idofl commented 1 year ago

I agree that if developers follow our Cloud Run quickstart guide, and apply the $port env var, then this will enable the code to run with the proper port, regardless of the default port setting in the container image. However, there are additional ways for developers to start their journey:

In any of the above cases, if the developers decide to switch from a dockerfile to build packs, they will encounter the inconsistent behavior.

I agree this is more of a feature request than a bug. You can change the type.

jahjahbin commented 1 year ago

Makes sense to me. Since developers can use this for platforms other than Cloud Run, even outside of GCP, I think we'll have to think about a good place to put these defaults -- maybe a Cloud Run order group (and the respective order groups for App Engine Standard/Flex/GCF)?

jama22 commented 1 year ago

Hey folks sorry just catching up on this thread. Not very familiar with the .NET workflow, so apologize in advance for some basic questions @idofl . Is there ever a situation where we don't want to set ASPNETCORE_URLS=80? I'm wondering if there's cases where you want one value for local development (building a container locally with pack) vs when you deploy remotely (deploy to Cloud Run)

In the short term, users can also manually set this value by including a project.toml file setting the ASPNETCORE_URLS to the appropriate value, that's documented in in GCP Buildpacks docs

If we think this is generally a sane default for all .NET core buildpacks, we can make that change and document under language-specific configurations. I don't have a page set up for .NET yet, but I"m happy to volunteer the time to create that content.

idofl commented 1 year ago

@jama22 When we used the build packs for our .NET code, we expected to get a container image that has a similar configuration/behavior as the official dotnet container images. The default behavior of the MCR dotnet images is indeed to set ASPNETCORE_URLS=80. Please note that as of dotnet 8.0, the MCR images set the variable to 8080 (images for version 7 and below still use 80).

I think it is a logical expectation that the resulting build pack image has similar configuration as the original image offered by Microsoft, otherwise it may confuse people who are migrating from MCR images to build packs.

jama22 commented 1 year ago

Okay, so it sounds like the feature is to set ASPNETCORE_URLS=80 for .NET 6 and .NET ~7~ 8 (when we release later this year). Future versions .NET 8 and above should default to ASPNETCORE_URLS=8080

This could be a good first issue for someone to tackle on this project

realhaik commented 1 year ago

.NET 7 (when we release later this year). @jama22 You are certainly taking your time, because 7 was released a year ago, and this year is also almost over. I think the only sensible solution now will be to skip 7 altogether and release 8 instead.

jama22 commented 1 year ago

@realhaik sorry i got my versions mixed up. We are indeed skipping 7 and releasing 8