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

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

Conversation

vpnachev
Copy link
Contributor

@vpnachev vpnachev commented Jun 27, 2022

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


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

k8s-ci-robot commented Jun 27, 2022

@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

k8s-ci-robot commented Jun 27, 2022

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
Copy link
Contributor Author

vpnachev commented Jun 27, 2022

/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

k8s-ci-robot commented Jun 27, 2022

@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

k8s-ci-robot commented Jun 27, 2022

@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

leilajal commented Jun 30, 2022

/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?

Copy link
Member

@liggitt liggitt left a comment

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

@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 Indicates that a PR is ready to be merged. label Jul 18, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 18, 2022

[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
14 checks passed
@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 Jul 18, 2022
k8s-ci-robot added a commit that referenced this pull request Aug 4, 2022
…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
…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
…0813-origin-release-1.24

Automated cherry pick of #110813: Ensure the dir of --audit-log-path exists
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. 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 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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants