The committers listed above are authorized under a signed CLA.

• ✅ login: luozhiwenn / name: zhwn (76c8765)

Welcome @luozhiwenn!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @luozhiwenn. 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.

/ok-to-test

Is there a reason to add this specific chmod vs say, altering umask in the process? What's the use case where we'd want to have a custom umask only for some of kubelet's files?

Is there a reason to add this specific chmod vs say, altering umask in the process? What's the use case where we'd want to have a custom umask only for some of kubelet's files?

Hi, is the following change what you refer to altering umask in the process?

func ensureHostsFile(fileName string, hostIPs []string, hostName, hostDomainName string, hostAliases []v1.HostAlias, useHostNetwork bool) error {
        // ...
        oldMask, _ := util.Umask(0)
	err = ioutil.WriteFile(fileName, hostsFileContent, 0644)
	util.Umask(oldMask)
        return err
}

There is a concurrency problem in altering umask in the process because method ensureHostsFile is invoked concurrently:

1. kubelet with umask 027
2. Creating Pod A etc-hosts file, save oldMask 027 and set umask to 000
3. Concurrently creating Pod B etc-hosts file, save oldMask 000 and set umask to 000
4. Creates Pod A etc-hosts file complete, set umask back to oldMask 027
5. Creates Pod B etc-hosts file complete, set umask back to oldMask 000
6. kubelet with umask 000

This concurrency problem causes the umask of the kubelet to be modified, therefore, I prefer to explicitly correct permissions with chmod rather than adding lock in this process.

Inspired by

No I am referring to syscall.Umask early in the process and leaving it altered.

Instead of scoped only to writing this particular file.

Or alternately:

What is the use case to not permanently alter the umask of kubelet early in the process?

What use case requires a umask for kubelet that does not require it here?

If we're going to intentionally ignore umask for this file, why not for the whole kubelet process?

Apologies for the delayed response.

Or alternately:

What is the use case to not permanently alter the umask of kubelet early in the process?

What use case requires a umask for kubelet that does not require it here?

If we're going to intentionally ignore umask for this file, why not for the whole kubelet process?

• For security purposes, umask is configured by default(022 or 027?) in many Linux systems. We hope that the kubelet conforms to this configuration, so that files with excessive permissions are not created inadvertently. In particular, kubelet is usually run as the root user, and the owner of the files created by kubelet is also root, which shuld be careful.
• Different from other files created by kubelet (apps are not interested in), applications read the /etc/hosts file in a typical scenario. Therefore, we want to ensure that the permission on the file is readable by other users regardless of the umask of kubelet.

/priority important-longterm
/triage accepted
/assign @BenTheElder

@luozhiwenn as I understand it, the bug occurs only when there is no /etc/hosts file, which forces os.WriteFile to create it with 0644 + umask?
I hope there isn't a possible race condition between the time you create the file, and when you set the proper permissions?

Forgot to comment: I unassigned because I'm not an approver here and someone who is needs to decide on the approach.

I hope there isn't a possible race condition between the time you create the file, and when you set the proper permissions?

I don't think that's an issue because we're relaxing permissions, not tightening them.

This is unavoidable I think, unless maybe we write it to a temp location, chmod, then move it (like projected configmaps)

/lgtm

LGTM label has been added.

/assign @derekwaynecarr @dchen1107 @mrunalp

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

/remove-lifecycle stale
@kubernetes/sig-node-pr-reviews

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luozhiwenn, mrunalp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

I think maybe this warrants a release note? It's a behavior change.

release note: "kubelet will ensure /etc/hosts file is mode 0644 regardless of umask" or something along those lines?

In case we're updating the release notes, which you can do by editing the PR body:
/hold

Comment /hold cancel to remove the hold and merge when your ready (including if we're not changing release note and going ahead and merging as-is).

In case we're updating the release notes, which you can do by editing the PR body: /hold

Comment /hold cancel to remove the hold and merge when your ready (including if we're not changing release note and going ahead and merging as-is).

done

/hold cancel