Apply some idiomatic patterns to CPLB component #4289
Conversation
|
This pull request has merge conflicts that need to be resolved. |
|
This pull request has merge conflicts that need to be resolved. |
|
LGTM, needs to pass tests again after #4290 is merged |
That's how the other components are configuring the supervisor. Don't deviate from the standards unless there's a reason for it. Signed-off-by: Tom Wieczorek <[email protected]>
The file is ephemeral and doesn't need to live on a persistent path. This also saves us from ensuring the existence of the base directory in the cplb component, as the RunDir's existence is ensured during controller startup. Signed-off-by: Tom Wieczorek <[email protected]>
This aligns with other file generation routines in k0s. Also, use template.Must(...) to parse the template string, as it's a hard coded constant. Signed-off-by: Tom Wieczorek <[email protected]>
|
Rebased. |
| return fmt.Errorf("failed to parse keepalived template: %w", err) | ||
| } | ||
|
|
||
| template := template.Must(template.New("keepalived").Parse(keepalivedConfigTemplate)) |
There was a problem hiding this comment.
| template := template.Must(template.New("keepalived").Parse(keepalivedConfigTemplate)) | |
| template, err := template.New("keepalived").Parse(keepalivedConfigTemplate) | |
| if err != nil { | |
| return fmt.Errorf("parse keepalived template: %w", err) | |
| } |
There was a problem hiding this comment.
I think those "Must" style functions are specifically meant for const inputs like here. Why bother with error handling if the input is guaranteed to be valid? Using template.Must(...) here is no different from using e.g. regex.MustParse("[a-z0-9]").
There was a problem hiding this comment.
I think it's to ease defining globals:
var parsedTemplate = template.Must(template.New("keepalived").Parse(keepalivedConfigTemplate))
func main() {
parsedTemplate.Execute(...)
}At least, that seems to be the intention of regexp.MustCompile:
MustCompile is like [Compile] but panics if the expression cannot be parsed.
It simplifies safe initialization of global variables holding compiled regular
expressions.
So, as the template is constant, it could be parsed at startup:
-const keepalivedConfigTemplate = `
+var keepalivedConfigTemplate = template.Must(template.New("keepalived").Parse(`
| if err = os.Chmod(k.K0sVars.KeepalivedConfigFile, fs.FileMode(0400)); err != nil { | ||
| return fmt.Errorf("failed to chmod keepalived config file: %w", err) | ||
| if err := file.WriteAtomically(k.configFilePath, 0400, func(file io.Writer) error { | ||
| if err := file.(*os.File).Chown(k.uid, -1); err != nil { |
There was a problem hiding this comment.
maybe file.WriteAtomicallys callback should be func (file *os.File) if it's always a file.
There was a problem hiding this comment.
This is actually me cheating here because I know it's a file 😈
When I created the WriteAtomically function, I explicitly wanted to hide the fact that this is a file, so that nobody would do unintended things like calling file.Name(), file.Remove() and so on. The function's API has the permissions, but it's lacking ownership information. I missed that, since os.WriteFile is similar, probably because you cannot convey ownership information through the open syscall and have to do it separately? Have to think about it a bit...
There was a problem hiding this comment.
That said, I'm not even 100% sure right now that os.Rename will keep ownership info. I assume, but would I bet? For Linux and Windows?
There was a problem hiding this comment.
Reminder to self: os.Chwon/file.Chown is not a thing on Windows anyways.
Description
This is how other components do it.
The file is ephemeral and doesn't need to live on a persistent path. This also saves us from ensuring the existence of the base directory in the cplb component, as the RunDir's existence is ensured during controller startup.
Type of change
How Has This Been Tested?
Checklist: