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

Local file-based name resolver with SQLite #3178

Merged
merged 21 commits into from Nov 1, 2023

Conversation

ItalyPaleAle
Copy link
Contributor

Fixes #1380
Will fix (when runtime PR is merged) dapr/dapr#3256

Addresses a longstanding request to have an alternative name resolver component for local development, for situations where mDNS can't be used (and even in those situations, a local name resolver is more convenient for local development).

As discussed in #1380, this is implemented with SQLite, which is fast and efficient.

PR includes tests implemented via unit tests, since we don't have an infrastructure for writing conformance or certification tests for name resolvers.

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle requested review from a team as code owners October 13, 2023 23:04
@ItalyPaleAle
Copy link
Contributor Author

All cert tests are failing because the APIs changed and the pinned runtime needs to be updated. Usual problem :) However nothing changed here that would impact cert tests

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Copy link
Contributor

@JoshVanL JoshVanL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments from me

nameresolution/metadata.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite_migrations.go Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
ItalyPaleAle and others added 4 commits October 13, 2023 20:11
Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
See dapr#3179

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
nameresolution/consul/consul_test.go Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Outdated Show resolved Hide resolved
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
internal/authentication/sqlite/metadata.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Show resolved Hide resolved
return err
}

// Show a warning if SQLite is configured with an in-memory DB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think IsMemory should be allowed, as Service Invocation across different Apps, a basic use case, will not work?
Or is this development just for development scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not disable it outright, although yes, it kind of defeats the point of svc invocation across multiple apps (but can be used to test an app invoking itself). We also use that for unit testing.

nameresolution/mdns/mdns.go Outdated Show resolved Hide resolved
nameresolution/sqlite/sqlite.go Show resolved Hide resolved
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle added this to the v1.13 milestone Oct 30, 2023
Signed-off-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
@berndverst
Copy link
Member

If these new unit tests are slow we might want to move them to be some sort of pseudo certification tests.

@berndverst berndverst merged commit 8680e27 into dapr:master Nov 1, 2023
9 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants