external authorization: set the SNI value from server name if it isn't available on the connection/socket

[marc-barry]

This is my first commit to the Envoy project and I haven't written C++ in many years. I'm still navigating the types and hierarchy and best practices for obtaining the data I need to complete this pull request. I have started in draft first to see the CI processes and look at how the tests run and if my changes cause any issues with current tests.

Commit Message: external authorization: set the SNI value from server name if it isn't available on the connection/socket
Additional Description: Leverages the TLS inspectors server name value, if one was set.
Risk Level: low
Testing: Will test that the SNI value is set from the server name of a connection when the TLS session doesn't have the SNI.
Docs Changes: N/A
Release Notes: exterbal authorization
Platform Specific Features: N/A
[Optional Runtime guard:]
Fixes #34002
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

Hi @marc-barry, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #34100 was opened by marc-barry.

see: more, trace.

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34100 was opened by marc-barry.

see: more, trace.

[marc-barry]

I'm unable to determine which of the unit tests in envoy/test/extensions/filters/common/ext_authz/check_request_utils_test.cc is failing. I know that is the location that CI reports to me but I don't know the Envoy CI system well enough to know how to determine which tests is the issue.

[fredyw]

I'm unable to determine which of the unit tests in envoy/test/extensions/filters/common/ext_authz/check_request_utils_test.cc is failing. I know that is the location that CI reports to me but I don't know the Envoy CI system well enough to know how to determine which tests is the issue.

A passing by comment:

In the envoy-presubmit Azure Pipelines, click on View more details on Azure Pipelines at the bottom. That'll bring you to the Azure Devops Pipelines and simply click on the error.

This is the actual error: https://dev.azure.com/cncf/envoy/_build/results?buildId=170326&view=logs&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=d1a98671-b7ba-5fbf-f06c-ff337c010df4&l=249

[ggreenway]

Thanks for working on this!

Please add a test case for this new logic, and you'll need to add a release note.

/wait

[ggreenway]

Please add some test coverage for the new functionality.

/wait

[marc-barry]

I'm working on tests now. Both for the new functionality and addressing the tests that this new functionality is causing to fail.

[marc-barry]

@ggreenway I addressed all you comments. I added a new unit test for TCP and then adapted callHttpCheckAndValidateRequestAttributes, which is called by the HTTP tests, to conditionally perform a check using the correct source of the SNI value for comparison. I also adapted the existing CheckAttrContextPeerTLSSession test to have some additional checks that certain functions are called a certain number of times, such as requestedServerName().

[ggreenway]

Looks good!

Please add a release note to changelogs/current.yaml.

/wait

[marc-barry]

@ggreenway is there anything I need to do for that failing check? It does look to me that it might be CI flakiness but perhaps there is an underlying issue that needs my attention.

[ggreenway]

That's a weird error; pretty sure you didn't cause it. I'm re-running the failing job to see if it passes.