Skip to content

Commit

Permalink
logging: fix file mode configuration parsing (#6383)
Browse files Browse the repository at this point in the history
Commit 101d3e7 introduced file mode setting,
but was missing a JSON Marshaller so that
CaddyFile can be converted to JSON safely.
  • Loading branch information
ririsoft authored Jun 8, 2024
1 parent 9be4f19 commit 0bc27e5
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 0 deletions.
5 changes: 5 additions & 0 deletions modules/logging/filewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ func (m *fileMode) UnmarshalJSON(b []byte) error {
return err
}

// MarshalJSON satisfies json.Marshaler.
func (m *fileMode) MarshalJSON() ([]byte, error) {
return []byte(fmt.Sprintf("\"%04o\"", *m)), nil
}

// parseFileMode parses a file mode string,
// adding support for `chmod` unix command like
// 1 to 4 digital octal values.
Expand Down
39 changes: 39 additions & 0 deletions modules/logging/filewriter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,42 @@ func TestFileModeJSON(t *testing.T) {
})
}
}

func TestFileModeToJSON(t *testing.T) {
tests := []struct {
name string
mode fileMode
want string
wantErr bool
}{
{
name: "none zero",
mode: 0644,
want: `"0644"`,
wantErr: false,
},
{
name: "zero mode",
mode: 0,
want: `"0000"`,
wantErr: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var b []byte
var err error

if b, err = json.Marshal(&tt.mode); (err != nil) != tt.wantErr {
t.Fatalf("MarshalJSON() error = %v, want %v", err, tt.wantErr)
}

got := string(b[:])

if got != tt.want {
t.Errorf("got mode %v, want %v", got, tt.want)
}
})
}
}

10 comments on commit 0bc27e5

@ta2013
Copy link

@ta2013 ta2013 commented on 0bc27e5 Jun 9, 2024

Choose a reason for hiding this comment

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

Hi @ririsoft i womder does this mode get effect when reload config? Or just restart caddy?

my case:

currently exists logs have 600 , i add this mode 644, reload caddy, nothing happen.

i must backup , delete all logs, restart caddy, to get to work .

@ririsoft
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ta2013 : This is a good point and a use case I did not tested.
I need to do some testing. I will let you know once I have sorted this out and submit a PR if identify an easy fix.
Thanks for your testing it really helps !

@ririsoft
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ta2013 , @mholt : So here is the thing: I was assuming that os.OpenFile would set the mode even if the file already exists, but this is not what the docs says.
I will submit a PR to os.Chmod the file after opening it and before writing to it.

@mholt
Copy link
Member

@mholt mholt commented on 0bc27e5 Jun 11, 2024

Choose a reason for hiding this comment

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

Sounds good 👍 Thanks!

@ririsoft
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mholt : I have investigated further and I think I have identified and issue:

os.Chmod the file after opening it and before writing to it is not enough. We must ensure OpenWriter is called during config reload.

writer, loaded, err := writers.LoadOrNew(key, func() (Destructor, error) {

But when the configuration is reloaded the log files are not re-opened because already in the pool.
The only way to update the log file options (mode but also roll settings I suspect) is to restart the server.

I am not really sure on how to fix this, I have not a complete understanding on how the configuration reload works. In particular I do not want to introduce a situation where caddy is loosing logs during the reload if we re-open the log files.

I could submit a PR so that a restart would change the file mode without the need to remove the file as exposed by @ta2013, as a first step that slightly mitigate the issue.
But we want something better right ?

I am considering opening a new issue to track this. A solution for this could also fix #5316.

What do you think ?

@mholt
Copy link
Member

@mholt mholt commented on 0bc27e5 Jun 12, 2024

Choose a reason for hiding this comment

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

Honestly I'm not super sure it's the server's responsibility to change existing log file permissions. I think if new files are created with the specified permissions that's probably enough. What is the use case for the server changing the permissions of existing files? How often is that done?

@ririsoft
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I'm not super sure it's the server's responsibility to change existing log file permissions. I think if new files are created with the specified permissions that's probably enough. What is the use case for the server changing the permissions of existing files? How often is that done?

Personally I am fine with that. Logs permissions are usually set once for all, otherwise I would be interested to hear from the user story.

But if we do not document that I am afraid we will have more issues/discussions raised by users, because caddy is so good at reloading configuration with zero downtime that people are expecting every config options to be "reloadable". What do you think ? Where should I submit a PR to update the doc ?

@mholt
Copy link
Member

@mholt mholt commented on 0bc27e5 Jun 12, 2024

Choose a reason for hiding this comment

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

A good idea. Docs are on our website repo: https://github.com/caddyserver/website

@francislavoie
Copy link
Member

@francislavoie francislavoie commented on 0bc27e5 Jun 12, 2024

Choose a reason for hiding this comment

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

For next time, lets open an issue to discuss any followup next time 😅 it's impossible to read comments on commits from within the GitHub Mobile app unfortunately (see https://github.com/orgs/community/discussions/61420), so I couldn't follow the conversation while not at my PC. It's also much more visible/searchable long-term if we keep the discussion in issues.

@ririsoft
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#6391 opened to track the fix for setting modes to already existing files, so that a server restart will change the mode, without the need to delete existing logs.

Please sign in to comment.