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

Update config parameters #120

Merged
merged 6 commits into from
Oct 26, 2022

Conversation

sanfern
Copy link
Contributor

@sanfern sanfern commented Sep 30, 2022

Updating sample config file and updating the document link. This l3afd.cfg is an example for prod config deployment.

Signed-off-by: sanfern [email protected]

Signed-off-by: sanfern <[email protected]>
Signed-off-by: sferna1 <[email protected]>
@sanfern sanfern force-pushed the sanfern-dev-config-update branch from 08786ea to 8bf64aa Compare September 30, 2022 18:33
Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

Lets add in the PR notes this is meant to be example prod deployment config options.

Signed-off-by: sferna1 <[email protected]>
Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

I left comments on several of the config options. Also we need to update the configdoc.md for the below items:
L3afConfigsRestAPIAddr - update docs to say this is optional

KFRepoURL - update docs to say this is required
KFPollInterval - update docs to say this is optional
NMetricSamples - update docs to say this is optional

XDPRootProgram* - update docs to say these are optional
TCRootProgram* - update docs to say these are optional

EBPFChainDebugAddr - missing doc for this option
EBPFChainDebugEnabled - missing doc for this option

MTLSEnabled - update docs to say this is optional (default: true)
MTLSCACertFilename - update docs to say this is optional
MTLSServerCertFilename - update docs to say this is optional
MTLSServerKeyFilename - update docs to say this is optional

XDPRootProgramUserProgramDaemon - need to remove from docs
TCRootProgramUserProgramDaemon - need to remove from docs

@sanfern sanfern force-pushed the sanfern-dev-config-update branch from 60fda53 to e40337c Compare October 4, 2022 17:54
@sanfern sanfern force-pushed the sanfern-dev-config-update branch from e40337c to 20d3dfb Compare October 4, 2022 18:08
Comment on lines +56 to +57
|kf-poll-interval|`"30s"`|Periodic interval at which to scrape metrics using Prometheus| No |
|n-metric-samples|`"20"`|Number of Metric Samples| No |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we say that the "example" values are the default values used?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think changing example to default makes sense and instead of giving example on each config option here we can point to the complete example config.

| FieldName | Example | Description | Required |
| ------------- | ------------- | --------------- |----------|
|enabled| `"true"` | Boolean to check mtls enabled or not on REST API exposed by l3afd| No |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
|enabled| `"true"` | Boolean to check mtls enabled or not on REST API exposed by l3afd| No |
|enabled| `"true"` | Boolean controlling whether mTLS is enabled or not on the REST API exposed by l3afd| No |

|enabled| `"true"` | Boolean to check mtls enabled or not on REST API exposed by l3afd| No |
|min-tls-version|`"1.3"`| Minimum tls version allowed| No |
|cert-dir|`"/etc/l3af/certs"`|Absolute path of ca certificates. On Linux this points to a filesystem directory, but on Windows it can point to [certificate store](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/certificate-stores) | No |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
|cert-dir|`"/etc/l3af/certs"`|Absolute path of ca certificates. On Linux this points to a filesystem directory, but on Windows it can point to [certificate store](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/certificate-stores) | No |
|cert-dir|`"/etc/l3af/certs"`|Absolute path of CA certificates. On Linux this points to a filesystem directory, but on Windows it can point to a [certificate store](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/certificate-stores) | No |

Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

We should also update the sections for the kernel version something like:

|kernel-major-version|"4"|Major version of the kernel required to run eBPF programs (Linux Only)| No |
|kernel-minor-version|"15"|Minor version of the kernel required to run eBPF programs (Linux Only)| No |

@@ -1,6 +1,6 @@
# L3AFD Config Options Documentation

See [l3afd.cfg](https://github.com/l3af-project/l3af-arch/blob/main/dev_environment/cfg/l3afd.cfg) for a full example configuration.
See [l3afd.cfg](https://github.com/l3af-project/l3afd/blob/main/config/l3afd.cfg) for a full example configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

lets convert this to relative link since it references file in same repo

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this got missed in your last commit

|restapi-addr|`"localhost:53000"`| Hostname and Port of l3af-configs REST API | Yes |
| FieldName | Default | Description | Required |
| ------------- | ------------- | --------------- |----------|
|restapi-addr|`"localhost:53000"`| Hostname and Port of l3af-configs REST API | No |

# [l3af-config-store]
Copy link
Contributor

Choose a reason for hiding this comment

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

should be heading2 "##" similar to above


# [l3af-config-store]
| FieldName | Example | Description | Required |
| FieldName | Default | Description | Required |
| ------------- | ------------- | --------------- | --------------- |
|filename|`"/etc/l3afd/l3af-config.json"`|Absolute path of persistent config file where we are storing L3afBPFPrograms objects. For more info see [models](https://github.com/l3af-project/l3afd/blob/main/models/l3afd.go)| Yes |

# [mtls]
Copy link
Contributor

Choose a reason for hiding this comment

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

should be heading2 "##" similar to above

| FieldName | Example | Description | Required |
| ------------- | ------------- | --------------- | --------------- |
|url| `"http://localhost:8000/"`|Default repository from which to download eBPF packages| No |
| FieldName | Example | Description | Required |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change heading "Example" to "Default" to match the format below. We should do this for all sections.

Signed-off-by: sferna1 <[email protected]>
@sanfern sanfern force-pushed the sanfern-dev-config-update branch from 9fd4713 to caaf361 Compare October 25, 2022 15:28
Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

LGTM

@sanfern sanfern merged commit b2c9249 into l3af-project:main Oct 26, 2022
sanfern added a commit to sanfern/l3afd that referenced this pull request Mar 6, 2023
* Update config parameters

Signed-off-by: Santhosh Fernandes <[email protected]>
Co-authored-by: Jason Niesz <[email protected]>
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.

5 participants