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

ApplicationLoadBalancerFunction doesn't play nicely with healthchecks #436

Closed
aidansteele opened this issue Apr 9, 2019 · 4 comments
Closed
Labels
bug This issue is a bug.

Comments

@aidansteele
Copy link

Hi,

Background:

Target groups with Lambda targets don't have health checks turned on by default. If you turn on health checks for an ASP.Net Core app using the ApplicationLoadBalancerFunction.FunctionHandlerAsync handler, health checks will fail and the Lambda will emit exception stack traces.

The Lambda request payload for health checks is documented here as looking like:

{
    "requestContext": {
        "elb": {
            "targetGroupArn": "arn:aws:elasticloadbalancing:region:123456789012:targetgroup/my-target-group/6d0ecf831eec9f09"
        }
    },
    "httpMethod": "GET",  
    "path": "/",  
    "queryStringParameters": {},  
    "headers": {
        "user-agent": "ELB-HealthChecker/2.0"
    },  
    "body": "",  
    "isBase64Encoded": false
}

Problem:

This line of code is what parses the scheme:

requestFeatures.Scheme = GetSingleHeaderValue(lambdaRequest, "x-forwarded-proto");

After this line runs for the above request payload, requestFeatures.Scheme is still null. This causes a null reference exception here:

https://github.com/aspnet/Hosting/blob/1da228d28fdfcfb2d77241c4e0413efec1beeafd/src/Microsoft.AspNetCore.Hosting/Internal/HostingRequestStartingLog.cs#L37

The properties after Scheme probably fail too, but this is the first to fail.

Solution:

Given that ASP.Net Core appears to treat these properties as non-null, we should probably populate them unconditionally. Any value will be better than no value. Any thoughts about how you'd best like to approach this? I'd be happy to submit a PR fixing it with unit test coverage.

@normj normj added the guidance Question that needs advice or information. label Apr 23, 2019
@normj
Copy link
Member

normj commented Apr 23, 2019

That is an interesting one. I'm afraid to just default the value to https in case there is a situation that is not correct. I wonder if we should look to see if the user-agent starts with "ELB-HealthChecker" and if so default schema to https.

@normj
Copy link
Member

normj commented Apr 29, 2019

PR is submitted to handle ELB Health Check: #452

@normj
Copy link
Member

normj commented May 1, 2019

Version 3.0.4 is out with this fix.

@normj
Copy link
Member

normj commented May 7, 2019

Closing as the fix was shipped.

@normj normj closed this as completed May 7, 2019
@klaytaybai klaytaybai added bug This issue is a bug. and removed guidance Question that needs advice or information. labels May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

No branches or pull requests

3 participants