Network Observability FlowCollector API doc updates #64761

skrthomas · 2023-09-14T20:14:43Z

Version(s):

4.11+
Issue:

https://issues.redhat.com/browse/OSDOCS-7660

Link to docs preview:

https://64761--docspreview.netlify.app/openshift-enterprise/latest/networking/network_observability/flowcollector-api
QE review:

QE has approved this change.

Additional information:

Doc generated by netobserv/network-observability-operator#412

ocpdocs-previewbot · 2023-09-14T20:31:02Z

🤖 Updated build preview is available at:
https://64761--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/25355

skrthomas · 2023-09-14T20:42:30Z

@jotak I think the preview looks good. Good to go in my view!
@memodi can you take a look at this for QE approval? I think all these parameters have already been ACK'd in other engineering PRs.

jotak

LGTM!

memodi

overall LGTM just few minor comments/questions

memodi · 2023-09-15T13:45:16Z

modules/network-observability-flowcollector-api-specifications.adoc

 +
- *: the mention of _"unsupported"_, or _"deprecated"_ for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.
+ *: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.


any reason why we mention this multiple times?

This is basically our boilerplate blurb whenever there are unsupported/deprecated items that are listed in the API documentation, so it appears at each one of those. Just because, if we put an asterisk and then refer to a single blurb on the page, the user has to go all the way to the bottom of the page to find the blurb then go back to where they were. So we just mention it at each point that its relevant.

memodi · 2023-09-15T13:51:11Z

modules/network-observability-flowcollector-api-specifications.adoc


 | `kafka`
 | `object`
 | Kafka configuration, such as the address and topic, to send enriched flows to.

 | `type`
 | `string`
-| `type` selects the type of exporters. The available options are `KAFKA` and `IPFIX`. `IPFIX` is _unsupported (*)_.
+| `type` selects the type of exporters. The available options are `KAFKA` and `IPFIX`. `IPFIX` is unsupported (*).


should it be specified as [Unsupported (*)]. it's inconsistent.

Really good catch here @memodi because I think IPFIX is supported in 1.4 actually. I have this doc Jira + PR: https://issues.redhat.com/browse/OSDOCS-6348

@jotak is IPFIX supported for 1.4 release time or is it going out in 4.14 time?

Via slack, we discussed IPFIX is supported so this part of the API documentation will need to be updated cc @jotak

oops, good catch, I'll update the upstream doc

=> netobserv/network-observability-operator#426

modules/network-observability-flowcollector-api-specifications.adoc

memodi · 2023-09-19T19:28:42Z

/label qe-approved

stevsmit

@skrthomas I think overall this looks really good. API docs are inherently weird, in that the normal conventions of OCP guidelines, imo, kind of go out the window; I just think it's difficult to provide a thorough review using those guidelines.

That said, broadly, I would take a look at the rendering on the portal, and specifically at Descriptions, and consider how some entries, like Sampling, are not in backticks in the description (also, you have two entries for Sampling, and have them formatted in two different ways. I'd choose the sampling field under ".spec.agent.ipfix" personally). I see why you did it like this, and I think it's fine, but I also could see it working as something like what follows:

sampling sets the rate of the flow reporter.

Namespace is another field that doens't follow the same pattern. Just fyi. Enable is another one. That said, I think it's fine as-is, and hard to follow the same conventions for some field; I'm just pointing this out.

Another thing to consider might be putting literal values in backticks, as per the OCP Guidelines, for example, 9093. I believe that this falls under the category of literal value. Would things like GOGC and GOMAXPROCS also be values for the env field? Any "value" that can be set under a parameter, that is mention in the Descriptions field, probably needs set in backticks (e.g., port has a few)

Overall, good work. These are a log to get through.

stevsmit · 2023-09-20T13:25:33Z

modules/network-observability-flowcollector-api-specifications.adoc

@@ -25,21 +23,21 @@ Type::

 | `apiVersion`
 | `string`
-| APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and might reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
+| APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources


Curious why you swapped might/may here? https://www.ibm.com/docs/en/ibm-style?topic=word-usage#m

THough since they're API docs, part of me wonders if you're copying directly from the source code...

This is a direct copy of asciidoc generated from the source code...I'm not sure why these got changed.

Maybe they were holdovers I noticed last round of API updates and I manually changed them but didn't tell Joel to change them on his end.

This field, "APIVersion", comes from the upstream and we don't have control over it (it's upstream kubernetes)

stevsmit · 2023-09-20T13:25:38Z

modules/network-observability-flowcollector-api-specifications.adoc


 | `kind`
 | `string`
-| Kind is a string value representing the REST resource this object represents. Servers might infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds


that's also upstream k8s

stevsmit · 2023-09-20T13:26:44Z

modules/network-observability-flowcollector-api-specifications.adoc


 | `kind`
 | `string`
-| Kind is a string value representing the REST resource this object represents. Servers might infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
+| Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds


consider may/might swap

Suggested change

| Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds

| Kind is a string value representing the REST resource that this object represents. Servers may infer this from the endpoint that the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds

that's also upstream k8s

stevsmit · 2023-09-20T13:28:32Z

modules/network-observability-flowcollector-api-specifications.adoc

 +
- *: the mention of _"unsupported"_, or _"deprecated"_ for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.
+ *: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.


It's kind of weird making these suggestions because from what I can tell based on some of the comments, these are already-in-use blurbs that you probably won't want to alter.

Also, can you make sure that this is formatted correctly on the portal? It currently renders as an asterisk.

Suggested change

*: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.

*: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. For example, it might have been contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.

This full statement only appears in the spec table and the spec field description...The asterisks is used throughout but the text itself is only at the beginning. When I commented to Mehul earlier, I was thinking that this whole statement was repeated, but its only "deprecated" or "unsupported" that appear beside the "*". I think the asterisk is fine, and the full-text in the spec makes sense to me.

On the currently live customer portal, I see the full text with asterisks and that looks okay to me:

The portal chapters do kinda make this look weird.

stevsmit · 2023-09-20T13:39:06Z

modules/network-observability-flowcollector-api-specifications.adoc

@@ -179,27 +172,35 @@ Type::

 | `excludeInterfaces`
 | `array (string)`
-| `excludeInterfaces` contains the interface names that will be excluded from flow tracing. An entry is enclosed by slashes, such as `/br-/`, is matched as a regular expression. Otherwise it is matched as a case-sensitive string.
+| `excludeInterfaces` contains the interface names that are excluded from flow tracing. An entry is enclosed by slashes, such as `/br-/`, is matched as a regular expression. Otherwise it is matched as a case-sensitive string.


Suggested change

| `excludeInterfaces` contains the interface names that are excluded from flow tracing. An entry is enclosed by slashes, such as `/br-/`, is matched as a regular expression. Otherwise it is matched as a case-sensitive string.

| `excludeInterfaces` contains the interface names that are excluded from flow tracing. An entry is enclosed by slashes, such as `/br-/`, and is matched as a regular expression. Otherwise it is matched as a case-sensitive string.

I also don't think the comma is needed before this "and". It should be ".../br-/ and is matched as a reg..."

In fact, the first "is" shouldn't be there and I guess that's misleading you for getting the meaning here. It should be like:

An entry enclosed by slashes, such as /br-/, is matched as a regular expression.

Side-question: any preference between "enclosed by", "enclosed in" or "enclosed within" ?

@jotak I think "enclosed by" is good.

stevsmit · 2023-09-20T14:16:46Z

modules/network-observability-flowcollector-api-specifications.adoc


 | `topic`
 | `string`
-| Kafka topic to use. It must exist, NetObserv will not create it.
+| Kafka topic to use. It must exist, Network Observability does not create it.


Suggested change

| Kafka topic to use. It must exist, Network Observability does not create it.

| Kafka topic to use. It must exist. Network Observability does not create it.

stevsmit · 2023-09-20T15:28:26Z

modules/network-observability-flowcollector-api-specifications.adoc

@@ -914,6 +1123,10 @@ Type::
 | `string`
 | `batchWait` is the maximum time to wait before sending a batch.

+| `enable`
+| `boolean`
+| enable storing flows to Loki. It is required for the {product-title} Console plugin installation.


Maybe like...

It's also the only one in this set that isn't in backticks, so I was trying to think of a way to set it in backticks.

Suggested change

| enable storing flows to Loki. It is required for the {product-title} Console plugin installation.

| Set to `enable` to store flows to Loki. It is required for the {product-title} Console plugin installation.

Good suggestion.

In that case, it should be perhaps:

Set "true" to store flows ....
?

(It can be set to true or false )

@jotak Can we say

Set enable to true to store flows in Loki.

@skrthomas sounds good to me, i'm also updating upstream

stevsmit · 2023-09-20T15:31:35Z

modules/network-observability-flowcollector-api-specifications.adoc

@@ -1385,10 +1602,18 @@ Type::
 |===
 | Property | Type | Description

+| `insecureSkipVerify`
+| `boolean`
+| insecureSkipVerify allows skipping client-side verification of the provided certificate. If set to true, the `providedCaFile` field is ignored.


Suggested change

| insecureSkipVerify allows skipping client-side verification of the provided certificate. If set to true, the `providedCaFile` field is ignored.

| `insecureSkipVerify` allows skipping client-side verification of the provided certificate. If set to true, the `providedCaFile` field is ignored.

stevsmit · 2023-09-20T15:43:09Z

modules/network-observability-flowcollector-api-specifications.adoc

 +
- *: the mention of _"unsupported"_, or _"deprecated"_ for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.
+ *: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.


Can you make sure that this renders on the portal correctly? It currently renders as an asterisk.

If you apply the above suggestion, you might want to change it here:

*: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. For example, it might have been contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.

I responded to the earlier comment with a screencapture from the live customer portal, where I see this entire statement with the asterisks and it looks OK to me. I can swap "for instance" for "for example" in both cases of this deprecation stmt.

stevsmit · 2023-09-20T16:06:31Z

modules/network-observability-flowcollector-api-specifications.adoc

+
+| `type`
+| `string`
+| Type for the file reference: "configmap" or "secret"


Would these strings be in backticks since they're values?

Strings are in quotes I think, https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#yaml-formatting-for-kubernetes-and-openshift-api-objects

IN the YAML example, I see "" around string values. It isn't actually clearly defined there.

Unless I'm misinterpreting this.

skrthomas · 2023-09-20T21:21:28Z

Thanks for the review @stevsmit !!

Sampling - I agree, in spec.agent.ebpf.samling, in that description, I think we can say "sampling sets the rate of the flow reporter" cc @jotak
I'm unsure about putting GOGC and GOMAXPROCS in backticks...Its only used in the NetObserv API reference, so I don't see any other examples of the treatment in other areas of docs.openshift. I put 9093 in backticks though.
I changed enable as you suggested in your comment. There are a lot of namespaces...yeah. Its the main inconsistent one. At least its consistently inconsistent? 😅 I'm ok leaving it...we could say something like "namespace designates where the config map, etc"

skrthomas · 2023-09-28T19:40:30Z

/cherrypick enterprise-4.11

skrthomas · 2023-09-28T19:40:37Z

/cherrypick enterprise-4.12

skrthomas · 2023-09-28T19:40:43Z

/cherrypick enterprise-4.13

skrthomas · 2023-09-28T19:40:49Z

/cherrypick enterprise-4.14

openshift-cherrypick-robot · 2023-09-28T19:41:18Z

@skrthomas: new pull request created: #65458

In response to this:

/cherrypick enterprise-4.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2023-09-28T19:41:25Z

@skrthomas: new pull request created: #65459

In response to this:

/cherrypick enterprise-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2023-09-28T19:41:33Z

@skrthomas: new pull request created: #65460

In response to this:

/cherrypick enterprise-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2023-09-28T19:41:37Z

@skrthomas: new pull request created: #65461

In response to this:

/cherrypick enterprise-4.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 14, 2023

skrthomas requested a review from jotak September 14, 2023 20:39

skrthomas requested a review from memodi September 14, 2023 20:42

jotak approved these changes Sep 15, 2023

View reviewed changes

memodi suggested changes Sep 15, 2023

View reviewed changes

skrthomas force-pushed the API-updates branch from ebb7499 to 19d3943 Compare September 19, 2023 13:45

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2023

skrthomas mentioned this pull request Sep 19, 2023

Remove unsupported mention on IPFIX doc netobserv/network-observability-operator#426

Merged

10 tasks

skrthomas force-pushed the API-updates branch from 19d3943 to 91e3cdf Compare September 19, 2023 14:08

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 19, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 19, 2023

skrthomas added branch/enterprise-4.11 branch/enterprise-4.12 branch/enterprise-4.13 branch/enterprise-4.14 peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 19, 2023

stevsmit added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 19, 2023

stevsmit reviewed Sep 20, 2023

View reviewed changes

stevsmit added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Sep 20, 2023

stevsmit added this to the Continuous Release milestone Sep 20, 2023

Network Observability FlowCollector API doc updates

7a61268

skrthomas force-pushed the API-updates branch from 91e3cdf to 7a61268 Compare September 20, 2023 21:11

skrthomas merged commit 9d49c3e into openshift:main Sep 28, 2023
1 check passed

This was referenced Sep 28, 2023

[enterprise-4.11] Network Observability FlowCollector API doc updates #65458

Merged

[enterprise-4.12] Network Observability FlowCollector API doc updates #65459

Merged

openshift-cherrypick-robot mentioned this pull request Sep 28, 2023

[enterprise-4.13] Network Observability FlowCollector API doc updates #65460

Merged

openshift-cherrypick-robot mentioned this pull request Sep 28, 2023

[enterprise-4.14] Network Observability FlowCollector API doc updates #65461

Merged

	\| Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
	\| Kind is a string value representing the REST resource that this object represents. Servers may infer this from the endpoint that the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds

	*: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. It might have been, for instance, contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.
	*: the mention of "unsupported", or "deprecated" for a feature throughout this document means that this feature is not officially supported by Red Hat. For example, it might have been contributed by the community and accepted without a formal agreement for maintenance. The product maintainers might provide some support for these features as a best effort only.

	\| `excludeInterfaces` contains the interface names that are excluded from flow tracing. An entry is enclosed by slashes, such as `/br-/`, is matched as a regular expression. Otherwise it is matched as a case-sensitive string.
	\| `excludeInterfaces` contains the interface names that are excluded from flow tracing. An entry is enclosed by slashes, such as `/br-/`, and is matched as a regular expression. Otherwise it is matched as a case-sensitive string.

	\| Kafka topic to use. It must exist, Network Observability does not create it.
	\| Kafka topic to use. It must exist. Network Observability does not create it.

	\| enable storing flows to Loki. It is required for the {product-title} Console plugin installation.
	\| Set to `enable` to store flows to Loki. It is required for the {product-title} Console plugin installation.

	\| insecureSkipVerify allows skipping client-side verification of the provided certificate. If set to true, the `providedCaFile` field is ignored.
	\| `insecureSkipVerify` allows skipping client-side verification of the provided certificate. If set to true, the `providedCaFile` field is ignored.

Network Observability FlowCollector API doc updates #64761

Network Observability FlowCollector API doc updates #64761

Conversation

skrthomas commented Sep 14, 2023 • edited Loading

ocpdocs-previewbot commented Sep 14, 2023 • edited Loading

skrthomas commented Sep 14, 2023

jotak left a comment

Choose a reason for hiding this comment

memodi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrthomas Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memodi commented Sep 19, 2023

stevsmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrthomas Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrthomas commented Sep 20, 2023

skrthomas commented Sep 28, 2023

skrthomas commented Sep 28, 2023

skrthomas commented Sep 28, 2023

skrthomas commented Sep 28, 2023

openshift-cherrypick-robot commented Sep 28, 2023

openshift-cherrypick-robot commented Sep 28, 2023

openshift-cherrypick-robot commented Sep 28, 2023

openshift-cherrypick-robot commented Sep 28, 2023

skrthomas commented Sep 14, 2023 •

edited

Loading

ocpdocs-previewbot commented Sep 14, 2023 •

edited

Loading

skrthomas Sep 15, 2023 •

edited

Loading

skrthomas Sep 20, 2023 •

edited

Loading