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

capi: suppress warnings on creating files without mode #339

Merged

Conversation

dongsupark
Copy link
Member

Since Ansible 2.9.12 or 2.8.14, Ansible shows the following warning, when a file gets created with mode not being specified in the config.

[WARNING]: File '/tmp/kube-apiserver.tar' created with default permissions
'600'. The previous default was '666'. Specify 'mode' to avoid this warning.

To get rid of such a warning, specify mode to 0600 when creating files, as much as possible.

See also ansible/ansible#71200 .

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 21, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @dongsupark. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 21, 2020
@dongsupark
Copy link
Member Author

/assign @detiber

@codenrhoden
Copy link
Contributor

/ok-to-test

Thanks for bringing this up, @dongsupark. That issue you linked was illuminating.
I'm slightly worried that there will be unexpected behavior where something that was previously group readable no longer is, or some other version. It's a big change that we'll have to test out in full for sure.

@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 Aug 26, 2020
@dongsupark
Copy link
Member Author

@codenrhoden Sure. Let me know if any regression was found during the full tests.

@@ -50,21 +51,25 @@
section: Service
option: Type
value: notify
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be 0644 judging by the state of existing systemd unit files on my system

Suggested change
mode: 0600
mode: 0644


- name: Create containerd boot order drop in file
template:
dest: /etc/systemd/system/containerd.service.d/boot-order.conf
src: etc/systemd/system/containerd.service.d/boot-order.conf
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: 0600
mode: 0644


- name: Create containerd memory pressure drop in file
template:
dest: /etc/systemd/system/containerd.service.d/memory-pressure.conf
src: etc/systemd/system/containerd.service.d/memory-pressure.conf
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: 0600
mode: 0644


- name: Create containerd max tasks drop in file
template:
dest: /etc/systemd/system/containerd.service.d/max-tasks.conf
src: etc/systemd/system/containerd.service.d/max-tasks.conf
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: 0600
mode: 0644

@@ -75,6 +80,7 @@
template:
dest: /etc/containerd/config.toml
src: etc/containerd/config.toml
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

package installed version of this file is 0644 on my system.

Suggested change
mode: 0600
mode: 0644

@@ -34,6 +34,7 @@
overlay
br_netfilter
dest: /etc/modules-load.d/kubernetes.conf
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: 0600
mode: 0644

@@ -46,6 +46,7 @@
file:
src: /usr/libexec/cloud-init
dest: /usr/lib/cloud-init
mode: 0700
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect there to be a need for setting a mode for a symlink, I would expect it to be 0777 by default: https://man7.org/linux/man-pages/man7/symlink.7.html#:~:text=On%20Linux%2C%20the%20permissions%20of,and%20can't%20be%20changed.

@@ -16,6 +16,7 @@
template:
src: etc/apt/sources.list.j2
dest: /etc/apt/sources.list
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Judging by the default mode of /etc/apt/sources.list on a new system, this should probably be:

Suggested change
mode: 0600
mode: 0644

@@ -36,6 +37,7 @@
copy:
src: "{{ item }}"
dest: "/etc/apt/sources.list.d/{{ item | basename }}"
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: 0600
mode: 0644

@@ -29,5 +29,6 @@
copy:
src: "{{ item }}"
dest: "/etc/yum.repos.d/{{ item | basename }}"
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: 0600
mode: 0644

Since Ansible 2.9.12 or 2.8.14, Ansible shows the following warning,
when a file gets created with `mode` not being specified in the config.

```
[WARNING]: File '/tmp/kube-apiserver.tar' created with default permissions
'600'. The previous default was '666'. Specify 'mode' to avoid this warning.
```

To get rid of such a warning, specify `mode` to correct modes like 0644
when creating files, as much as possible.

See also ansible/ansible#71200 .
@dongsupark
Copy link
Member Author

@detiber Thanks for the suggestions. Updated the PR.

@@ -16,6 +16,7 @@
template:
dest: /home/builder/.bash_profile
src: photon_bash_profile
mode: 0600
Copy link
Member

Choose a reason for hiding this comment

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

@codenrhoden This is the only mode change that I'm unsure about, can you verify that the bash profile here still works as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried this out -- works just fine. I think this is good to go now.

@detiber
Copy link
Member

detiber commented Aug 26, 2020

/lgtm

Thanks for tackling this @dongsupark

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2020
@codenrhoden
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codenrhoden, dongsupark

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 Aug 26, 2020
@k8s-ci-robot k8s-ci-robot merged commit 8ef9f58 into kubernetes-sigs:master Aug 26, 2020
@dongsupark dongsupark deleted the dongsu/suppress-mode-warning branch August 27, 2020 09:07
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

4 participants