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

Bug 1862426: gather the audit logs for oauth apiserver #144

Merged
merged 4 commits into from
Sep 23, 2020

Conversation

sanchezl
Copy link
Contributor

@sanchezl sanchezl commented Mar 11, 2020

  • simplified logic
  • added progress logging
  • also capture oauth-apiserver logs
  • add SINCE_TIMEFRAME env, defaulted to 1w

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 11, 2020
@sferich888
Copy link
Contributor

@sanchezl we need to review this in the context of #143 (and what it is trying to achieve). #164 is also going to be affected by this.

Copy link
Contributor

@sferich888 sferich888 left a comment

Choose a reason for hiding this comment

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

If we make these changes, we can close; #143 and work on merging this; once https://bugzilla.redhat.com/show_bug.cgi?id=1861746 is investigated and fixed.

done | \
xargs --max-args=4 --max-procs=45 bash -c \
'echo "INFO: Started downloading $4 from $3";
oc adm node-logs $3 --path=$1/$4 | gzip > $2/$3-$4.gz;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add --since ${SINCE_TIMEFRAME} to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sferich888 , I have added the --since parameter, but it doesn't seem to actually do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --since parameter is only applied to journal logs. It it ignored for --path logs, which are simply served by an instance of http.FileServer.

Copy link
Contributor

Choose a reason for hiding this comment

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

What other ways can we limit the collection?

Is an awk script needed?

#!/usr/bin/gawk -f BEGIN { starttime = mktime(starttime) endtime = mktime(endtime) } func in_range(n, start, end) { return start <= n && n < end } match($0, /^([0-9]{4})-([0-9]{2})-([0-9]{2})\s/, m) && in_range(mktime(m[1] " " m[2] " " m[3] " 00 00 00"),

Source: https://askubuntu.com/questions/800769/how-to-get-text-from-range-of-dates-using-grep-sed-in-large-text-file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sferich888 I think I can work something out, but as a followup.

Choose a reason for hiding this comment

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

In others scripts I see the full path of command, like:/usr/bin/oc

# $2 - local output path
# $3 - node name
# $4 - log file name
paths=(openshift-apiserver kube-apiserver)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sanchezl please add oauth-apiserver to the list and close #164

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sferich888
Copy link
Contributor

sferich888 commented Jul 31, 2020

@sanchezl we need to make sure we look into https://bugzilla.redhat.com/show_bug.cgi?id=1861746

@sferich888
Copy link
Contributor

@sanchezl also note RFE-309 is something that will be closed by this.

@sanchezl sanchezl force-pushed the patch-1 branch 3 times, most recently from f1354ae to 73ce5db Compare August 2, 2020 04:30
@sanchezl sanchezl changed the title Update gather_audit_logs Bug 1862426 - gather the audit logs for oauth apiserver Aug 3, 2020
@sanchezl
Copy link
Contributor Author

sanchezl commented Aug 4, 2020

Waiting for openshift/origin#25369.

@soltysh
Copy link
Contributor

soltysh commented Aug 11, 2020

/retets

@sanchezl
Copy link
Contributor Author

/retest

1 similar comment
@soltysh
Copy link
Contributor

soltysh commented Sep 3, 2020

/retest

@sanchezl
Copy link
Contributor Author

sanchezl commented Sep 5, 2020

/test e2e-aws

@sanchezl
Copy link
Contributor Author

sanchezl commented Sep 6, 2020

/test-e2e-aws

@sanchezl sanchezl changed the title Bug 1862426 - gather the audit logs for oauth apiserver Bug 1862426: gather the audit logs for oauth apiserver Sep 6, 2020
@openshift-ci-robot
Copy link

@sanchezl: This pull request references Bugzilla bug 1862426, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1862426: gather the audit logs for oauth apiserver

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-robot openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 6, 2020
@sanchezl
Copy link
Contributor Author

sanchezl commented Sep 6, 2020

/retest

@soltysh
Copy link
Contributor

soltysh commented Sep 9, 2020

/retest

@mfojtik
Copy link
Contributor

mfojtik commented Sep 10, 2020

/approve

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2020
@@ -41,9 +41,6 @@ done
# Gather etcd information
/usr/bin/gather_etcd

# Collect System Audit Logs
/usr/bin/gather_audit_logs
Copy link
Contributor

Choose a reason for hiding this comment

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

you can't do this until the e2e test works.

Copy link
Contributor

Choose a reason for hiding this comment

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

that e2e you're talking about already explicitly calls this script see https://github.com/openshift/origin/blob/1a83021f4dbc91de606a71d3a27cd80664aae26c/test/extended/cli/mustgather.go#L160 so it shouldn't block this PR

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020
@deads2k
Copy link
Contributor

deads2k commented Sep 22, 2020

wait for openshift/origin#25493 to merge

@deads2k
Copy link
Contributor

deads2k commented Sep 22, 2020

/test all
/lgm
/hold

need to see green tests including audit (not skipped) before merge.

@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 Sep 22, 2020
@deads2k deads2k added 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. labels Sep 22, 2020
@sanchezl
Copy link
Contributor Author

/test all

Now that openshift/origin#25493 has merged.

@sttts
Copy link
Contributor

sttts commented Sep 23, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2020
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

Green audit e2e
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: mfojtik, sanchezl, soltysh

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-merge-robot openshift-merge-robot merged commit b35e739 into openshift:master Sep 23, 2020
@openshift-ci-robot
Copy link

@sanchezl: All pull requests linked via external trackers have merged:

Bugzilla bug 1862426 has been moved to the MODIFIED state.

In response to this:

Bug 1862426: gather the audit logs for oauth apiserver

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.

@sanchezl sanchezl deleted the patch-1 branch January 26, 2021 20:00
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. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.