Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement Proposal: API to Forward Logs to CloudWatch #570

Merged

Conversation

alanconway
Copy link
Contributor

@alanconway alanconway commented Dec 17, 2020

@openshift-ci-robot
Copy link

@alanconway: GitHub didn't allow me to assign the following users: jcantril.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/cc @jcantrill
/cc @jeremyeder
/cc @sichvoge
/assign @jcantril

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.

@alanconway
Copy link
Contributor Author

/assign @jcantrill

Copy link

@portante portante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit complicated for a first pass with the ability to set groups and streams arbitrarily.

Is there a way to just support 3 groups (app, infra, audit) and/or 2 groups (infra & audit) and apps grouped by namespace? Seems like those would be easier to get right in the very, very short amount of time we have to implement this.

enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
@alanconway alanconway force-pushed the forward-cloudwatch branch 3 times, most recently from cbc1641 to 93d1887 Compare December 18, 2020 16:57
@alanconway
Copy link
Contributor Author

@jewzaam @portante @jcantrill update taking account of all your comments (thanks for that) but may still be some questions outstanding.

@jcantrill
Copy link
Contributor

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 4, 2021
@alanconway
Copy link
Contributor Author

@jcantrill @portante I think the latest draft may be a winner, please comment if you agree so we can LGTM and unhold.

enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@alanconway alanconway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feedback, I think we're close. @portante @jcantrill please scan again. I've clarified the role of log stream names, otherwise its small changes.

enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
Copy link

@sichvoge sichvoge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an additional comment for retentionDays since I am not comfortable to actually expose this in our API. See explanation in my comment.

enhancements/cluster-logging/forward_to_cloudwatch.md Outdated Show resolved Hide resolved
@alanconway alanconway force-pushed the forward-cloudwatch branch 2 times, most recently from 5376054 to dd3954f Compare January 12, 2021 16:36
@alanconway
Copy link
Contributor Author

@jcantrill @portante @sichvoge all comments taken on board, properly updated this time (I managed to check this out twice so brief confusion) Please comment.

@jcantrill
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link

[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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021

- `region`: (string) AWS region name, required to connect.
- `groupBy`: (string, default "category") Take group name from logging meta-data. Values:
- `category`: category of log entry - one of "application", "infrastructure", or "audit"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this source to be consistent with the rest of logforwarding


For the first implementation, the log stream name will be:
- *container logs*: use the fluentd tag, via the `use_tag_as_stream true` plugin setting. Unique because it includes container-id.
- *node logs* (audit, infrastructure): use fluentd tag + node-id. Unique per node and per audit/infrastructure log file.
Copy link
Contributor

@jcantrill jcantrill Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what or how to get node-id but the node name is same as host name. Landing on this which would give you journal and all audit logs grouped by node hostname:

<label {{.LabelName}}>
  <filter kubernetes.**>
    @type record_transformer
    <record>
      cw_group_name {{.LogGroupName }}
      cw_stream_name ${record["kubernetes"]["container_image_id"]}
    </record>
  </filter>
  <filter journal.** *audit.log>
    @type record_transformer
    <record>
      cw_group_name ${record["hostname"]}
      cw_stream_name ${tag}
    </record>
  </filter>

I can remove the container streams to use tag instead of the hash which would include NS/Podname/containerhash some thing like var.log.containers.NS.podname.hash or the like

Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold cancel
/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 15, 2021
@openshift-merge-robot openshift-merge-robot merged commit 2b06f37 into openshift:master Jan 15, 2021
@alanconway alanconway deleted the forward-cloudwatch branch March 10, 2021 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants