Unhandled errors from deferred file close operations

[dholbach]

From Ada Logics

Target
./source-controller/controllers/storage.go
./source-controller/pkg/sourceignore/sourceignore.go
./kustomize-controller/controllers/kustomization_impersonation.go
./kustomize-controller/internal/sops/pgp/keysource.go

Throughout the codebase there are places where file close operations are deferred within a function where a file is being written to, e.g:

localPath := s.LocalPath(*artifact)
f, err := os.Open(localPath)
if err != nil {
  return err
}
defer f.Close()

// untar the artifact
untarPath := filepath.Join(tmp, "unpack")
if _, err = untar.Untar(f, untarPath); err != nil {
  return err
}

This can lead to undefined behaviour since any errors returned by the f.Close() operation are ignored. This can have consequences in the event where a close operation failed and the data was not yet flushed to the file, which the rest of the code will assume it to be. For a detailed discussion on this, please see here.

Recommendation
Ensure that errors from f.Close() are handled.

[tomhuang12]

I took a stab at it on kustomize-controller. Please take a look. If this looks good, I will apply the same change to source-controller.

[tomhuang12]

Closed PR for kustomize-controller as file write operation is already taken care of and file read does not need this particular update.

I have also gone through ./source-controller/controllers/storage.go and ./source-controller/pkg/sourceignore/sourceignore.go and looks like the file write operations are also updated properly. I believe this issue can be closed.

[pjbgf]

@tomhuang12 thank you for reviewing. I just had another look and I think the only outstanding occurrency is:

https://github.com/fluxcd/source-controller/blob/main/controllers/storage.go#L358

Do you mind having a look at it please?

[tomhuang12]

Thanks for a second look @pjbgf 😅 . Please review my PR and let me know if there's any issues. Hopefully it looks good the first try 🤞 !

[pjbgf]

@dholbach this issue can be marked as done.