LOG-4990: Initial impl of CLF.obs API #2451

jcantrill · 2024-04-27T01:52:00Z

Description

This PR:

Is an initial implementation of the proposed ClusterLogForwarder.observability.io/v1 API

Links

https://issues.redhat.com/browse/LOG-4990

cc @alanconway @periklis @xperimental @cahartma @Clee2691 @vparfonov

/hold

openshift-ci-robot · 2024-04-27T01:52:04Z

@jcantrill: This pull request references LOG-4990 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

In response to this:

Description

This PR:

Is an initial implementation of the proposed ClusterLogForwarder.observability.io/v1 API

Links

https://issues.redhat.com/browse/LOG-4990

cc @alanconway @periklis @xperimental @cahartma @Clee2691 @vparfonov

/hold

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 openshift-eng/jira-lifecycle-plugin repository.

jcantrill · 2024-04-27T01:52:16Z

/hold

openshift-ci · 2024-04-27T01:52:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

periklis

LGTM to me. Left some comments for impovement. Key question remains if Namespaced is wanted for this resource. If yes, see my ask on the enhancement proposal to explain when it is used and what for.

api/observability/v1/clusterlogforwarder_types.go

api/observability/v1/output_types.go

periklis · 2024-04-29T18:51:03Z

config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml

+    listKind: ClusterLogForwarderList
+    plural: clusterlogforwarders
+    singular: clusterlogforwarder
+  scope: Namespaced


Double checking here. Is Namespaced intended?

Yes. Maps to mCLF concept. Added reference in enhancement based upon comment

periklis

Looks overall good to me. A couple of general minor enhancements:

Every field being optional and marked with //+optional should be marked with // +kubebuilder:validation:Optional because this is the future-proof marker. I don't know if and when the others will be dropped, but let's add the new ones too.
Same for // +required, we should use also // +kubebuilder:validation:Required
Do we need to add //+nullable for optional fields at all or is this a legacy thing?
The kubebuilder:validation:minLength:=1 marker is used on many required fields. Is this needed if the field is properly marked as required? Maybe we were simply missing // +kubebuilder:validation:Required in the old logging CLF types?

api/observability/v1/clusterlogforwarder_types.go

periklis · 2024-05-02T08:51:59Z

api/observability/v1/filter_api_audit_types.go

+	Rules []auditv1.PolicyRule `json:"rules,omitempty"`
+
+	// OmitStages is a list of stages for which no events are created.
+	// Note that this can also be specified per rule in which case the union of both are omitted.
+	// +optional
+	OmitStages []auditv1.Stage `json:"omitStages,omitempty"`


Any possibility to add //+listType and //+listMapKey here?

Skipping for now since I don't know how this applies. I don't believe there is a place for "unique" entry in the list

api/observability/v1/input_types.go

periklis · 2024-05-02T08:54:14Z

api/observability/v1/input_types.go

+	// +optional
+	Includes []NamespaceContainerSpec `json:"includes,omitempty"`
+
+	// Excludes is the set of namespaces and containers to ignore when collecting logs.
+	// Takes precedence over Includes option.
+	//
+	// +optional
+	Excludes []NamespaceContainerSpec `json:"excludes,omitempty"`


Possibility to add // +listType:=map and // +listMapKey:=namespace here. WDYT?

I don't think this applies for these since they can have either namespace or container and neither is really an identifying key

periklis · 2024-05-02T08:58:00Z

api/observability/v1/input_types.go

+// Infrastructure enables infrastructure logs.
+// Sources of these logs:
+// * container workloads deployed to namespaces: default, kube*, openshift*
+// * journald logs from cluster nodes
+type Infrastructure struct {
+
+	// Sources defines the list of infrastructure sources to collect.
+	// This field is optional and omission results in the collection of all infrastructure sources.
+	//  Valid sources are: node, container
+	//
+	// +optional
+	Sources []string `json:"sources,omitempty"`
+}
+
+const (
+
+	// InfrastructureSourceNode are journald logs from the node
+	InfrastructureSourceNode string = "node"
+
+	// InfrastructureSourceContainer are container logs from workloads deployed
+	// in any of the following namespaces: default, kube*, openshift*
+	InfrastructureSourceContainer string = "container"
+)
+
+var InfrastructureSources = sets.NewString(InfrastructureSourceNode, InfrastructureSourceContainer)


