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] Set file mode explicitly for regular files #1323

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

kevin85421
Copy link
Contributor

@kevin85421 kevin85421 commented Dec 3, 2023

Proposed changes

  • As shown in the following code snippet, the function ensureFiles checks the file mode for both regular files and secret files.

    if f.Type == file.TypeRegular {
    Expect(info.Mode()).To(Equal(os.FileMode(0o644)))
    } else {
    Expect(info.Mode()).To(Equal(os.FileMode(0o640)))
    }

  • The function ReplaceFiles in nginx/file/manager.go creates files by internally calling os.Create, which, by default, creates files with mode 0666 (before applying umask). See the source code of os.Create for more details.

  • The function writeFile changes the mode of secret files to 0640 by calling chmod, but does nothing for regular files. Hence, the check Expect(info.Mode()).To(Equal(os.FileMode(0o644))) in nginx/file/manager_test.go only passes for umask with specific values.

  • In my environment, the umask value is 002. Therefore, the mode for regular files will be 0666 - 0002 = 0664, causing the unit test to fail. In the following screenshot, 420 is 0o644, and 436 is 0o664.
    Screen Shot 2023-12-02 at 6 05 36 PM

  • Solution: This PR sets the file mode explicitly.

Checklist

Closes #1328

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork
  • Run make unit-test locally.
    Screen Shot 2023-12-02 at 6 08 43 PM

Copy link

netlify bot commented Dec 3, 2023

Deploy Preview for nginx-gateway-fabric ready!

Name Link
🔨 Latest commit 1976f20
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/656f3cabd64150000896d48a
😎 Deploy Preview https://deploy-preview-1323--nginx-gateway-fabric.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kevin85421 kevin85421 changed the title WIP [Bug] Set file mode explicitly Dec 3, 2023
@kevin85421 kevin85421 changed the title [Bug] Set file mode explicitly [Bug] Set file mode explicitly for regular files Dec 3, 2023
@kevin85421 kevin85421 marked this pull request as ready for review December 3, 2023 02:09
@kevin85421 kevin85421 requested a review from a team as a code owner December 3, 2023 02:09
@sjberman
Copy link
Contributor

sjberman commented Dec 4, 2023

Please update your main commit message to contain the basic problem/solution as defined in your PR description.

@sjberman sjberman added the bug Something isn't working label Dec 4, 2023
@sjberman
Copy link
Contributor

sjberman commented Dec 4, 2023

Could you also create an associated bug to track and close with this PR?

@kevin85421
Copy link
Contributor Author

Please update your main commit message to contain the basic problem/solution as defined in your PR description.

Would you mind providing more details? Most open-source projects in which I have maintained/contributed squashed commits from a PR into a single commit and written the messages there. I am not sure how does NGINX community works.

In my understanding, you ask me to squash the commits manually and write the commit message:

# Step 1: Reset the 2 commits in this PR
git reset HEAD^^

# Step 2:
git add ${ALL_FILES_GOT_RESET}

# Step 3: Write commit message here
git commit

# Step 4:
git push -f ...

Is my understanding correct?

Could you also create an associated bug to track and close with this PR?

I have already opened an issue #1328.

@sjberman
Copy link
Contributor

sjberman commented Dec 4, 2023

Yes, we can supply the commit message when squashing on a merge. However, it makes it easier for us when reviewing to know what changes are going into each commit. "update" doesn't tell us anything useful about the changes that were made in that commit.

It also saves us the step of copying the PR description into the squashed commit message because it'll already be there if you have that commit message to start with.

You don't have to squash the commits manually, but the first commit should at least have a useful message (likely matching your PR description).

@sjberman
Copy link
Contributor

sjberman commented Dec 4, 2023

You can also squash or change a commit message by doing git rebase -i head~X where X is the number of commits you want to go back and manage.

* As shown in the following code snippet, the function `ensureFiles`
checks the file mode for both regular files and secret files.
  https://github.com/nginxinc/nginx-gateway-fabric/blob/6d4cfd7f0de32e9f98dae358cb6cec93529109a5/internal/mode/static/nginx/file/manager_test.go#L43-L47

* The function `ReplaceFiles` in `nginx/file/manager.go` creates files
by internally calling [os.Create](https://pkg.go.dev/os#Create), which,
by default, creates files with mode 0666 (before applying `umask`). See
the [source
code](https://github.com/golang/go/blob/de5b418bea70aaf27de1f47e9b5813940d1e15a4/src/os/file.go#L357-L364)
of `os.Create` for more details.

* The function `writeFile` changes the mode of secret files to 0640 by
calling `chmod`, but does nothing for regular files. Hence, the check
`Expect(info.Mode()).To(Equal(os.FileMode(0o644))) ` in
`nginx/file/manager_test.go` only passes for `umask` with specific
values.

* In my environment, the `umask` value is 002. Therefore, the mode for
regular files will be 0666 - 0002 = 0664, causing the unit test to fail.
In the following screenshot, 420 is 0o644, and 436 is 0o664.
  ![Screen Shot 2023-12-02 at 6 05 36
PM](https://github.com/nginxinc/nginx-gateway-fabric/assets/20109646/b621c7de-2465-4c5a-988b-4cf625e5dca7)

* Solution: This PR sets the file mode explicitly.
@kevin85421
Copy link
Contributor Author

Thank you for the explanation! It's a bit embarrassing, but this is the first time I've learned how to write long commit messages locally lol. I have already updated the PR.

@sjberman sjberman enabled auto-merge (squash) December 5, 2023 15:08
@sjberman sjberman merged commit d27dde5 into nginxinc:main Dec 5, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug] File mode tests for regular files pass only with specific umask values
3 participants