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

feat: configuration validation #1778

Merged
merged 33 commits into from
Oct 5, 2023

Conversation

maschad
Copy link
Member

@maschad maschad commented May 29, 2023

Closes #1573
Closes #1757
Closes #1903

I decided to go with yup for a few reasons:

  • Integration was more seamless
  • Is used by substantially more projects ( 545k vs 105k)
  • Preferred the schema building via chaining as opposed to defining complex functions (especially for defining integers, mins and defaults)
  • Was more actively maintained ( most recent commit 3 weeks ago as opposed to last year November
  • More downloads ( 4,201,246 vs 889,754 weekly downloads)

@maschad maschad changed the title Feat/configuration-validation feat: configuration validation May 29, 2023
@maschad maschad requested a review from achingbrain July 4, 2023 00:03
@maschad maschad self-assigned this Jul 4, 2023
@maschad maschad marked this pull request as ready for review July 4, 2023 00:03
@maschad
Copy link
Member Author

maschad commented Jul 4, 2023

Currently blocked by #1867 unblocked

@maschad maschad marked this pull request as draft July 4, 2023 21:21
@maschad maschad marked this pull request as ready for review July 5, 2023 00:05
@achingbrain
Copy link
Member

achingbrain commented Aug 15, 2023

Looking good.

What's the thinking behind this pattern?:

class ServiceClass implements MyService {
  constructor (components, init) {
     // use init
  }
}

export function myService (init: MyServiceInit = {}): (components: MyComponents) => MyService {
  const validatedConfig = object({
    // config def
  }).validateSync(init)

  return (components) => {
    return new ServiceClass(components, validatedConfig)
  }
}

The thing that looks wonky to me is that the class doesn't validate it's args, it's done in the factory instead, so the class has to trust whatever it's given.

The config schema is also created for each object instantiation instead of having one definition that all instances can use - does it retain state or something?

Why not do something like:

const configSchema = object({
  // config def
})

class ServiceClass implements MyService {
  constructor (components, init) {
     init = configSchema.validate(init)
     // use init
  }
}

export function myService (init: MyServiceInit = {}): (components: MyComponents) => MyService {
  return (components) => {
    return new ServiceClass(components, init)
  }
}

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

See the comment above..

@maschad
Copy link
Member Author

maschad commented Aug 18, 2023

The thing that looks wonky to me is that the class doesn't validate it's args, it's done in the factory instead, so the class has to trust whatever it's given.

The config schema is also created for each object instantiation instead of having one definition that all instances can use - does it retain state or something?

If we continue to use factory method pattern with a 1-to-1 mapping of service to method then this way doesn't offer any benefits but also there isn't any drawback of the service object trusting the parameters it was passed (since only one method would pass those parameters in).

The config schema is also created for each object instantiation instead of having one definition that all instances can use - does it retain state or something?

Yup's schema is immutable, meaning that modifications to the schema will result in new schema instances rather than altering the original schema. So I can see the drawback of creating one for each object instantiated.

I don't have a strong opinion on doing it this way so if the memory savings outweigh the potential flexibility to have more factory methods then I can refactor.

I decided to refactor this given that we can also remove the need for re-setting the defaults in the constructor of the classes since yup returns an object with defined values for each field.

@maschad maschad requested a review from achingbrain August 21, 2023 02:04
@maschad maschad requested a review from a team September 3, 2023 23:28
@maschad
Copy link
Member Author

maschad commented Sep 15, 2023

@achingbrain would you be able to give this a review please?

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

I don't see any major errors but I have a few questions:

  1. Are the changes in the *.spec.* files fixes to incorrect configs set during testing, that were found because of these changes? If so, that's great.
  2. What is the bundle size increase with this?
  3. Are there any hot-paths that we need to look out for?
    • things where instantiation happens frequently and config validation is re-run (and not needed)? If there are, I missed them.

packages/libp2p/src/circuit-relay/server/index.ts Outdated Show resolved Hide resolved
packages/libp2p/src/config/config.ts Show resolved Hide resolved
this.keepAlive = init.keepAlive ?? true
this.gateway = init.gateway

const validIPRegex = /^(?:(?:^|\.)(?:\d|[1-9]\d|1\d{2}|2[0-4]\d|25[0-5])){4}$/
Copy link
Member

Choose a reason for hiding this comment

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

would love to see a link to the tests around the regex similar to https://www.regextester.com/104038 or https://regex101.com/r/aL7tV3/1 or similar

Copy link
Member Author

Choose a reason for hiding this comment

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

That link should suffice :) Normally I would agree with you but since it's for validation as opposed to a regex used in a function, the validation is a test in and of itself imo

Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
@maschad
Copy link
Member Author

maschad commented Oct 5, 2023

I don't see any major errors but I have a few questions:

  1. Are the changes in the *.spec.* files fixes to incorrect configs set during testing, that were found because of these changes? If so, that's great.

That's correct.

  1. What is the bundle size increase with this?

Gzipped and minified yup is 12kB which is negligible

  1. Are there any hot-paths that we need to look out for?

    • things where instantiation happens frequently and config validation is re-run (and not needed)? If there are, I missed them.

This would only instantiate one validation object per class that's already instantiated, which again is insignificant, any optimizations in that regard would be peripheral to this.

@maschad maschad merged commit e9099d4 into libp2p:master Oct 5, 2023
17 checks passed
@maschad maschad deleted the feat/configuration-validation branch October 5, 2023 23:34
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

This was not ready. I know it's taken a long time but other issues have been higher priority. Please wait for review before merging.

packages/libp2p/src/fetch/index.ts Show resolved Hide resolved
Comment on lines +28 to +29
maxIncomingPendingConnections: 1000,
maxConnections: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

The default value for these was fine before, why do these now need to be set?

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment #1778

packages/libp2p/src/circuit-relay/server/index.ts Outdated Show resolved Hide resolved
this.keepAlive = init.keepAlive ?? true
this.gateway = init.gateway

const validIPRegex = /^(?:(?:^|\.)(?:\d|[1-9]\d|1\d{2}|2[0-4]\d|25[0-5])){4}$/
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a function for validation instead of a regex? Then reuse something like @chainsafe/is-ip

Copy link
Member Author

Choose a reason for hiding this comment

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

what's the benefit of using a external dependency and parsing the string over a regular expression?

@@ -235,7 +235,9 @@ describe('libp2p.connections', () => {
},
connectionManager: {
minConnections,
maxConnections: 1
maxConnections: 1,
inboundConnectionThreshold: 1,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to specify the extra config now? This seems to happen in a few places here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the intention is that we should not be able to have more inbound connections than maxConnections as that would result in them getting pruned anyways, discussing with @wemeetagain he raised a use case where this may be applicable though, where a node may want to reach maxConnections as fast as possible and so it may accept a greater amount of inbound connections in a short space of time.

@@ -24,7 +24,9 @@ export default {
const peerId = await createEd25519PeerId()
const libp2p = await createLibp2p({
connectionManager: {
inboundConnectionThreshold: Infinity,
inboundConnectionThreshold: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

I take it yup doesn't accept Infinity as a number?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

@@ -108,14 +109,23 @@ class DefaultAutoNATService implements Startable {
private started: boolean

constructor (components: AutoNATComponents, init: AutoNATServiceInit) {
const validatedConfig = object({
Copy link
Member

Choose a reason for hiding this comment

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

Please have config definitions outside of the class so we don't create new validators on every object instantiation:

const configValidator = object({
  protocolPrefix: string().default(PROTOCOL_PREFIX),
  // ...etc
})

class DefaultAutoNATService implements Startable {
  // ...etc

  constructor (components: AutoNATComponents, init: AutoNATServiceInit) {
    const config = configValidator.validateSync(init)
    // ...etc
  }
}

achingbrain pushed a commit that referenced this pull request Oct 6, 2023
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
@achingbrain
Copy link
Member

Reverted and re-opened as #2133

@maschad maschad requested a review from wemeetagain October 6, 2023 16:29
maschad added a commit to maschad/js-libp2p that referenced this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants