CRD description includes comments not meant to be included

[tsaarni]

There is a convention in Kubernetes to use --- separator in comment of a type to signify that the rest of the comment is not part of the description.

See example here

// Condition contains details for one aspect of the current state of this API Resource.
// ---
// This struct is intended for direct use as an array at the field path .status.conditions.  For example,
//
//	type FooStatus struct{
//	    // Represents the observations of a foo's current state.
//	    // Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
//	    // +patchMergeKey=type
//	    // +patchStrategy=merge
//	    // +listType=map
//	    // +listMapKey=type
//	    Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
//
//	    // other fields
//	}
type Condition struct {
...

When the OpenAPI docs are generated for Kubernetes internal API resources, I believe the comment part is stripped by this code. When using e.g. kubectl explain PodDisruptionBudget.status.conditions to read the documentation for the above type, or the generated web page for Condition, it includes only "Condition contains details for one aspect of the current state of this API Resource" and not the example code that comes after ---.

However, when that same type is used as part of CRD the part after --- is included in description, see example here.

As CRDs are installed at runtime, it will not go through Kubernetes swagger generation pipeline that would strip the comments. It would be great if controller-tools would remove the comment part from description in the same way as Kubernetes itself does. Google search for https://www.google.com/search?q=%22Represents+the+observations+of+a+foo%27s+current+state%22 shows that the problem is wide spread.

[tsaarni]

I did very quick test and something like this could fix the problem

diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go
index e76d3ea8..c1a758e4 100644
--- a/pkg/crd/schema.go
+++ b/pkg/crd/schema.go
@@ -185,13 +185,18 @@ func typeToSchema(ctx *schemaContext, rawType ast.Expr) *apiext.JSONSchemaProps
                return &apiext.JSONSchemaProps{}
        }
 
-       props.Description = ctx.info.Doc
+       props.Description = stripComments(ctx.info.Doc)
 
        applyMarkers(ctx, ctx.info.Markers, props, rawType)
 
        return props
 }
 
+func stripComments(rawDoc string) string {
+       // Ignore all lines after ---
+       return strings.Split(rawDoc, "---")[0]
+}
+
 // qualifiedName constructs a JSONSchema-safe qualified name for a type
 // (`<typeName>` or `<safePkgPath>~0<typeName>`, where `<safePkgPath>`
 // is the package path with `/` replaced by `~1`, according to JSONPointer
@@ -400,7 +405,7 @@ func structToSchema(ctx *schemaContext, structType *ast.StructType) *apiext.JSON
                } else {
                        propSchema = typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), field.RawField.Type)
                }
-               propSchema.Description = field.Doc
+               propSchema.Description = stripComments(field.Doc)
 
                applyMarkers(ctx, field.Markers, propSchema, field.RawField)

If you agree with this as an overall approach, I can create PR!

[JoelSpeed]

I've also seen this issue, as a workaround, you can prefix lines with a +

// This is my godoc
// + ---
// + I don't want this to be in the generated docs

However, in one or the other of the CRD vs openAPI, you have a trailing marker, either + or --- depending on if you + --- or straight up --- on that line

A proper solution in keeping with other tooling would be great IMO, though, we might have to consider the effect on existing CRDs, is this a breaking change?

[tsaarni]

Thanks for the comment @JoelSpeed!

is this a breaking change?

I'd be interested in opinions as well. Just pondering the question myself:

Maybe the strongest argument why it is not breaking is that from API compatibility viewpoint changing the documentation would be OK - there is no mention of it in custom-resource-definition-versioning.md or api_changes.md

I'm not sure if it has been stated what the formatting for type descriptions is? For example, should they be considered just as pure godoc? And btw: godoc does not treat --- as comment marker and strip the rest of the text, I tested that.

Kubectl has not (so far) supported formatting of descriptions itself so --- in existing documentation makes no sense from that point. But because CRD uses OpenAPI then OpenAPI spec could come into play, and it mentions

Rich Text Formatting

Throughout the specification description fields are noted as supporting CommonMark markdown formatting. Where OpenAPI tooling renders rich text it MUST support, at a minimum, markdown syntax as described by CommonMark 0.27. Tooling MAY choose to ignore some CommonMark features to address security concerns.

When rendering CRD descriptions according to the above, one would interpret --- as horizontal rule, which to me sounds quite unlikely "layout" choice inside API field description. If impacted one would obviously still remain compatible but if I'd loose the rest of my description, I would likely be a little bit annoyed.

[cbandy]

Related: #649, #728

[tsaarni]

I bumped into #870 which seems to imply that --- is sometimes used as part of docs, when markdown and embedded yaml is included in the documentation. In that case, it would be difficult to enforce Kubernetes conventions and treat that as comment marker for removing the rest of the godoc string.

We could consider godoc as markdown and allow literal --- only within code blocks. However, #870 seems to use --- also outside code block.

Cc: @qinqon

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale