LOG-1002: Add cloudwatch logforwarding type #839

jcantrill · 2020-12-15T17:27:17Z

Description

This PR add logforwarding to Cloudwatch capabilities

/cc @igor-karpukhin
/assign @alanconway

Links

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

/hold

jcantrill · 2020-12-15T19:09:09Z

/hold

alanconway

/lgtm

We may want to modify the naming and API but we should merge this as a first cut and work from there.

jcantrill · 2020-12-16T17:27:43Z

/retest

jcantrill · 2020-12-16T17:29:15Z

We may want to modify the naming and API but we should merge this as a first cut and work from there.

@alanconway since the API is not firm here, I hesitate to merge because then that IS the API. I'm still working to create TP branches where we could merge this and call it beta. That's ultimately what I would most desire to deliver for ROSA unless we are confident in the api

alanconway · 2020-12-17T22:55:10Z

/lgtm cancel

Here's the API enhancement proposal: openshift/enhancements#570
Not a big change, has some small extra bits.

jcantrill · 2021-01-15T19:06:14Z

@alanconway review comments please

alanconway · 2021-01-25T15:26:00Z

LGTM but do we want to start the tech-preview branch for this?

alanconway

Looks good, minor changes requested.

pkg/apis/logging/v1/output_types.go

alanconway · 2021-02-12T20:34:51Z

pkg/generators/forwarding/fluentd/fluent_conf.go

+}
+func (conf *outputLabelConf) LogGroupPrefix() string {
+	if conf.Target.Type == logging.OutputTypeCloudwatch {
+		prefix := conf.Target.Cloudwatch.GroupPrefix


Put the TrimSpace here so we check for "" and use the trimmed value.

I don't believe I can do that without losing the ability to treat the following as different cases:

nil = use default

non-nil and not empty = use value

pkg/generators/forwarding/fluentd/templates.go

alanconway · 2021-02-12T21:00:06Z

pkg/generators/forwarding/fluentd/templates.go

+    remove_log_group_name_key true
+    auto_create_stream true
+    concurrency 2
+    aws_key_id "#{open('{{ .SecretPath "aws_access_key_id"}}','r') do |f|f.read end}"


Only set this if the key is found, otherwise we'll have a new crash loop when its missing. Here's what I did for the general TLS stuff:

{{- with $path := .SecretPathIfFound "tls.key"}} tls_client_private_key_path "{{$path}}" {{- end}}

Also if these keys are required, they should be checked as part of VerifyOutputSecret, but only for cloudwatch outputs.

These are absolutely required for the plugin to work. Will the generator code prior to the template successfully skip output generation for missing secrets?

alanconway · 2021-02-12T21:05:10Z

pkg/k8shandler/clusterloggingrequest.go

+
+	//CLFVerifier is a collection of functions to control verification
+	//of ClusterLogForwarding
+	CLFVerifier ClusterLogForwarderVerifier


Not sure this is the best approach. We need to customize the verify behavior per-output, not for the whole forwarder,

Perhaps a map of map[string]VerifyFunc = { "cloudwatch": ...; "kafka": ... }, so Normalize can call the right verify func for each output type.

This was added to allow the internal config generator bin to work without being connected to the cluster and really is not intended to be used otherwise. We can repurpose it if needed but ATM I don't meet the need.

OK, understood. However we still need different secret verification for cloudwatch (needs "aws_...") vs. other TLS outputs. We could have a global map of verify functions keyed by output type, and have output types with special needs put a function in the map that the validation code will call if not nil. What do you think?

sure. if that is needed seems reasonable

Not needed for this PR. This is OK

jcantrill · 2021-02-16T21:28:18Z

/hold cancel

alanconway

/lgtm

igor-karpukhin · 2021-02-18T16:56:29Z

/lgtm

openshift-ci-robot · 2021-02-18T16:56:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, igor-karpukhin, 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~~ [alanconway,jcantrill]

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

jcantrill · 2021-02-18T20:09:03Z

/retest

LOG-1002: Add cloudwatch logforwarding type

openshift-ci-robot assigned alanconway Dec 15, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2020

openshift-ci-robot requested a review from igor-karpukhin December 15, 2020 17:27

jcantrill added release/4.7 and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 15, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2020

jcantrill force-pushed the LOG-1002 branch from 8285693 to 0c0f79b Compare December 15, 2020 19:07

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2020

alanconway approved these changes Dec 16, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2020

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Dec 17, 2020

jcantrill force-pushed the LOG-1002 branch from 13ee76e to 5189005 Compare January 14, 2021 21:32

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2021

jcantrill force-pushed the LOG-1002 branch from 5189005 to ed2b546 Compare January 15, 2021 13:53

jcantrill force-pushed the LOG-1002 branch from ed2b546 to 494a36a Compare January 21, 2021 16:33

jcantrill force-pushed the LOG-1002 branch from 494a36a to 1a1ddd9 Compare January 25, 2021 18:26

jcantrill force-pushed the LOG-1002 branch from 1a1ddd9 to 04e4a69 Compare February 5, 2021 19:57

jcantrill changed the base branch from master to tech-preview February 5, 2021 19:58

jcantrill force-pushed the LOG-1002 branch 2 times, most recently from 84fc4ac to a5e2f9f Compare February 11, 2021 19:16

alanconway suggested changes Feb 12, 2021

View reviewed changes

jcantrill added release/5.1 and removed release/4.7 labels Feb 15, 2021

jcantrill force-pushed the LOG-1002 branch from a5e2f9f to a862be5 Compare February 15, 2021 20:10

LOG-1002: Add cloudwatch logforwarding type

4f55e73

jcantrill force-pushed the LOG-1002 branch from a862be5 to 4f55e73 Compare February 16, 2021 20:17

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2021

alanconway reviewed Feb 18, 2021

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2021

openshift-ci-robot assigned igor-karpukhin Feb 18, 2021

openshift-merge-robot merged commit 1848765 into openshift:tech-preview Feb 18, 2021

pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022

Merge pull request openshift#839 from jcantrill/LOG-1002

ca5ed96

LOG-1002: Add cloudwatch logforwarding type

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOG-1002: Add cloudwatch logforwarding type #839

LOG-1002: Add cloudwatch logforwarding type #839

jcantrill commented Dec 15, 2020

jcantrill commented Dec 15, 2020

alanconway left a comment

jcantrill commented Dec 16, 2020

jcantrill commented Dec 16, 2020 •

edited

Loading

alanconway commented Dec 17, 2020

jcantrill commented Jan 15, 2021

alanconway commented Jan 25, 2021

alanconway left a comment

alanconway Feb 12, 2021

jcantrill Feb 15, 2021

alanconway Feb 12, 2021

jcantrill Feb 15, 2021

jcantrill Feb 16, 2021

alanconway Feb 12, 2021

jcantrill Feb 15, 2021 •

edited

Loading

alanconway Feb 15, 2021

jcantrill Feb 16, 2021

alanconway Feb 18, 2021

jcantrill commented Feb 16, 2021

alanconway left a comment

igor-karpukhin commented Feb 18, 2021

openshift-ci-robot commented Feb 18, 2021

jcantrill commented Feb 18, 2021

LOG-1002: Add cloudwatch logforwarding type #839

LOG-1002: Add cloudwatch logforwarding type #839

Conversation

jcantrill commented Dec 15, 2020

Description

Links

jcantrill commented Dec 15, 2020

alanconway left a comment

Choose a reason for hiding this comment

jcantrill commented Dec 16, 2020

jcantrill commented Dec 16, 2020 • edited Loading

alanconway commented Dec 17, 2020

jcantrill commented Jan 15, 2021

alanconway commented Jan 25, 2021

alanconway 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

jcantrill Feb 15, 2021 • 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

jcantrill commented Feb 16, 2021

alanconway left a comment

Choose a reason for hiding this comment

igor-karpukhin commented Feb 18, 2021

openshift-ci-robot commented Feb 18, 2021

jcantrill commented Feb 18, 2021

jcantrill commented Dec 16, 2020 •

edited

Loading

jcantrill Feb 15, 2021 •

edited

Loading