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

Jump to bottom

Ensure the directory for the file in flag --audit-log-path exists #110813

Merged
merged 1 commit into from Jul 18, 2022
Merged

Ensure the directory for the file in flag --audit-log-path exists #110813

merged 1 commit into from Jul 18, 2022

Conversation

vpnachev
Copy link
Contributor

@vpnachev vpnachev commented Jun 27, 2022 •
edited by k8s-ci-robot

What type of PR is this?

/kind bug
/sig api-machinery
/sig auth
/area apiserver

What this PR does / why we need it:

Ensure the directory for the file in flag --audit-log-path exists.

Until #95387 the gopkg.in/natefinch/lumberjack.v2 module was taking care to create the file and the directory specified in the --audit-log-path flag, but since then if the directory of the file does not exist, the apiserver (not k8s only, may be any extension apiserver using the k8s.io/apiserver module) fails with error like

Error: ensureLogFile: open /tmp/audit/audit.log: no such file or directory

when the direcotry /tmp/audit does not exist.

With this change, if the directory already exist, nothing is changed, including the already set permissions.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

If the parent directory of the file specified in the `--audit-log-path` argument does not exist, Kubernetes now creates it.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@vpnachev
Ensure the dir of --audit-log-path exists
a380ef5 
Signed-off-by: Vladimir Nachev <vladimir.nachev@sap.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 27, 2022
@k8s-ci-robot
Copy link
Contributor

@vpnachev: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 27, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @vpnachev. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@vpnachev vpnachev mentioned this pull request Jun 27, 2022
Merged
@vpnachev
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 27, 2022
@sftim
Copy link
Contributor

sftim commented Jun 27, 2022

/release-note-edit

If the parent directory of the file specified in the --audit-log-path argument does not exist, Kubernetes now creates it.

@k8s-ci-robot
Copy link
Contributor

@sftim: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit

If the parent directory of the file specified in the --audit-log-path argument does not exist, Kubernetes now creates it.

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.

@sftim
Copy link
Contributor

sftim commented Jun 27, 2022

/release-note-edit

If the parent directory of the file specified in the `--audit-log-path` argument does not exist, Kubernetes now creates it.

@k8s-ci-robot
Copy link
Contributor

@sftim: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit

If the parent directory of the file specified in the `--audit-log-path` argument does not exist, Kubernetes now creates it.

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.

@sftim
Copy link
Contributor

sftim commented Jun 27, 2022

/release-note-edit

If the parent directory of the file specified in the `--audit-log-path` argument does not exist, Kubernetes now creates it.

@sftim
Copy link
Contributor

sftim commented Jun 27, 2022

Sorry about the noise. Anyway, I think that text is more idiomatic and also clearer.

@dims
Copy link
Member

dims commented Jun 28, 2022

/assign @deads2k

David, as reviewer/approver on #95387 WDYT?

@leilajal
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 30, 2022
@vpnachev
Copy link
Contributor Author

vpnachev commented Jul 8, 2022

@liggitt @deads2k have you got a chance to take a look at this PR?

liggitt
liggitt reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

one question about an edge case with an audit log path that only specifies a filename and no directory, lgtm otherwise

staging/src/k8s.io/apiserver/pkg/server/options/audit.go Show resolved Hide resolved
@dims
Copy link
Member

dims commented Jul 18, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2022
@liggitt
Copy link
Member

liggitt commented Jul 18, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, vpnachev

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2022
@k8s-ci-robot k8s-ci-robot merged commit ff20035 into kubernetes:master Jul 18, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jul 18, 2022
@vpnachev vpnachev deleted the apiserver/create-dir-for-audit-log-path branch July 18, 2022 16:42
This was referenced Jul 18, 2022
Merged
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Aug 4, 2022
@k8s-ci-robot
Merge pull request #111227 from vpnachev/automated-cherry-pick-of-#11…
5dd5cf1 
…0813-origin-release-1.22

Automated cherry pick of #110813: Ensure the dir of --audit-log-path exists
k8s-ci-robot added a commit that referenced this pull request Aug 5, 2022
@k8s-ci-robot
Merge pull request #111226 from vpnachev/automated-cherry-pick-of-#11…
19c5ffe 
…0813-origin-release-1.23

Automated cherry pick of #110813: Ensure the dir of --audit-log-path exists
k8s-ci-robot added a commit that referenced this pull request Aug 5, 2022
@k8s-ci-robot
Merge pull request #111225 from vpnachev/automated-cherry-pick-of-#11…
3dc15e1 
…0813-origin-release-1.24

Automated cherry pick of #110813: Ensure the dir of --audit-log-path exists
@vpnachev vpnachev mentioned this pull request Aug 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims dims left review comments

@liggitt liggitt liggitt left review comments

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@liggitt liggitt

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vpnachev @k8s-ci-robot @sftim @dims @leilajal @liggitt @deads2k