Added documentation for core peer forwarder by asifsmohammed · Pull Request #1797 · opensearch-project/data-prepper

asifsmohammed · 2022-09-22T00:38:02Z

Signed-off-by: Asif Sohail Mohammed nsifmoh@amazon.com

Description

Adds core peer forwarder documentation.

Issues Resolved

resolves #1772

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dlvenable · 2022-09-22T18:01:48Z

docs/peer_forwarder.md

+
+Presently peer discovery is provided by either a static list or by a DNS record lookup or AWS Cloudmap.
+
+### Static discovery mode


It would be good to elaborate on what this static list is.

For example:

When configured for static discovery mode, each Data Prepper node discovers nodes using a list of IP addresses or domain names.

dlvenable · 2022-09-22T19:21:47Z

docs/peer_forwarder.md

+    mutual_tls:
+```
+
+### <a name="DNS_Discovery"></a>Operating a Cluster with DNS Discovery


It might be good to expand on what this should be. I believe this is typically an A record which is populated with a list of IP addresses.

dlvenable · 2022-09-22T19:22:46Z

docs/peer_forwarder.md

+### <a name="DNS_Discovery"></a>Operating a Cluster with DNS Discovery
+DNS discovery is recommended when scaling out a Data Prepper cluster. The core concept is to configure a DNS provider to return a list of Data Prepper hosts when given a single domain name.
+
+#### With Kubernetes (Recommended)


Why is this the recommended deployment? I'm not quite sure where this recommendation came from or why. Maybe we should just leave out recommendations for now.

This I just copied from the existing peer forwarder plugin documentation. I can remove all 3 recommendations for now.

dlvenable · 2022-09-22T19:23:56Z

docs/peer_forwarder.md

+  discovery_mode: aws_cloud_map
+  awsCloudMapNamespaceName: "my-namespace"
+  awsCloudMapServiceName: "data-prepper-cluster"
+  awsCloudMapQueryParameters:


This parameter is optional. We should be clear about this.

codecov-commenter · 2022-09-23T18:16:49Z

Codecov Report

Merging #1797 (2d2e951) into main (504e8b8) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1797      +/-   ##
============================================
- Coverage     93.96%   93.91%   -0.05%     
  Complexity     1457     1457              
============================================
  Files           188      188              
  Lines          4275     4275              
  Branches        347      347              
============================================
- Hits           4017     4015       -2     
- Misses          177      179       +2     
  Partials         81       81

Impacted Files	Coverage Δ
...rwarder/discovery/AwsCloudMapPeerListProvider.java	`92.59% <0.00%> (-2.47%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

oeyh

Nice doc with lots of examples. Just a few small comments:

oeyh · 2022-09-23T20:18:25Z

docs/peer_forwarder.md

+
+### DNS lookup discovery mode
+DNS discovery is recommended when scaling out a Data Prepper cluster. The core concept is to configure a DNS provider to return a list of Data Prepper hosts when given a single domain name.
+This is a [DNS A Record](https://www.cloudflare.com/learning/dns/dns-records/dns-a-record/) which indicates a list list of IP addresses of a given domain.


Suggested change

This is a [DNS A Record](https://www.cloudflare.com/learning/dns/dns-records/dns-a-record/) which indicates a list list of IP addresses of a given domain.

This is a [DNS A Record](https://www.cloudflare.com/learning/dns/dns-records/dns-a-record/) which indicates a list of IP addresses of a given domain.

oeyh · 2022-09-23T20:23:24Z

docs/peer_forwarder.md

+namespace configured for API instance discovery. You can create a new one following the instructions
+provided by the [CloudMap documentation](https://docs.aws.amazon.com/cloud-map/latest/dg/working-with-namespaces.html).
+
+Your pipeline configuration needs to include:


Should it be in data prepper server configuration instead?

Good catch!

oeyh · 2022-09-23T20:24:50Z

docs/peer_forwarder.md

+* `aws_region` - The AWS region where your namespace exists.
+* `discovery_mode` - Set to `aws_cloud_map`
+
+Your pipeline configuration can optionally include:


Same here, data prepper config?

oeyh · 2022-09-23T20:26:34Z

docs/peer_forwarder.md

+## Configuration
+
+* `port`(Optional): An `int` between 0 and 65535 represents the port peer forwarder server is running on. Default value is `21892`.
+* `request_timeout`(Optional): Duration - A int representing the request timeout in milliseconds for Peer Forwarder HTTP server. Default value is `10000`.


nit:

Suggested change

* `request_timeout`(Optional): Duration - A int representing the request timeout in milliseconds for Peer Forwarder HTTP server. Default value is `10000`.

* `request_timeout`(Optional): Duration - An `int` representing the request timeout in milliseconds for Peer Forwarder HTTP server. Default value is `10000`.

oeyh · 2022-09-23T20:31:52Z

docs/peer_forwarder.md

+* `ssl_insecure_disable_verification`(Optional) : A `boolean` that disables the verification of server's TLS certificate chain. Default value is `false`.
+* `use_acm_certificate_for_ssl`(Optional) : A `boolean` that enables TLS/SSL using certificate and private key from AWS Certificate Manager (ACM). Default is `false`.
+* `acm_certificate_arn`(Optional) : A `String` represents the ACM certificate ARN. ACM certificate take preference over S3 or local file system certificate. Required if `use_acm_certificate_for_ssl` is set to `true`.
+* `acm_certificate_timeout_millis`(Optional) : An 'int' representing the timeout in milliseconds for ACM to get certificates. Default value is `120000`.


nit:

Suggested change

* `acm_certificate_timeout_millis`(Optional) : An 'int' representing the timeout in milliseconds for ACM to get certificates. Default value is `120000`.

* `acm_certificate_timeout_millis`(Optional) : An `int` representing the timeout in milliseconds for ACM to get certificates. Default value is `120000`.

dlvenable

I made a suggestion, but I'm also happy to merge it as it is now.

dlvenable · 2022-09-23T22:22:50Z

docs/peer_forwarder.md

 ```
+
 ### DNS lookup discovery mode
+DNS discovery is recommended when scaling out a Data Prepper cluster. The core concept is to configure a DNS provider to return a list of Data Prepper hosts when given a single domain name.


DNS discovery is recommended when scaling out a Data Prepper cluster.

I suggest the following instead:

We recommend using DNS discovery over static discovery when scaling out a Data Prepper cluster.

We don't recommend DNS discovery exclusively. Cloud Map is a great solution for some AWS customers. But, it is good to recommend the DNS over static list.

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

dlvenable · 2022-09-26T13:33:21Z

docs/peer_forwarder.md

 ### AWS Cloud Map discovery mode

-[AWS CloudMap](https://docs.aws.amazon.com/cloud-map/latest/dg/what-is-cloud-map.html) provides API-based service discovery as well as DNS-based service discovery.
+[AWS Cloud Map](https://docs.aws.amazon.com/cloud-map/latest/dg/what-is-cloud-map.html) provides API-based service discovery as well as DNS-based service discovery.


Good catch! Thanks!

asifsmohammed requested a review from a team as a code owner September 22, 2022 00:38

dlvenable requested changes Sep 22, 2022

View reviewed changes

oeyh reviewed Sep 23, 2022

View reviewed changes

dlvenable previously approved these changes Sep 23, 2022

View reviewed changes

asifsmohammed added 2 commits September 25, 2022 21:56

Added documentation for core peer forwarder

a6f3b25

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

Addressed feedback

50ca8fe

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

asifsmohammed dismissed dlvenable’s stale review via 5ba130c September 26, 2022 02:56

asifsmohammed force-pushed the cpf-documentation branch from a5c9360 to 5ba130c Compare September 26, 2022 02:56

Addressed feedback

2d2e951

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

asifsmohammed force-pushed the cpf-documentation branch from 5ba130c to 2d2e951 Compare September 26, 2022 02:59

dlvenable approved these changes Sep 26, 2022

View reviewed changes

oeyh approved these changes Sep 26, 2022

View reviewed changes

asifsmohammed merged commit 63307d9 into opensearch-project:main Sep 26, 2022


		Presently peer discovery is provided by either a static list or by a DNS record lookup or AWS Cloudmap.

		### Static discovery mode

	This is a [DNS A Record](https://www.cloudflare.com/learning/dns/dns-records/dns-a-record/) which indicates a list list of IP addresses of a given domain.
	This is a [DNS A Record](https://www.cloudflare.com/learning/dns/dns-records/dns-a-record/) which indicates a list of IP addresses of a given domain.

	* `request_timeout`(Optional): Duration - A int representing the request timeout in milliseconds for Peer Forwarder HTTP server. Default value is `10000`.
	* `request_timeout`(Optional): Duration - An `int` representing the request timeout in milliseconds for Peer Forwarder HTTP server. Default value is `10000`.

	* `acm_certificate_timeout_millis`(Optional) : An 'int' representing the timeout in milliseconds for ACM to get certificates. Default value is `120000`.
	* `acm_certificate_timeout_millis`(Optional) : An `int` representing the timeout in milliseconds for ACM to get certificates. Default value is `120000`.

Conversation

asifsmohammed commented Sep 22, 2022

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oeyh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlvenable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Sep 23, 2022 •

edited

Loading