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

FIX | Adding support for sharedInstances to managed SNI #1237

Merged
merged 14 commits into from
Sep 20, 2021

Conversation

JRahnama
Copy link
Contributor

@JRahnama JRahnama commented Aug 30, 2021

fixes #1231

Manged SNI did not have sharedInstances support for lcoaldb.

  1. in SNI Proxy for localdb the assumption is that when we split the connection string by \\ it always returns an array with length of 2 however when the connection string is set for a shared instance we get an array with a length of 3 since it is in a format of (localdb)\.\shared Instance name

Based on MS Docs all shared instances connection strings should be in a format of
(localdb)\.\ "shared instance's name"
a shared instance could be created as below:
Make sure you have installed SqlLocalDB

  1. Open Developer Command Prompt
  2. Type: SqlLocalDB create "" (you can choose any name)
  3. Type: SqlLocalDB share " your instance name " "provide a name for shared instance"
  4. SqlLocalDB start "name your instance"
  5. Continue either on SSMS or CLI with SqlCmd: for SSMS use (localdb)\your instancename
  6. Create LOGIN [Domain\User]; (run this if the login is not created by default for the user otherwise it fails)
  7. GRANT LOGIN TO [Domain\User]

in the same format one can unshare and delete a shared instance.

One can log into shared instances using ssms with the format mentioned above. Also, Visual studio provides the connection string when you go to View > SQL Server Object Explorer and select local. (It only shows localdb and shared isntances if you have already created one and it is running).

Added: (localdb)\. will login the user into the default localdb.

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 30, 2021

Regex is much faster in 5 but this is a really trivial case and what you really want is just to split the string and then count the number of resulting strings. Regex isn't needed or much easier here.

@JRahnama
Copy link
Contributor Author

Regex is much faster in 5 but this is a really trivial case and what you really want is just to split the string and then count the number of resulting strings. Regex isn't needed or much easier here.

Ok. seems fair. It was an over kill. I've changed it.

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 30, 2021

Is (localdb)\LocalDbAppName a valid name? It looks like it should be from your test input but if I replicate the function GetLocalDBInstance it returns false, perhaps you wanted to negate that if (string.IsNullOrEmpty(..)) check?

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 30, 2021

Is case important in the output instanceName? because if it isn't I think I've got a version of the function which only allocates the output string. If i have to lower case it then I'll need to allocate a stringbuilder.

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 31, 2021

Have a look, see what you think: https://gist.github.com/Wraith2/b2dbcf6af7d21c48620dc00eb0879364

@JRahnama JRahnama self-assigned this Aug 31, 2021
@JRahnama JRahnama added the Area\Managed SNI Issues that are targeted to the Managed SNI codebase. label Aug 31, 2021
@JRahnama JRahnama added this to the 4.0.0-preview2 milestone Aug 31, 2021
@JRahnama
Copy link
Contributor Author

Have a look, see what you think: https://gist.github.com/Wraith2/b2dbcf6af7d21c48620dc00eb0879364

I like the idea, but System.Range is not supported in NetCoreApp 2.1. Although the support is dropped for that version, but it will take some time for us to get there.

I was thinking about this, what is preventing us from getting the very last index of the array?
We can get instanceName = input[^1] or instanceName = tokensByBackSlash[tokensByBackSlash.Length-1]

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 31, 2021

You can simply replace the range expressions with Slice, they're all start offset only so the single arg version works.

We can get the last index if the split works but then what happens if a user passes us a string with 4 slashes in? Invalid yes but will it behave the same? More importantly I was trying to avoid the allocation that the split causes, it's an array and 3 strings all of them on the heap and we know that 2 of the strings are going to get thrown away instantly.

The split version is fine if you want to stick with that. It's not a hot path so it's not going to cause anyone a problem. I just like to see if there are efficient ways to implement things like this, as I did with the azure endpoint parsing.

@JRahnama
Copy link
Contributor Author

JRahnama commented Sep 1, 2021

@Wraith2, FYI ReadOnlySpan is not supported in netstandard 2.0, but installing System.Memory solves the issue.
for ReadOnlySpan.StartsWith Netstandard2.0 does not accept an string type as an argument had to be ReadOnlySpan<char> type. All done. Thanks for the support.

BUILDGUIDE.md Outdated Show resolved Hide resolved
Co-authored-by: Johnny Pham <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area\Managed SNI Issues that are targeted to the Managed SNI codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LocalDB shared instance not supported
5 participants