Map "binding.ContainerPort" instead of "binding.Port" to "manifest containerPort"

[tebeco]

fixes #725

It seems that the manifest code was mapping binding.Port" to the field "containerPort" inthe KubernetesManifestGenerator`
As I have, for now, a limited understanding on that, There might be other underlying reason that I'm missing here

Adding back context from the Issue here for review:

From @jongio
#725 (comment)

Here's what I recommend:

If tye.yaml has containerPort, then use it for both containerPort in the deployment and targetPort in the service
If tye.yaml doesn't have containerPort, then use 80 for http and 443 for https for both containerPort and targetPort.

I personally do not see a scenario where containerPort or targetPort should be equal to port

@jkotalik to confirm the logic above, but @tebeco if you want to take a stab at updating to that, you are more than welcome to mod your PR.

This PR only covers HTTP scheme and not HTTPS due to the current absence of support yet for HTTPS :

                        if (binding.Protocol == "https")
                        {
                            // We skip https for now in deployment, because the E2E requires certificates
                            // and we haven't done those features yet.
                            continue;
                        }

What troubles me ?
tye.yaml seem to have the notion of port binding that seems to be used for local run when doing tye run
What happens when there's a port: 1111 specified but no containerPort ? Should it default to 80 ? or should it use the existing/defined port ?
eg:

name: foobar
services:
- name: thewebapione
  project: src/TheWebapiOne/TheWebapiOne.csproj
  bindings:
    - port: 1111
      protocol: http

This would feel like a dup to have to do:

name: foobar
services:
- name: thewebapione
  project: src/TheWebapiOne/TheWebapiOne.csproj
  bindings:
    - port: 1111
      protocol: http
      containerPort: 1111

[jkotalik]

I think we prefer what you currently have, not defaulting to port 80. Could you clean that up and we'll verify.

[tebeco]

done

[JunTaoLuo]

nit: new line not needed.

[tebeco]

🤨

[tebeco]

thx, that will teach me to double check when editing directly from the portal :D
(Not on the head please)

[tebeco]

What's the proper way to dig in this error ?
Download artifacts and open the html report ?

[jkotalik]

Modify CombineStep.cs line 41 to be:

                    var port = binding.ContainerPort ?? binding.Port ?? 80;

Currently, this will not work because the app will be listening on port 1111 instead of port 2222 in your example.

[tebeco]

will do that right after dinner ;)
sorry about that

[jkotalik]

There should be a test view just above to view failures. https://dev.azure.com/dnceng/public/_build/results?buildId=873333&view=ms.vss-test-web.build-test-results-tab&runId=27940212&resultId=100012&paneView=debug. Looks like a flaky test though.

[jkotalik]

Will approve once that done!

[tebeco]

can you enlight me about this ENV_VAR ?

project.EnvironmentVariables.Add(new EnvironmentVariableBuilder("PORT") { Value = port.ToString(CultureInfo.InvariantCulture), });

I saw it when i did tye generate but I'm not sure about the underlying reason of it as the ASPNETCORE_URLS already cover the http(s) port

[tebeco]

I decided to lookup binding.Port accross the repo, and now I'm curious :D

Should I also change the annotation for DAPR ?
This code seems to be use "when it's NOT LOCAL" meaning probably after tye generate or tye deploy
https://github.com/dotnet/tye/blob/master/src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs#L193

[tebeco]

To be honest I attempted to review all usage of BindingBuilder.Port` accross the repo, and I'm not 100% sure for each usage (yet) :(

[jkotalik]

I believe PORT is used in some applications as well (maybe by kestrel). I'd have to double check though.