-
Notifications
You must be signed in to change notification settings - Fork 281
HttpClient fixes for Container port and SSL validation #2688
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates the HttpClient configuration to support container ports and SSL validation fixes, including handling self-signed certificates.
- Adds a conditional debugger launch for troubleshooting.
- Enhances HttpClient configuration by reading the ASPNETCORE_URLS environment variable to dynamically set the port.
- Introduces support for allowing self-signed certificates via an environment variable.
Do we expect customers to run their docker adding the following arguments for these bugs to be fixed?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on answers to some usability questions..
@souvikghosh04 I think it would be a good idea to create a task so that we can track this |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the fix @Souvik04
Did we plan to support these environment variables as well? |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with support for ASPNETCORE_URLS. But if we need ASPNETCORE_HTTPS_PORTS, please create a separate PR.
… Usr/sogh/httpclientfixes
…ata-api-builder into Usr/sogh/httpclientfixes
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
@microsoft-github-policy-service agree company="Microsoft" |
.ConfigurePrimaryHttpMessageHandler(serviceProvider => | ||
{ | ||
// For debug purpose, USE_SELF_SIGNED_CERT can be set to true in Environment variables | ||
bool allowSelfSigned = Environment.GetEnvironmentVariable("USE_SELF_SIGNED_CERT")?.Equals("true", StringComparison.OrdinalIgnoreCase) == true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for bool names try to use a prefix such as is
so isSelfSigned
or isSelfSignEnabled
, or something similar.
if (httpContext is not null && | ||
httpContext.Request.Headers.TryGetValue("X-Forwarded-Port", out StringValues fwdPortVal) && | ||
int.TryParse(fwdPortVal.ToString(), out int fwdPort) && | ||
fwdPort > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume you're checking fwdPort > 0
as a sanity check to make sure the port number is valid, but shouldn't we add a check on the upper limit then too, so something like fwdPort > 0 && fwdPort < 65536
?
int colonIndex = trimmed.LastIndexOf(':'); | ||
if (colonIndex != -1 && | ||
int.TryParse(trimmed.Substring(colonIndex + 1), out int wildcardPort) && | ||
wildcardPort > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment as above, should we also check the upper bound for valid ports?
string trimmed = part.Trim(); | ||
|
||
// Handle wildcard format (e.g. http://+:5002) | ||
if (trimmed.StartsWith($"{scheme}://+:", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we handle a weak wildcard format as well which uses *
instead of +
, so http://*:<port>
they're mentioned as an option in these docs
https://learn.microsoft.com/en-us/windows/win32/http/urlprefix-strings
likewise, would http://[::]:<port>
be valid? and if so, could this break the usage of LastIndexOf(':')
? I guess not since its only looking for the last :
- Handle SSL validation failures in Health check when run in untrusted cert and allow option to run with self signed cert - Allow docker container to run on any port internally and allow health check API to use the internal port - Added support for specifying self signed cert (true/false) in environment variable- `USE_SELF_SIGNED_CERT` - Added support for running container on different ports as specified in environment variable- `ASPNETCORE_URLS=http://+:5000` - The internal port is usually 5000 and is set with environment variable `ASPNETCORE_URLS` which is where is the runtime is started - This port is not required to be changed, however, it can be overridden by setting the environment variable `ASPNETCORE_URLS` to `http://+:<internal_port>` - When using `docker run`, the `--publish <external_port>:<internal_port>` must match the internal port, as specified in the `ASPNETCORE_URLS` environment variable - For orchestrated environments, like- ACI, AKS etc, the internal port is set in the same way in environment variable, additionally, the external port is set in the service configuration (or as applicable to the specific orchestration tool) Running on same external and internal port. This was the existing way. ``` docker run --name dab-dev --publish 5000:5000 --detach --mount type=bind,source=$(pwd)/dab-config.json,target=/App/dab-config.json,readonly dab-dev:26may25 ``` Running on different external and internal ports. With this, we can expose an external port which is different than the internal container port. ``` docker run --name dab-dev --publish 1234:5000 --detach --mount type=bind,source=$(pwd)/dab-config.json,target=/App/dab-config.json,readonly dab-dev:26may25 ```  ``` GET http://localhost:1234/health Accept: application/json --------- Co-authored-by: Souvik Ghosh <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
- Handle SSL validation failures in Health check when run in untrusted cert and allow option to run with self signed cert - Allow docker container to run on any port internally and allow health check API to use the internal port - Added support for specifying self signed cert (true/false) in environment variable- `USE_SELF_SIGNED_CERT` - Added support for running container on different ports as specified in environment variable- `ASPNETCORE_URLS=http://+:5000` - The internal port is usually 5000 and is set with environment variable `ASPNETCORE_URLS` which is where is the runtime is started - This port is not required to be changed, however, it can be overridden by setting the environment variable `ASPNETCORE_URLS` to `http://+:<internal_port>` - When using `docker run`, the `--publish <external_port>:<internal_port>` must match the internal port, as specified in the `ASPNETCORE_URLS` environment variable - For orchestrated environments, like- ACI, AKS etc, the internal port is set in the same way in environment variable, additionally, the external port is set in the service configuration (or as applicable to the specific orchestration tool) Running on same external and internal port. This was the existing way. ``` docker run --name dab-dev --publish 5000:5000 --detach --mount type=bind,source=$(pwd)/dab-config.json,target=/App/dab-config.json,readonly dab-dev:26may25 ``` Running on different external and internal ports. With this, we can expose an external port which is different than the internal container port. ``` docker run --name dab-dev --publish 1234:5000 --detach --mount type=bind,source=$(pwd)/dab-config.json,target=/App/dab-config.json,readonly dab-dev:26may25 ```  ``` GET http://localhost:1234/health Accept: application/json --------- Co-authored-by: Souvik Ghosh <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
## Why make this change? This change is made in order to add all of the commits for milestone 1.5 into its respective branch. ## What is this change? This change cherry-picks all of the commits that were added after the first release candidate. Cherry-picked commits: - #2648 #2657 #2617 #2659 #2655 #2633 #2667 #2673 #2650 #2695 #2702 #2688 ## How was this tested? - [ ] Integration Tests - [ ] Unit Tests ## Sample Request(s) --------- Co-authored-by: Sezal Chug <[email protected]> Co-authored-by: sezalchug <[email protected]> Co-authored-by: Tommaso Stocchi <[email protected]> Co-authored-by: Aaron Powell <[email protected]> Co-authored-by: aaronburtle <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]> Co-authored-by: Jerry Nixon <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Michael Staib <[email protected]> Co-authored-by: souvikghosh04 <[email protected]> Co-authored-by: Souvik Ghosh <[email protected]>
Why make this change?
What is this change?
USE_SELF_SIGNED_CERT
ASPNETCORE_URLS=http://+:5000
Usage
ASPNETCORE_URLS
which is where is the runtime is startedASPNETCORE_URLS
tohttp://+:<internal_port>
docker run
, the--publish <external_port>:<internal_port>
must match the internal port, as specified in theASPNETCORE_URLS
environment variableHow was this tested?
Running on same external and internal port. This was the existing way.
Running on different external and internal ports. With this, we can expose an external port which is different than the internal container port.