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

Discovery: SQLite-based server implementation #2589

Merged
merged 24 commits into from
Nov 30, 2023
Merged

Conversation

reinkrul
Copy link
Member

@reinkrul reinkrul commented Nov 6, 2023

Base maintainer implementation, not including:

  • HTTP API
  • Checking revocations
  • Registration of the module
  • Diagnostics

Requires

@reinkrul reinkrul force-pushed the usecaselist-maintainer branch from c1c4acf to 8108827 Compare November 6, 2023 16:02
@reinkrul reinkrul requested review from woutslakhorst and gerardsn and removed request for woutslakhorst November 6, 2023 16:02
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

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

This PR is about the usecase maintainer impl but introduces SQL on the fly. The migration towards sql storage is a big one and needs a proper setup. We risk in introducing it with add-ons/patches and forget to properly design it.

Issues that come to mind:

  • put the migrations in sub dirs per supported DB
  • are the migration prefixes with 1, 2, 3 going to break when using 10 ("10"<"2_" stringwise)
  • where are we going to put the models for the tabels, all here or in their own packages?
  • migration file naming
  • connectionstring documentation
  • which DBs to support?
  • additional config for those DBs and can that be added to later on?

SQL introduction should be done in its own PR

storage/config.go Outdated Show resolved Hide resolved
@reinkrul
Copy link
Member Author

reinkrul commented Nov 9, 2023

Extracted SQL functionality into a separate PR: #2595

@reinkrul reinkrul force-pushed the usecaselist-maintainer branch from bcae7aa to 64d1cf4 Compare November 12, 2023 13:14
@reinkrul reinkrul marked this pull request as draft November 17, 2023 07:02
@reinkrul reinkrul force-pushed the usecaselist-maintainer branch 3 times, most recently from b84d289 to 5932201 Compare November 19, 2023 08:08
@reinkrul reinkrul force-pushed the usecaselist-maintainer branch from eb54df9 to 9dacfc4 Compare November 20, 2023 06:05
@reinkrul reinkrul changed the title Usecase: SQLite-based maintainer implementation Discovery: SQLite-based server implementation Nov 20, 2023
@reinkrul
Copy link
Member Author

Refactored with server and client having 1 store.

@reinkrul reinkrul requested a review from gerardsn November 20, 2023 06:06
@reinkrul reinkrul marked this pull request as ready for review November 20, 2023 06:06
README.rst Outdated Show resolved Hide resolved
discoveryservice/api/v1/api.go Outdated Show resolved Hide resolved
discoveryservice/config.go Outdated Show resolved Hide resolved
discoveryservice/config.go Outdated Show resolved Hide resolved
vcr/pe/schema/v2/schema.go Outdated Show resolved Hide resolved
discoveryservice/module.go Outdated Show resolved Hide resolved
discoveryservice/module.go Outdated Show resolved Hide resolved
discoveryservice/store.go Outdated Show resolved Hide resolved
discoveryservice/store.go Outdated Show resolved Hide resolved
discoveryservice/store.go Outdated Show resolved Hide resolved
@reinkrul
Copy link
Member Author

note to self: check that presentation.ID is scoped to the signer DID

@reinkrul
Copy link
Member Author

First feedback processed

Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

A PR like this takes me a full day to review to cover about 80% of the content. Adding more comment along the way or summarizing behavior of longer functions/methods (also private ones) would help me a lot.

discovery/test/invalid_definition/README.md Outdated Show resolved Hide resolved
storage/sql_migrations/2_discoveryservice.up.sql Outdated Show resolved Hide resolved
discovery/definition.go Outdated Show resolved Hide resolved
discovery/definition.go Outdated Show resolved Hide resolved
discovery/interface.go Show resolved Hide resolved
discovery/module.go Outdated Show resolved Hide resolved
vcr/pe/presentation_definition.go Outdated Show resolved Hide resolved
discovery/store.go Outdated Show resolved Hide resolved
discovery/store.go Show resolved Hide resolved
discovery/config.go Show resolved Hide resolved
discovery/config.go Show resolved Hide resolved
discovery/module.go Outdated Show resolved Hide resolved
vcr/pe/presentation_definition.go Outdated Show resolved Hide resolved
vcr/pe/presentation_submission.go Outdated Show resolved Hide resolved
vcr/pe/schema/v2/schema.go Show resolved Hide resolved
Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