Suggested change

// Infrastructure enables infrastructure logs.

// Sources of these logs:

// * container workloads deployed to namespaces: default, kube*, openshift*

// * journald logs from cluster nodes

type Infrastructure struct {

// Sources defines the list of infrastructure sources to collect.

// This field is optional and omission results in the collection of all infrastructure sources.

// Valid sources are: node, container

//

// +optional

Sources []string `json:"sources,omitempty"`

}

const (

// InfrastructureSourceNode are journald logs from the node

InfrastructureSourceNode string = "node"

// InfrastructureSourceContainer are container logs from workloads deployed

// in any of the following namespaces: default, kube*, openshift*

InfrastructureSourceContainer string = "container"

)

var InfrastructureSources = sets.NewString(InfrastructureSourceNode, InfrastructureSourceContainer)

// Infrastructure enables infrastructure logs.

// Sources of these logs:

// * container workloads deployed to namespaces: default, kube*, openshift*

// * journald logs from cluster nodes

type Infrastructure struct {

// Sources defines the list of infrastructure sources to collect.

// This field is optional and omission results in the collection of all infrastructure sources.

// Valid sources are: node, container

//

// +optional

// +kubebuilder:validation:UniqueItems:=true

Sources []InfrastructureSourceType `json:"sources,omitempty"`

}

type InfrastructureSourceType string

const (

// InfrastructureSourceNode are journald logs from the node

InfrastructureSourceNode InfrastructureSourceType = "node"

// InfrastructureSourceContainer are container logs from workloads deployed

// in any of the following namespaces: default, kube*, openshift*

InfrastructureSourceContainer InfrastructureSourceType = "container"

)

var InfrastructureSources = sets.NewString(InfrastructureSourceNode, InfrastructureSourceContainer)

api/observability/v1/input_types.go

periklis · 2024-05-02T09:00:11Z

api/observability/v1/input_types.go

+const (
+	ReceiverTypeHttp   = "http"
+	ReceiverTypeSyslog = "syslog"
+
+	// InputReceiverFormatKubeAPIAudit Log events in k8s list format, e.g. API audit log events.
+	InputReceiverFormatKubeAPIAudit = "kubeAPIAudit"
+)
+
+var ReservedInputReceiverNames = sets.NewString(ReceiverTypeHttp, ReceiverTypeSyslog)
+
+// ReceiverSpec is a union of input Receiver types.
+type ReceiverSpec struct {
+
+	// Type of Receiver plugin.
+	// +kubebuilder:validation:Enum:=http;syslog
+	// +required
+	Type string `json:"type,omitempty"`


Suggested change

const (

ReceiverTypeHttp = "http"

ReceiverTypeSyslog = "syslog"

// InputReceiverFormatKubeAPIAudit Log events in k8s list format, e.g. API audit log events.

InputReceiverFormatKubeAPIAudit = "kubeAPIAudit"

)

var ReservedInputReceiverNames = sets.NewString(ReceiverTypeHttp, ReceiverTypeSyslog)

// ReceiverSpec is a union of input Receiver types.

type ReceiverSpec struct {

// Type of Receiver plugin.

// +kubebuilder:validation:Enum:=http;syslog

// +required

Type string `json:"type,omitempty"`

type ReceiverType string

const (

ReceiverTypeHttp ReceiverType = "http"

ReceiverTypeSyslog ReceiverType = "syslog"

// InputReceiverFormatKubeAPIAudit Log events in k8s list format, e.g. API audit log events.

InputReceiverFormatKubeAPIAudit string = "kubeAPIAudit"

)

var ReservedInputReceiverNames = sets.NewString(ReceiverTypeHttp, ReceiverTypeSyslog)

// ReceiverSpec is a union of input Receiver types.

type ReceiverSpec struct {

// Type of Receiver plugin.

// +kubebuilder:validation:Enum:=http;syslog

// +required

Type ReceiverType `json:"type,omitempty"`

xperimental

I noticed that the list of comments I'd have to add would be quite long and it would essentially duplicate the work when you then need to apply them. So I made a branch with the changes instead.

There's always a "change commit" which describes what I've changed and a subsequent "update bundle" one that just does the codegeneration, so there's essentially 2x the commits it would need.

.bingo/operator-sdk.mod

xperimental · 2024-05-02T16:17:40Z

api/observability/v1/clusterlogforwarder_types.go

+type ClusterLogForwarderStatus struct {
+	// Conditions of the log forwarder.
+	//+operator-sdk:csv:customresourcedefinitions:type=status,displayName="Forwarder Conditions",xDescriptors={"urn:alm:descriptor:io.kubernetes.conditions"}
+	Conditions status.Conditions `json:"conditions,omitempty"`


status is an internal package, it should not be used in the public API. I propose we reuse the Condition struct in the Kubernetes API (k8s.io/apimachinery/pkg/apis/meta/v1).

api/observability/v1/clusterlogforwarder_types.go

xperimental · 2024-05-02T16:34:42Z

api/observability/v1/clusterlogforwarder_types.go

+// +kubebuilder:object:root=true
+// ClusterLogForwarderList contains a list of ClusterLogForwarder


(see above comment for longer explanation)

Alternative 1:

Suggested change

// +kubebuilder:object:root=true

// ClusterLogForwarderList contains a list of ClusterLogForwarder

// ClusterLogForwarderList contains a list of ClusterLogForwarder

//

// +kubebuilder:object:root=true

Alternative 2:

Suggested change

// +kubebuilder:object:root=true

// ClusterLogForwarderList contains a list of ClusterLogForwarder

//+kubebuilder:object:root=true

// ClusterLogForwarderList contains a list of ClusterLogForwarder

api/observability/v1/conditions.go

jcantrill · 2024-05-03T18:28:55Z

Do we need to add //+nullable for optional fields at all or is this a legacy thing?

Can not speak to this. I copied and followed the pattern there so likely a legacy thing

The kubebuilder:validation:minLength:=1 marker is used on many required fields. Is this needed if the field is properly marked as required? Maybe we were simply missing // +kubebuilder:validation:Required in the old logging CLF types?

Probably a legacy thing as well

.bingo/operator-sdk.mod

xperimental · 2024-05-06T12:06:26Z

api/observability/v1/clusterlogforwarder_types.go

+	// +kubebuilder:validation:Required
+	// +listType:=map
+	// +listMapKey:=name
+	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Receiver Outputs"


Noticed the inputs description was changed to just "Log Inputs", why have "Receiver" here and not just "Log Outputs"?

Suggested change

// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Receiver Outputs"

// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Log Outputs"

I think I'm going to update all of them to say "Log Forwarder" since Log seems not descriptive enough

xperimental · 2024-05-06T12:07:01Z

api/observability/v1/clusterlogforwarder_types.go

+	// +kubebuilder:validation:Optional
+	// +listType:=map
+	// +listMapKey:=name
+	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Forwarder Pipeline Filters"


Similarly here: Why use "Forwarder"?

Suggested change

// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Forwarder Pipeline Filters"

// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Log Pipeline Filters"

xperimental · 2024-05-06T12:07:26Z

api/observability/v1/clusterlogforwarder_types.go

+	// +kubebuilder:validation:Required
+	// +listType:=map
+	// +listMapKey:=name
+	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Forwarder Pipelines"


Simplify to "Log Pipelines"?

Suggested change

// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Forwarder Pipelines"

// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Log Pipelines"

api/observability/v1/clusterlogforwarder_types.go

api/observability/v1/filter_types.go

api/observability/v1/output_types.go

jcantrill · 2024-05-06T15:44:35Z

/hold cancel

xperimental · 2024-05-07T09:54:32Z

Looks good to me 👍

jcantrill · 2024-05-07T15:10:39Z

/override ci/prow/e2e-target

openshift-ci · 2024-05-07T15:11:12Z

@jcantrill: Overrode contexts on behalf of jcantrill: ci/prow/e2e-target

In response to this:

/override ci/prow/e2e-target

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.

xperimental · 2024-05-07T15:22:18Z

/lgtm

openshift-ci · 2024-05-07T15:44:44Z

@jcantrill: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-target	`98d6fc7`	link	true	`/test e2e-target`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

JoelSpeed

Tried to leave some general feedback on API conventions and validations you could look into, have tried not to repeat myself but quite a few of the things I mention you may want to comb through for repeats of, but hopefully this gives you some food for thought on improving the API validations without changing the shape too much, let me know if you want any more details on the suggestions

JoelSpeed · 2024-05-28T16:20:52Z

api/observability/v1/input_types.go

+
+// InputType specifies the type of log input to create.
+//
+// +kubebuilder:validation:Enum:=audit;application;infrastructure;receiver


Enums should be PascalCase to fit kube conventions

JoelSpeed · 2024-05-28T16:22:20Z

api/observability/v1/input_types.go

+	// Application, named set of `application` logs that
+	// can specify a set of match criteria
+	//
+	// +kubebuilder:validation:Optional
+	// +nullable
+	Application *Application `json:"application,omitempty"`
+
+	// Infrastructure, Enables `infrastructure` logs.
+	//
+	// +kubebuilder:validation:Optional
+	Infrastructure *Infrastructure `json:"infrastructure,omitempty"`
+
+	// Audit, enables `audit` logs.
+	//
+	// +kubebuilder:validation:Optional
+	Audit *Audit `json:"audit,omitempty"`
+
+	// Receiver to receive logs from non-cluster sources.
+	//
+	// +kubebuilder:validation:Optional
+	Receiver *ReceiverSpec `json:"receiver,omitempty"`


You could use CEL to prevent the wrong union member from being added when the Type specifies a specific member, eg https://github.com/openshift/api/blob/ba11c1587003dc84cb014fd8db3fa597a3faaa63/example/v1/types_stable.go#L108-L125

JoelSpeed · 2024-05-28T16:22:52Z

api/observability/v1/input_types.go

+
+// InputSpec defines a selector of log messages for a given log type.
+type InputSpec struct {
+	// Name used to refer to the input of a `pipeline`.


Kube conventions suggest using the json tag name for the start of the comment, since users are used to seeing serialized versions of fields, not the Go versions

JoelSpeed · 2024-05-28T16:23:37Z

api/observability/v1/input_types.go

+	// Note: infrastructure namespaces are still excluded for "*" values unless a qualifying glob pattern is specified.
+	//
+	// +kubebuilder:validation:Optional
+	Includes []NamespaceContainerSpec `json:"includes,omitempty"`


Should add listType to this, either atomic or map, and if map, add listMapKey for the key field within the struct

JoelSpeed · 2024-05-28T16:24:04Z

api/observability/v1/input_types.go

+	// Supports glob patterns and presumes "*" if omitted.
+	//
+	// +kubebuilder:validation:Optional
+	Namespace string `json:"namespace,omitempty"`


Generally good to add length limits and specific character sets to strings where possible

JoelSpeed · 2024-05-28T16:28:08Z

api/observability/v1/output_types.go

+	// See the `secret` field for more details.
+	//
+	// +kubebuilder:validation:Required
+	URL string `json:"url"`


You should add a length limit to this, and use CEL to validate that it is a valid URL

JoelSpeed · 2024-05-28T16:28:30Z

api/observability/v1/output_types.go

+
+// BaseOutputTuningSpec tuning parameters for an output
+type BaseOutputTuningSpec struct {
+	Delivery DeliveryMode `json:"delivery,omitempty"`


Missing godoc, also not marked as optional or required

JoelSpeed · 2024-05-28T16:28:53Z

api/observability/v1/output_types.go

+	// MaxWrite limits the maximum payload in terms of bytes of a single "send" to the output.
+	//
+	// +kubebuilder:validation:Optional
+	MaxWrite *resource.Quantity `json:"maxWrite,omitempty"`


CEL can now validate quantities as well

JoelSpeed · 2024-05-28T16:32:37Z

api/observability/v1/output_types.go

+	// Headers specify optional headers to be sent with the request
+	//
+	// +kubebuilder:validation:Optional
+	Headers map[string]string `json:"headers,omitempty"`


Generally it's best to avoid maps in CRDs, they make it hard to make structural schemas. Create instead a list with a struct type with a key and value

headers: - name: ... values: []

JoelSpeed · 2024-05-28T16:33:46Z

api/observability/v1/output_types.go

+	//     Emergency Alert Critical Error Warning Notice Informational Debug
+	//
+	// +kubebuilder:validation:Optional
+	// TODO: Add regex validation


Regex, CEL, enum? CEL in general can provide more flexibility

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 27, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2024

jcantrill added release/6.0 and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 27, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2024

openshift-ci bot requested review from Clee2691 and vparfonov April 27, 2024 01:52

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2024

jcantrill mentioned this pull request Apr 27, 2024

First cut of v2 API, still some work to do: see doc.go for notes. #2327

Closed

periklis reviewed Apr 29, 2024

View reviewed changes

jcantrill force-pushed the log4990 branch from 1270c18 to 1076cf7 Compare April 30, 2024 15:32

openshift-ci bot added the midstream/Dockerfile A Dockerfile.in sync is needed with midstream label Apr 30, 2024

jcantrill force-pushed the log4990 branch from aef6200 to 2da3e08 Compare April 30, 2024 18:24

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024

jcantrill force-pushed the log4990 branch from 2da3e08 to cdc70e1 Compare May 1, 2024 13:59

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2024

periklis reviewed May 2, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024

xperimental reviewed May 3, 2024

View reviewed changes

jcantrill force-pushed the log4990 branch from cdc70e1 to 50cc2a5 Compare May 3, 2024 19:25

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2024

jcantrill force-pushed the log4990 branch from 50cc2a5 to bb1a2f5 Compare May 5, 2024 19:41

xperimental reviewed May 6, 2024

View reviewed changes

LOG-4990: Initial impl of CLF.observability API

98d6fc7

jcantrill force-pushed the log4990 branch from bb1a2f5 to 98d6fc7 Compare May 6, 2024 15:44

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2024

openshift-ci bot assigned xperimental May 7, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2024

openshift-merge-bot bot merged commit 0b4f2e6 into openshift:master May 7, 2024
7 checks passed

jcantrill deleted the log4990 branch May 7, 2024 15:48

jcantrill mentioned this pull request May 14, 2024

LOG-4928: Cluster logging next APIs openshift/enhancements#1537

Merged

JoelSpeed reviewed May 28, 2024

View reviewed changes

		// +kubebuilder:object:root=true
		// ClusterLogForwarderList contains a list of ClusterLogForwarder

	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Receiver Outputs"
	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Log Outputs"

LOG-4990: Initial impl of CLF.obs API #2451

LOG-4990: Initial impl of CLF.obs API #2451

Conversation

jcantrill commented Apr 27, 2024

Description

Links

openshift-ci-robot commented Apr 27, 2024 • edited by openshift-ci bot

Description

Links

jcantrill commented Apr 27, 2024

openshift-ci bot commented Apr 27, 2024

periklis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis 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

xperimental left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcantrill commented May 3, 2024

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

jcantrill commented May 6, 2024

xperimental commented May 7, 2024

jcantrill commented May 7, 2024

openshift-ci bot commented May 7, 2024

xperimental commented May 7, 2024

openshift-ci bot commented May 7, 2024

JoelSpeed 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

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 27, 2024 •

edited by openshift-ci bot