corefx
corefx copied to clipboard
dotnet
[Release 3.1] | Address Shim gss api on Linux to delay loading libgssapi_krb5.so while using Net6
Ports dotnet/sqlclient#1411 to add shim gss api on Linux following changes in dotnet/runtime#55037
Summary
When working with Kerberos authentication on Unix with recent changes in Net6 users fail to get a connection from Sql server. There is no exception thrown as well. It just crashes.
Customer Impact
This will prevent users from upgrading to Net6 while using Kerberos authentication in Unix.
Testing
Adding tests require some special setup on CI pipelines to install Net6 TFS and enabled Kerberos authentication.
cc: @danmoseley @saurabh500 @David-Engel
I didn't notice you were putting this in 3.1. I believe this is needed in 6.0. That would be the dotnet/runtime repo, in the release/6.0 branch.
@danmoseley
I thought System.Data.SqlClient was removed from runtime after 3.1 and this was the last servicing branch that would contain it. This is where we've backported all SDS fixes in the past: https://github.com/dotnet/corefx/pulls?q=is%3Apr+author%3Acheenamalhotra And there are zero SDS PRs in dotnet/runtime: https://github.com/dotnet/runtime/pulls?q=is%3Aopen+is%3Apr+label%3Aarea-System.Data.SqlClient
Service releases of SDS from this 3.1 branch rely on continued backwards compatibility in .NET 5+.
My assumption was that the fix needed to be in the libraries themselves.
It is possible I didn't think about this clearly (moving too fast). Let's discuss in https://github.com/dotnet/runtime/pull/65469
You might be right. I didn't realize the code change was not in a SqlClient-specific file. There is probably a nuance I'm missing, too.
@David-Engel @danmoseley the branch is open for servicing changes. What should we do about this PR?
What should we do about this PR?
System.Data.SqlClient still needs the fix. But I think the PR needs to be updated. From jkotas' comment:
The delta in this PR affects both the core runtime assemblies and System.Data.SqlClient. The change in core runtime assemblies is unnecessary - it comes with unnecessary risk. If you go with adding the workaround to release/3.1, it should be authored to affect System.Data.SqlClient only.
@JRahnama It sounds like the fix needs to be moved into an appropriate file that only affects System.Data.SqlClient.
This PR needs changes and needs tactics approval before merging.
@ericstj @danmoseley - Changes have been made. What's involved with getting tactics approval for this? We have one PR approval from Jan. Do we need any others?
What's involved with getting tactics approval for this? We have one PR approval from Jan. Do we need any others?
Once it's ready for approval you need to add the servicing-consider label and someone needs to represent in the tactics meeting. When you're ready, reach out to Dan or myself over email and we can share the invite.
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
@David-Engel
- The latest changes in this PR have been signed off by @jkotas .
- Many CI legs failed. Are they related to this change?
- The PR did not have the
servicing-considerlabel and still has the* NO MERGE *label, so it didn't go to Tactics. Please add them if it's ready to go through Tactics.
It will have to wait for the next servicing release because today is the Code Complete due date for July servicing, unless the label is added before the Tactics meeting tomorrow, and the CI question is answered.
@David-Engel @JRahnama The branch is open once again for servicing changes. Can you please address the above items? Otherwise, if the change isn't needed anymore, please close the PR.
@carlossanlop
- Many CI legs failed. Are they related to this change?
I don't believe so. This change is all under SqlClient and the failures appear to be related to other areas.
- The PR did not have the
servicing-considerlabel and still has the* NO MERGE *label, so it didn't go to Tactics. Please add them if it's ready to go through Tactics.
Removed * NO MERGE * and added servicing-consider.
@ericstj could someone please confirm no packaging changes required here?
I didn't intentionally remove @saurabh500 and now it won't let me put him back.
Note that System.Data.SqlClient built from this repo does not go out of support in Dec along with .NET 3.1 -- the sources here remain the latest bits for the package (with a high bar for servicing). Microsoft.Data.SqlClient remains recommended for new code.
@ericstj I wonder whether we should consider whether we should move this code elsewhere (perhaps runtime in the 7.0 branch) so we can fully retire corefx when we move the ASP.NET relevant bits from 2.1 also?
@ericstj I wonder whether we should consider whether we should move this code elsewhere (perhaps runtime in the 7.0 branch) so we can fully retire corefx when we move the ASP.NET relevant bits from 2.1 also?
Some history here: we had set the precedent in 1.x that packages would be maintained in main so that we could keep servicing costs down. Want an update? - just move to latest. In 2.0 we removed many of the packages that were absorbed by the shared framework and netstandard because we knew we'd never need to ship them again - they contained no serviceable code since the implementations moved into the frameworks (.NETCore shared framework and .NETFramework). It was later that some packages were removed that were not absorbed by frameworks - or not all frameworks. These were removed based on the premise that they were "done" and would go out of support when their branch when out of support. It sounds like that's not the case here. What's changed since then? Why are we changing the POR?
we move the ASP.NET relevant bits from 2.1
Where are we moving the ASP.NET relevant bits from 2.1?
I am wondering whether we need to have dotnet/runtime-graveyard repo for packages that are in a perpetual servicing-only mode and that does not really make sense to carry in dotnet/runtime.
@jkotas I was referring to discussion about moving the few bits in this repo that are supported indefinitely as part of ASP.NET 2.1 on .NET Framework into the aspnet repo soon so any servicing can happen in one place. @ericstj is the owner of this plan and can speak to it and correct me if needed.
@JRahnama @DavoudEshtehari please add the OOB package authoring changes so this can merge.
I think none of the test failures are related, but can you please also double check?
please add the OOB package authoring changes so this can merge.
I am not sure if I know what to do to accomplish this. Can you explain a bit more please?
I think none of the test failures are related, but can you please also double check?
The code is inside corefx and as far as I was able to see nothing was related to this change.
@JRahnama you need to include changes that are very similar to the ones @ericstj shared above: https://github.com/dotnet/corefx/commit/ad726016a345c82123141b63bb8fecfc3c887f86
The idea is to bump the version of the package that needs to be generated for your assembly, to ensure it gets properly shipped.
This was probably mentioned already, but do we expect to push this out aligned with core .NET bits (presumably Nov 8th at this point) or just push out on its own? I assume the latter?
Test failures are (of course) unrelated. Some bit rot in unrelated tests, not worth fixing at this point.
This was probably mentioned already, but do we expect to push this out aligned with core .NET bits (presumably Nov 8th at this point) or just push out on its own? I assume the latter?
We have no preference on this item. Whatever is more convenient for the servicing process.