comments are suggestions. Requesting changes because retracted_jti is not a DID (see previous comments)

discovery/module.go Show resolved Hide resolved
Comment on lines +146 to +148
if err := s.prune(); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

it depends how care orgs will be allowed to access discovery services. As long as care orgs can't use use-cases that aren't configured in a service definition I suppose there is no issue.

rationale: If a node only acts as client (applies to most) and all service definitions are gone after a restart (config error), the db will only contain outdated info. The client will also no longer publish its clients to the discovery services. This could result in weird situations where orgs can find orgs managed by other vendors but not their own, etc..

@reinkrul reinkrul requested a review from gerardsn November 30, 2023 13:38
@reinkrul reinkrul merged commit d1bc3af into master Nov 30, 2023
9 checks passed
@reinkrul reinkrul deleted the usecaselist-maintainer branch November 30, 2023 14:00
woutslakhorst added a commit that referenced this pull request Dec 12, 2023
additional tests

additional test

PR feedback

PR feedback

test fix

remove failing test (#2618)

added e2e test for OpenID4VP s2s flow (#2617)

PEX: Provide ParseEnvelope to correctly parse PEX VP envelopes (#2620)

Bump schneider.vip/problem from 1.8.1 to 1.9.0 (#2622)

Bumps [schneider.vip/problem](https://github.com/mschneider82/problem) from 1.8.1 to 1.9.0.
- [Commits](mschneider82/problem@v1.8.1...v1.9.0)

---
updated-dependencies:
- dependency-name: schneider.vip/problem
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump golang.org/x/crypto from 0.15.0 to 0.16.0 (#2628)

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.15.0 to 0.16.0.
- [Commits](golang/crypto@v0.15.0...v0.16.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump golang.org/x/time from 0.4.0 to 0.5.0 (#2627)

Bumps [golang.org/x/time](https://github.com/golang/time) from 0.4.0 to 0.5.0.
- [Commits](golang/time@v0.4.0...v0.5.0)

---
updated-dependencies:
- dependency-name: golang.org/x/time
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Missing rfc021 e2e tests changes (#2623)

Discovery: SQLite-based server implementation (#2589)

Bump github.com/nuts-foundation/go-leia/v4 from 4.0.0 to 4.0.1 (#2632)

Bumps [github.com/nuts-foundation/go-leia/v4](https://github.com/nuts-foundation/go-leia) from 4.0.0 to 4.0.1.
- [Release notes](https://github.com/nuts-foundation/go-leia/releases)
- [Commits](nuts-foundation/go-leia@v4.0.0...v4.0.1)

---
updated-dependencies:
- dependency-name: github.com/nuts-foundation/go-leia/v4
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Root URL server config property to replace auth.publicurl (#2633)

change idToDID to use did:web in iam API (#2635)

Bump github.com/lestrrat-go/jwx/v2 from 2.0.17 to 2.0.18 (#2645)

Bumps [github.com/lestrrat-go/jwx/v2](https://github.com/lestrrat-go/jwx) from 2.0.17 to 2.0.18.
- [Release notes](https://github.com/lestrrat-go/jwx/releases)
- [Changelog](https://github.com/lestrrat-go/jwx/blob/develop/v2/Changes)
- [Commits](lestrrat-go/jwx@v2.0.17...v2.0.18)

---
updated-dependencies:
- dependency-name: github.com/lestrrat-go/jwx/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump github.com/nats-io/nats-server/v2 from 2.10.5 to 2.10.6 (#2644)

Bumps [github.com/nats-io/nats-server/v2](https://github.com/nats-io/nats-server) from 2.10.5 to 2.10.6.
- [Release notes](https://github.com/nats-io/nats-server/releases)
- [Changelog](https://github.com/nats-io/nats-server/blob/main/.goreleaser.yml)
- [Commits](nats-io/nats-server@v2.10.5...v2.10.6)

---
updated-dependencies:
- dependency-name: github.com/nats-io/nats-server/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump alpine from 3.18.4 to 3.18.5 (#2636)

Bumps alpine from 3.18.4 to 3.18.5.

---
updated-dependencies:
- dependency-name: alpine
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

more NO_CONTENT

timeout godoc
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.

3 participants