From 1f71e83d7219b2c3238338b7c55b4f684a612564 Mon Sep 17 00:00:00 2001 From: ShigrafS Date: Mon, 8 Sep 2025 17:13:13 +0530 Subject: [PATCH 1/7] refactor(file-handling): improve error handling and file cleanup Signed-off-by: ShigrafS --- files/tests/sanitization_test.go | 14 +++-- generators/github/git_repo.go | 10 +++- utils/kubernetes/kompose/convert.go | 79 ++++++++++++++++++++--------- utils/unarchive.go | 11 +++- utils/utils.go | 8 +-- utils/walker/git.go | 4 +- 6 files changed, 91 insertions(+), 35 deletions(-) diff --git a/files/tests/sanitization_test.go b/files/tests/sanitization_test.go index a92a1784..9f687cdd 100644 --- a/files/tests/sanitization_test.go +++ b/files/tests/sanitization_test.go @@ -146,9 +146,17 @@ func TestSanitizeFile(t *testing.T) { ".zip": true, } - tempDir, _ := os.MkdirTemp(" ", "temp-tests") - defer os.RemoveAll(tempDir) - // tempDir := "./temp" + // Create a temporary directory for the tests and handle potential errors. + tempDir, err := os.MkdirTemp(" ", "temp-tests") + if err != nil { + t.Fatalf("Failed to create temporary directory: %v", err) + } + // Defer cleanup and log any error that occurs during cleanup. + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Logf("Warning: failed to remove temporary directory %s: %v", tempDir, err) + } + }() for _, tc := range testCases { diff --git a/generators/github/git_repo.go b/generators/github/git_repo.go index b15a961e..41b7ceac 100644 --- a/generators/github/git_repo.go +++ b/generators/github/git_repo.go @@ -47,9 +47,15 @@ func (gr GitRepo) GetContent() (models.Package, error) { br := bufio.NewWriter(fd) defer func() { - br.Flush() - fd.Close() + if err := br.Flush(); err != nil { + fmt.Printf("Warning: failed to flush writer: %v\n", err) + } + + if err := fd.Close(); err != nil { + fmt.Printf("Warning: failed to close file: %v\n", err) + } }() + gw := gitWalker. Owner(owner). Repo(repo). diff --git a/utils/kubernetes/kompose/convert.go b/utils/kubernetes/kompose/convert.go index 5ab3a2f9..5e2d31a8 100644 --- a/utils/kubernetes/kompose/convert.go +++ b/utils/kubernetes/kompose/convert.go @@ -1,6 +1,7 @@ package kompose import ( + "fmt" "os" "path/filepath" "strconv" @@ -42,52 +43,93 @@ func Convert(dockerCompose DockerComposeFile) (string, error) { tempFilePath := filepath.Join(mesheryDir, "temp.data") resultFilePath := filepath.Join(mesheryDir, "result.yaml") + // Create the temp file if err := utils.CreateFile(dockerCompose, "temp.data", mesheryDir); err != nil { return "", ErrCvrtKompose(err) } - defer func() { - os.Remove(tempFilePath) - os.Remove(resultFilePath) - }() + // Format Docker Compose file for Kompose + err = formatComposeFile(&dockerCompose) + if err != nil { + return "", ErrCvrtKompose(err) + } - formatComposeFile(&dockerCompose) + // Check version compatibility err = versionCheck(dockerCompose) if err != nil { return "", ErrCvrtKompose(err) } + // Initialize Kompose client komposeClient, err := client.NewClient() if err != nil { return "", ErrCvrtKompose(err) } + // Set up Convert options ConvertOpt := client.ConvertOptions{ InputFiles: []string{tempFilePath}, OutFile: resultFilePath, GenerateNetworkPolicies: true, } + // Convert using Kompose client _, err = komposeClient.Convert(ConvertOpt) if err != nil { return "", ErrCvrtKompose(err) } + // Read the result file result, err := os.ReadFile(resultFilePath) if err != nil { return "", ErrCvrtKompose(err) } + // Clean up temporary files + cleanupErr := cleanup(tempFilePath, resultFilePath) + if cleanupErr != nil { + return "", cleanupErr + } + return string(result), nil } -type composeFile struct { - Version string `yaml:"version,omitempty"` +// cleanup removes temporary files +func cleanup(tempFilePath, resultFilePath string) error { + // Try to remove tempFilePath + if err := os.Remove(tempFilePath); err != nil { + return fmt.Errorf("failed to remove temp file %s: %w", tempFilePath, err) + } + + // Try to remove resultFilePath + if err := os.Remove(resultFilePath); err != nil { + return fmt.Errorf("failed to remove result file %s: %w", resultFilePath, err) + } + + return nil // No errors +} + +// formatComposeFile takes in a pointer to the compose file byte array and formats it +// so that it is compatible with Kompose. It expects a validated Docker Compose file. +func formatComposeFile(yamlManifest *DockerComposeFile) error { + data := composeFile{} + err := yaml.Unmarshal(*yamlManifest, &data) + if err != nil { + return fmt.Errorf("failed to unmarshal compose file: %w", err) + } + + // Marshal it again to ensure it is in the correct format for Kompose + out, err := yaml.Marshal(data) + if err != nil { + return fmt.Errorf("failed to marshal compose file: %w", err) + } + + *yamlManifest = out + return nil } -// checks if the version is compatible with `kompose` -// expects a valid docker compose yaml -// error = nil means it is compatible +// versionCheck checks if the version in the Docker Compose file is compatible with Kompose. +// It expects a valid Docker Compose YAML and returns an error if the version is not supported. func versionCheck(dc DockerComposeFile) error { cf := composeFile{} err := yaml.Unmarshal(dc, &cf) @@ -108,18 +150,7 @@ func versionCheck(dc DockerComposeFile) error { return nil } -// formatComposeFile takes in a pointer to the compose file byte array and formats it so that it is compatible with `Kompose` -// it expects a validated docker compose file and does not validate -func formatComposeFile(yamlManifest *DockerComposeFile) { - data := composeFile{} - err := yaml.Unmarshal(*yamlManifest, &data) - if err != nil { - return - } - // so that "3.3" and 3.3 are treated differently by `Kompose` - out, err := yaml.Marshal(data) - if err != nil { - return - } - *yamlManifest = out +// composeFile represents the structure of the Docker Compose file version. +type composeFile struct { + Version string `yaml:"version,omitempty"` } diff --git a/utils/unarchive.go b/utils/unarchive.go index 367c4340..10a0592d 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -4,6 +4,7 @@ import ( "archive/tar" "archive/zip" "compress/gzip" + "fmt" "io" "net/http" "os" @@ -149,6 +150,7 @@ func ExtractTarGz(path, downloadfilePath string) error { return ErrExtractTarXZ(err, path) } case tar.TypeReg: + // Ensure that the directory for the file exists _ = os.MkdirAll(filepath.Join(path, filepath.Dir(header.Name)), 0755) var outFile *os.File outFile, err = os.Create(filepath.Join(path, header.Name)) @@ -156,15 +158,20 @@ func ExtractTarGz(path, downloadfilePath string) error { return ErrExtractTarXZ(err, path) } if _, err = io.Copy(outFile, tarReader); err != nil { + outFile.Close() return ErrExtractTarXZ(err, path) } - outFile.Close() + + // Close the file and check for errors + if err := outFile.Close(); err != nil { + return fmt.Errorf("failed to close output file %s: %w", header.Name, err) + } default: return ErrExtractTarXZ(err, path) } - } + return nil } diff --git a/utils/utils.go b/utils/utils.go index d513bcb7..8f1e72f6 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -145,13 +145,15 @@ func CreateFile(contents []byte, filename string, location string) error { return err } + // Write contents to the file if _, err = fd.Write(contents); err != nil { - fd.Close() + fd.Close() // Close the file before returning the error return err } - if err = fd.Close(); err != nil { - return err + // Check error while closing the file + if err := fd.Close(); err != nil { + return fmt.Errorf("failed to close file descriptor: %w", err) } return nil diff --git a/utils/walker/git.go b/utils/walker/git.go index dd7dbb01..3223cb94 100644 --- a/utils/walker/git.go +++ b/utils/walker/git.go @@ -144,7 +144,9 @@ func clonewalk(g *Git) error { var wg sync.WaitGroup defer func() { wg.Wait() - os.RemoveAll(path) + if err := os.RemoveAll(path); err != nil { + fmt.Println(fmt.Errorf("failed to remove path %s: %w", path, err)) + } }() var err error cloneOptions := &git.CloneOptions{ From eefc2166a1f46aba1b5a98f39192f6c582e91325 Mon Sep 17 00:00:00 2001 From: SHIGRAF SALIK <140247389+ShigrafS@users.noreply.github.com> Date: Tue, 9 Sep 2025 15:05:24 +0000 Subject: [PATCH 2/7] fix: switched to meshkit loggers and addressed other review comments Signed-off-by: SHIGRAF SALIK <140247389+ShigrafS@users.noreply.github.com> --- generators/github/git_repo.go | 13 +++++++--- utils/unarchive.go | 48 +++++++++++++++++++++-------------- utils/utils.go | 1 - 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/generators/github/git_repo.go b/generators/github/git_repo.go index 41b7ceac..7821c11f 100644 --- a/generators/github/git_repo.go +++ b/generators/github/git_repo.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/meshery/meshkit/generators/models" + "github.com/meshery/meshkit/logger" "github.com/meshery/meshkit/utils" "github.com/meshery/meshkit/utils/helm" "github.com/meshery/meshkit/utils/walker" @@ -16,7 +17,7 @@ import ( type GitRepo struct { // - URL *url.URL + URL *url.URL PackageName string } @@ -24,6 +25,12 @@ type GitRepo struct { // 2. Unzipped/unarchived File type func (gr GitRepo) GetContent() (models.Package, error) { + log, err := logger.New("generators-github", logger.Options{}) + if err != nil { + // If logger fails to initialize, we can't proceed without logging capabilities. + // Alternatively, one could fall back to fmt.Println and continue. + return nil, err + } gitWalker := walker.NewGit() owner, repo, branch, root, err := gr.extractRepoDetailsFromSourceURL() @@ -48,11 +55,11 @@ func (gr GitRepo) GetContent() (models.Package, error) { defer func() { if err := br.Flush(); err != nil { - fmt.Printf("Warning: failed to flush writer: %v\n", err) + log.Warnf("failed to flush writer: %v", err) } if err := fd.Close(); err != nil { - fmt.Printf("Warning: failed to close file: %v\n", err) + log.Warnf("failed to close file: %v", err) } }() diff --git a/utils/unarchive.go b/utils/unarchive.go index 10a0592d..99674fc9 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -73,24 +73,23 @@ func readData(name string) (buffer []byte, err error) { func ExtractZip(path, artifactPath string) error { zipReader, err := zip.OpenReader(artifactPath) + if err != nil { + return ErrExtractZip(err, path) + } defer func() { _ = zipReader.Close() }() - if err != nil { - return ErrExtractZip(err, path) - } buffer := make([]byte, 1<<4) for _, file := range zipReader.File { fd, err := file.Open() - defer func() { - _ = fd.Close() - }() - if err != nil { return ErrExtractZip(err, path) } + defer func() { + _ = fd.Close() + }() filePath := filepath.Join(path, file.Name) @@ -106,16 +105,16 @@ func ExtractZip(path, artifactPath string) error { } _, err = io.CopyBuffer(openedFile, fd, buffer) if err != nil { + // We need to close the file, but the error from CopyBuffer is more important. + _ = openedFile.Close() + return ErrExtractZip(err, path) + } + if err := openedFile.Close(); err != nil { return ErrExtractZip(err, path) } - defer func() { - _ = openedFile.Close() - }() } - } return nil - } func ExtractTarGz(path, downloadfilePath string) error { @@ -123,14 +122,13 @@ func ExtractTarGz(path, downloadfilePath string) error { if err != nil { return ErrReadFile(err, downloadfilePath) } + defer gzipStream.Close() + uncompressedStream, err := gzip.NewReader(gzipStream) if err != nil { return ErrExtractTarXZ(err, path) } - defer func() { - _ = uncompressedStream.Close() - _ = gzipStream.Close() - }() + defer uncompressedStream.Close() tarReader := tar.NewReader(uncompressedStream) @@ -158,23 +156,35 @@ func ExtractTarGz(path, downloadfilePath string) error { return ErrExtractTarXZ(err, path) } if _, err = io.Copy(outFile, tarReader); err != nil { - outFile.Close() + // The subsequent Close() call will handle closing the file. + // Return the more critical I/O error immediately. + _ = outFile.Close() // Attempt to close, but ignore error as we are returning the copy error. return ErrExtractTarXZ(err, path) } // Close the file and check for errors if err := outFile.Close(); err != nil { - return fmt.Errorf("failed to close output file %s: %w", header.Name, err) + return ErrFileClose(err, header.Name) } default: - return ErrExtractTarXZ(err, path) + return ErrUnsupportedTarHeaderType(header.Typeflag, header.Name) } } return nil } +// ErrFileClose creates a standardized error for file closing failures. +func ErrFileClose(err error, filename string) error { + return fmt.Errorf("failed to close file %s: %w", filename, err) +} + +// ErrUnsupportedTarHeaderType creates an error for unsupported tar header types. +func ErrUnsupportedTarHeaderType(typeflag byte, name string) error { + return fmt.Errorf("unsupported tar header type %v for file %s", typeflag, name) +} + func ProcessContent(filePath string, f func(path string) error) error { pathInfo, err := os.Stat(filePath) if err != nil { diff --git a/utils/utils.go b/utils/utils.go index 8f1e72f6..f31e91eb 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -145,7 +145,6 @@ func CreateFile(contents []byte, filename string, location string) error { return err } - // Write contents to the file if _, err = fd.Write(contents); err != nil { fd.Close() // Close the file before returning the error return err From 67c1f36102329ffae79f2e71c4dbad38d1f2a473 Mon Sep 17 00:00:00 2001 From: SHIGRAF SALIK <140247389+ShigrafS@users.noreply.github.com> Date: Tue, 9 Sep 2025 16:32:05 +0000 Subject: [PATCH 3/7] fix: switched to meshkit logger and other fixes Signed-off-by: SHIGRAF SALIK <140247389+ShigrafS@users.noreply.github.com> --- utils/cue.go | 2 +- utils/utils.go | 28 +++++++++++++++++++--------- utils/walker/git.go | 24 +++++++++++++++--------- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/utils/cue.go b/utils/cue.go index 229edfb7..0cd1bce9 100644 --- a/utils/cue.go +++ b/utils/cue.go @@ -111,7 +111,7 @@ func Lookup(rootVal cue.Value, path string) (cue.Value, error) { return res, ErrCueLookup(res.Err()) } if !res.Exists() { - return res, ErrCueLookup(fmt.Errorf("Could not find the value at the path: %s", path)) + return res, ErrCueLookup(fmt.Errorf("could not find the value at the path: %s", path)) } return res.Value(), nil diff --git a/utils/utils.go b/utils/utils.go index f31e91eb..859c9349 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -136,23 +136,33 @@ func GetHome() string { return usr.HomeDir } -// CreateFile creates a file with the given content on the given location with -// the given filename +// CreateFile creates a file with the given content at the specified location. func CreateFile(contents []byte, filename string, location string) error { - // Create file in -rw-r--r-- mode - fd, err := os.OpenFile(filepath.Join(location, filename), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + log, err := logger.New("utils", logger.Options{}) if err != nil { + // Cannot create logger, return the underlying error. return err } + filePath := filepath.Join(location, filename) + fd, err := os.Create(filePath) + if err != nil { + return err // Or a meshkit-style error wrapper + } + if _, err = fd.Write(contents); err != nil { - fd.Close() // Close the file before returning the error + // Attempt to close the file descriptor, but prioritize the original write error. + // Log the closing error if it occurs, so the information is not lost. + if closeErr := fd.Close(); closeErr != nil { + log.Warn(fmt.Errorf("failed to close file descriptor for %s after a write error: %w", filename, closeErr)) + } return err } - // Check error while closing the file + // Check for an error while closing the file. if err := fd.Close(); err != nil { - return fmt.Errorf("failed to close file descriptor: %w", err) + // Return a standardized meshkit-style error. + return ErrFileClose(err, filename) } return nil @@ -890,8 +900,8 @@ func TruncateErrorMessage(err error, wordLimit int) error { words := strings.Fields(err.Error()) if len(words) > wordLimit { words = words[:wordLimit] - return fmt.Errorf("%s...", strings.Join(words, " ")) + return fmt.Errorf("%s", strings.Join(words, " ")) } return err -} \ No newline at end of file +} diff --git a/utils/walker/git.go b/utils/walker/git.go index 3223cb94..3d806df0 100644 --- a/utils/walker/git.go +++ b/utils/walker/git.go @@ -14,6 +14,7 @@ import ( "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" + "github.com/meshery/meshkit/logger" ) // Git represents the Git Walker @@ -136,6 +137,11 @@ func (g *Git) RegisterDirInterceptor(i DirInterceptor) *Git { return g } func clonewalk(g *Git) error { + log, err := logger.New("walker-git", logger.Options{}) + if err != nil { + return err + } + if g.maxFileSizeInBytes == 0 { return ErrInvalidSizeFile(errors.New("max file size passed as 0. Will not read any file")) } @@ -145,10 +151,9 @@ func clonewalk(g *Git) error { defer func() { wg.Wait() if err := os.RemoveAll(path); err != nil { - fmt.Println(fmt.Errorf("failed to remove path %s: %w", path, err)) + log.Error(fmt.Errorf("failed to remove path %s: %w", path, err)) } }() - var err error cloneOptions := &git.CloneOptions{ URL: fmt.Sprintf("%s/%s/%s", g.baseURL, g.owner, g.repo), SingleBranch: true, @@ -177,7 +182,7 @@ func clonewalk(g *Git) error { } if !info.IsDir() { - err = g.readFile(info, rootPath) + err = g.readFile(info, rootPath, log) if err != nil { return ErrCloningRepo(err) } @@ -199,7 +204,7 @@ func clonewalk(g *Git) error { if err != nil { return errInfo } - return g.readFile(f, path) + return g.readFile(f, path, log) }) if err != nil { return ErrCloningRepo(err) @@ -233,7 +238,7 @@ func clonewalk(g *Git) error { Path: fPath, }) if err != nil { - fmt.Println(err.Error()) + log.Error(err) } }(name, fPath, f.Name()) continue @@ -241,16 +246,16 @@ func clonewalk(g *Git) error { if f.IsDir() { continue } - err := g.readFile(f, fPath) + err := g.readFile(f, fPath, log) if err != nil { - fmt.Println(err.Error()) + log.Error(err) } } return nil } -func (g *Git) readFile(f fs.FileInfo, path string) error { +func (g *Git) readFile(f fs.FileInfo, path string, log logger.Handler) error { if f.Size() > g.maxFileSizeInBytes { return ErrInvalidSizeFile(errors.New("File exceeding size limit")) } @@ -258,6 +263,7 @@ func (g *Git) readFile(f fs.FileInfo, path string) error { if err != nil { return err } + defer filename.Close() content, err := io.ReadAll(filename) if err != nil { return err @@ -268,7 +274,7 @@ func (g *Git) readFile(f fs.FileInfo, path string) error { Content: string(content), }) if err != nil { - fmt.Println("Could not intercept the file ", f.Name()) + log.Warnf("Could not intercept the file %s: %v", f.Name(), err) } return err } From a499769d860bbb73b77de5141b6a9f07f765d0f9 Mon Sep 17 00:00:00 2001 From: ShigrafS Date: Mon, 15 Sep 2025 19:28:25 +0530 Subject: [PATCH 4/7] feat: added new error Signed-off-by: ShigrafS --- utils/error.go | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/utils/error.go b/utils/error.go index 41e2f7d9..242e2625 100644 --- a/utils/error.go +++ b/utils/error.go @@ -36,16 +36,17 @@ var ( ErrCreateDirCode = "meshkit-11182" // ErrDecodeYamlCode represents the error which is generated when yaml // decode process fails - ErrDecodeYamlCode = "meshkit-11183" - ErrExtractTarXZCode = "meshkit-11184" - ErrExtractZipCode = "meshkit-11185" - ErrReadDirCode = "meshkit-11186" - ErrInvalidSchemaVersionCode = "meshkit-11273" - ErrFileWalkDirCode = "meshkit-11274" - ErrRelPathCode = "meshkit-11275" - ErrCopyFileCode = "meshkit-11276" - ErrCloseFileCode = "meshkit-11277" - ErrCompressToTarGZCode = "meshkit-11248" + ErrDecodeYamlCode = "meshkit-11183" + ErrExtractTarXZCode = "meshkit-11184" + ErrExtractZipCode = "meshkit-11185" + ErrReadDirCode = "meshkit-11186" + ErrInvalidSchemaVersionCode = "meshkit-11273" + ErrFileWalkDirCode = "meshkit-11274" + ErrRelPathCode = "meshkit-11275" + ErrCopyFileCode = "meshkit-11276" + ErrCloseFileCode = "meshkit-11277" + ErrCompressToTarGZCode = "meshkit-11248" + ErrUnsupportedTarHeaderTypeCode = "meshkit-11282" ErrConvertToByteCode = "meshkit-11187" @@ -64,9 +65,9 @@ func ErrInvalidConstructSchemaVersion(contruct string, version string, supported ErrInvalidSchemaVersionCode, errors.Critical, []string{"Invalid schema version " + version}, - []string{"The `schemaVersion` key is either empty or has an incorrect value."}, + []string{"The schemaVersion key is either empty or has an incorrect value."}, []string{fmt.Sprintf("The schema is not of type '%s'", contruct)}, - []string{"Verify that `schemaVersion` key should be '%s'", supportedVersion}, + []string{"Verify that schemaVersion key should be '%s'", supportedVersion}, ) } @@ -75,17 +76,17 @@ var ( ErrUnmarshalTypeCode, errors.Alert, []string{"Invalid extraction type"}, - []string{"The file type to be extracted is neither `tar.gz` nor `zip`."}, - []string{"Invalid object format. The file is not of type `zip` or `tar.gz`."}, - []string{"Make sure to check that the file type is `zip` or `tar.gz`."}, + []string{"The file type to be extracted is neither tar.gz nor zip."}, + []string{"Invalid object format. The file is not of type zip or tar.gz."}, + []string{"Make sure to check that the file type is zip or tar.gz."}, ) ErrInvalidSchemaVersion = errors.New( ErrInvalidSchemaVersionCode, errors.Alert, []string{"Invalid schema version"}, - []string{"The `schemaVersion` key is either empty or has an incorrect value."}, + []string{"The schemaVersion key is either empty or has an incorrect value."}, []string{"The schema is not of type 'relationship', 'component', 'model' , 'policy'."}, - []string{"Verify that `schemaVersion` key should be either `relationships.meshery.io`, `component.meshery.io`, `model.meshery.io` or `policy.meshery.io`."}, + []string{"Verify that schemaVersion key should be either relationships.meshery.io, component.meshery.io, model.meshery.io or policy.meshery.io."}, ) ) @@ -272,3 +273,14 @@ func ErrGoogleSheetSRV(err error) error { func ErrWritingIntoFile(err error, obj string) error { return errors.New(ErrWritingIntoFileCode, errors.Alert, []string{fmt.Sprintf("failed to write into file %s", obj)}, []string{err.Error()}, []string{"Insufficient permissions to write into file", "file might be corrupted"}, []string{"check if sufficient permissions are givent to the file", "check if the file is corrupted"}) } + +func ErrUnsupportedTarHeaderType(typeflag byte, name string) error { + return errors.New( + ErrUnsupportedTarHeaderTypeCode, + errors.Alert, + []string{"Unsupported tar header type encountered during extraction"}, + []string{fmt.Sprintf("The tar archive contains an entry '%s' with an unsupported type flag '%c'.", name, typeflag)}, + []string{"The tar archive is malformed or contains an entry type that this utility cannot handle (e.g., special device files)."}, + []string{"Ensure the tar archive was created with standard file types (directories, regular files, symlinks).", "Check the integrity of the archive file."}, + ) +} From 41f3586d857e43f4bcd7c7ce797a4e12be0eb693 Mon Sep 17 00:00:00 2001 From: ShigrafS Date: Mon, 15 Sep 2025 20:31:38 +0530 Subject: [PATCH 5/7] fix: addressed review comments Signed-off-by: ShigrafS --- utils/unarchive.go | 62 ++++++++++++++++++++++------------------------ utils/utils.go | 10 ++++---- 2 files changed, 34 insertions(+), 38 deletions(-) diff --git a/utils/unarchive.go b/utils/unarchive.go index 99674fc9..612215ec 100644 --- a/utils/unarchive.go +++ b/utils/unarchive.go @@ -10,6 +10,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/meshery/meshkit/utils" ) func IsTarGz(name string) bool { @@ -117,18 +119,24 @@ func ExtractZip(path, artifactPath string) error { return nil } -func ExtractTarGz(path, downloadfilePath string) error { +func ExtractTarGz(path, downloadfilePath string) (err error) { // Note: named return `err` is added gzipStream, err := os.Open(downloadfilePath) if err != nil { - return ErrReadFile(err, downloadfilePath) + return utils.ErrReadFile(err, downloadfilePath) // Assuming ErrReadFile is in the utils package } - defer gzipStream.Close() + // CORRECTED: defer now handles the error from Close() + defer func() { + if closeErr := gzipStream.Close(); closeErr != nil && err == nil { + err = utils.ErrCloseFile(closeErr) + } + }() uncompressedStream, err := gzip.NewReader(gzipStream) if err != nil { - return ErrExtractTarXZ(err, path) + return utils.ErrExtractTarXZ(err, path) } - defer uncompressedStream.Close() + defer uncompressedStream.Close() // Note: This Close() can also fail and should ideally be handled. + // For now, we are addressing the reviewer's direct comment. tarReader := tar.NewReader(uncompressedStream) @@ -138,53 +146,41 @@ func ExtractTarGz(path, downloadfilePath string) error { if err == io.EOF { break } - if err != nil { - return ErrExtractTarXZ(err, path) + return utils.ErrExtractTarXZ(err, path) + } + + targetPath := filepath.Join(path, header.Name) + if !strings.HasPrefix(targetPath, filepath.Clean(path)+string(os.PathSeparator)) { + return fmt.Errorf("tar entry is trying to escape the destination directory: %s", header.Name) } + switch header.Typeflag { case tar.TypeDir: - if err = os.MkdirAll(filepath.Join(path, header.Name), 0755); err != nil { - return ErrExtractTarXZ(err, path) + if err := os.MkdirAll(targetPath, 0755); err != nil { + return utils.ErrExtractTarXZ(err, path) // Re-using existing error for simplicity } case tar.TypeReg: - // Ensure that the directory for the file exists - _ = os.MkdirAll(filepath.Join(path, filepath.Dir(header.Name)), 0755) - var outFile *os.File - outFile, err = os.Create(filepath.Join(path, header.Name)) + outFile, err := os.Create(targetPath) if err != nil { - return ErrExtractTarXZ(err, path) + return utils.ErrExtractTarXZ(err, path) } if _, err = io.Copy(outFile, tarReader); err != nil { - // The subsequent Close() call will handle closing the file. - // Return the more critical I/O error immediately. - _ = outFile.Close() // Attempt to close, but ignore error as we are returning the copy error. - return ErrExtractTarXZ(err, path) + _ = outFile.Close() // Attempt to close, but return the more important Copy error + return utils.ErrExtractTarXZ(err, path) } - - // Close the file and check for errors + // CORRECTED: Using the standard Meshkit error function if err := outFile.Close(); err != nil { - return ErrFileClose(err, header.Name) + return utils.ErrCloseFile(err) } default: - return ErrUnsupportedTarHeaderType(header.Typeflag, header.Name) + return utils.ErrUnsupportedTarHeaderType(header.Typeflag, header.Name) } } - return nil } -// ErrFileClose creates a standardized error for file closing failures. -func ErrFileClose(err error, filename string) error { - return fmt.Errorf("failed to close file %s: %w", filename, err) -} - -// ErrUnsupportedTarHeaderType creates an error for unsupported tar header types. -func ErrUnsupportedTarHeaderType(typeflag byte, name string) error { - return fmt.Errorf("unsupported tar header type %v for file %s", typeflag, name) -} - func ProcessContent(filePath string, f func(path string) error) error { pathInfo, err := os.Stat(filePath) if err != nil { diff --git a/utils/utils.go b/utils/utils.go index 859c9349..4bd35ac2 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -25,6 +25,7 @@ import ( "github.com/meshery/meshkit/logger" "github.com/meshery/meshkit/models/meshmodel/entity" + "github.com/meshery/meshkit/utils" log "github.com/sirupsen/logrus" "gopkg.in/yaml.v3" @@ -147,7 +148,7 @@ func CreateFile(contents []byte, filename string, location string) error { filePath := filepath.Join(location, filename) fd, err := os.Create(filePath) if err != nil { - return err // Or a meshkit-style error wrapper + return err // Or a meshkit-style error wrapper for Create } if _, err = fd.Write(contents); err != nil { @@ -156,13 +157,12 @@ func CreateFile(contents []byte, filename string, location string) error { if closeErr := fd.Close(); closeErr != nil { log.Warn(fmt.Errorf("failed to close file descriptor for %s after a write error: %w", filename, closeErr)) } - return err + return err // Return the original, more important write error } - // Check for an error while closing the file. + // CORRECTED: Check for an error while closing the file using the standard Meshkit error. if err := fd.Close(); err != nil { - // Return a standardized meshkit-style error. - return ErrFileClose(err, filename) + return utils.ErrCloseFile(err) } return nil From ac17449b298d70a7b7ec8558ef543394da453843 Mon Sep 17 00:00:00 2001 From: ShigrafS Date: Tue, 16 Sep 2025 18:51:43 +0530 Subject: [PATCH 6/7] refactor: revert to defer Signed-off-by: ShigrafS --- utils/kubernetes/kompose/convert.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/utils/kubernetes/kompose/convert.go b/utils/kubernetes/kompose/convert.go index 5e2d31a8..0fccec7c 100644 --- a/utils/kubernetes/kompose/convert.go +++ b/utils/kubernetes/kompose/convert.go @@ -43,6 +43,18 @@ func Convert(dockerCompose DockerComposeFile) (string, error) { tempFilePath := filepath.Join(mesheryDir, "temp.data") resultFilePath := filepath.Join(mesheryDir, "result.yaml") + // Defer cleanup to ensure temp files are removed even on error. + defer func() { + // We log cleanup errors but do not return them, as the primary function error is more important. + if err := os.Remove(tempFilePath); err != nil && !os.IsNotExist(err) { + // A logger should be used here to warn about the failed cleanup. + // e.g., log.Warnf("failed to remove temporary file %s: %v", tempFilePath, err) + } + if err := os.Remove(resultFilePath); err != nil && !os.IsNotExist(err) { + // e.g., log.Warnf("failed to remove result file %s: %v", resultFilePath, err) + } + }() + // Create the temp file if err := utils.CreateFile(dockerCompose, "temp.data", mesheryDir); err != nil { return "", ErrCvrtKompose(err) @@ -85,12 +97,6 @@ func Convert(dockerCompose DockerComposeFile) (string, error) { return "", ErrCvrtKompose(err) } - // Clean up temporary files - cleanupErr := cleanup(tempFilePath, resultFilePath) - if cleanupErr != nil { - return "", cleanupErr - } - return string(result), nil } From 4a8909693319d5a7a3abab6cf8b180b90084a906 Mon Sep 17 00:00:00 2001 From: SHIGRAF SALIK <140247389+ShigrafS@users.noreply.github.com> Date: Sun, 21 Sep 2025 17:04:34 +0000 Subject: [PATCH 7/7] fix: adressed review comments Signed-off-by: SHIGRAF SALIK <140247389+ShigrafS@users.noreply.github.com> --- utils/kubernetes/kompose/convert.go | 93 +++++++++++++---------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/utils/kubernetes/kompose/convert.go b/utils/kubernetes/kompose/convert.go index 0fccec7c..6d1cdcc8 100644 --- a/utils/kubernetes/kompose/convert.go +++ b/utils/kubernetes/kompose/convert.go @@ -7,15 +7,16 @@ import ( "strconv" "github.com/kubernetes/kompose/client" + "github.com/meshery/meshkit/logger" "github.com/meshery/meshkit/utils" "gopkg.in/yaml.v3" ) const DefaultDockerComposeSchemaURL = "https://raw.githubusercontent.com/compose-spec/compose-spec/master/schema/compose-spec.json" -// Checks whether the given manifest is a valid docker-compose file. -// schemaURL is assigned a default url if not specified -// error will be 'nil' if it is a valid docker compose file +// IsManifestADockerCompose checks whether the given manifest is a valid docker-compose file. +// schemaURL is assigned a default url if not specified. +// The error will be 'nil' if it is a valid docker compose file. func IsManifestADockerCompose(manifest []byte, schemaURL string) error { if schemaURL == "" { schemaURL = DefaultDockerComposeSchemaURL @@ -29,69 +30,72 @@ func IsManifestADockerCompose(manifest []byte, schemaURL string) error { return err } -// converts a given docker-compose file into kubernetes manifests -// expects a validated docker-compose file +// Convert converts a given docker-compose file into kubernetes manifests. +// It expects a validated docker-compose file. func Convert(dockerCompose DockerComposeFile) (string, error) { - // Get user's home directory + // Initialize a logger for this function's scope. + log, err := logger.New("kompose-convert", logger.Options{}) + if err != nil { + // If the logger fails, print an error and continue without logging. + fmt.Printf("Logger initialization failed: %v\n", err) + } + + // Get user's home directory. homeDir, err := os.UserHomeDir() if err != nil { return "", ErrCvrtKompose(err) } - // Construct path to .meshery directory + // Construct path to .meshery directory. mesheryDir := filepath.Join(homeDir, ".meshery") tempFilePath := filepath.Join(mesheryDir, "temp.data") resultFilePath := filepath.Join(mesheryDir, "result.yaml") - // Defer cleanup to ensure temp files are removed even on error. + // Defer cleanup to ensure temp files are removed, even on error. defer func() { - // We log cleanup errors but do not return them, as the primary function error is more important. + // Log cleanup errors as warnings, as the primary function error is more important. if err := os.Remove(tempFilePath); err != nil && !os.IsNotExist(err) { - // A logger should be used here to warn about the failed cleanup. - // e.g., log.Warnf("failed to remove temporary file %s: %v", tempFilePath, err) + log.Warnf("failed to remove temporary file %s: %v", tempFilePath, err) } if err := os.Remove(resultFilePath); err != nil && !os.IsNotExist(err) { - // e.g., log.Warnf("failed to remove result file %s: %v", resultFilePath, err) + log.Warnf("failed to remove result file %s: %v", resultFilePath, err) } }() - // Create the temp file - if err := utils.CreateFile(dockerCompose, "temp.data", mesheryDir); err != nil { + // Format the Docker Compose file content to be compatible with Kompose. + if err := formatComposeFile(&dockerCompose); err != nil { return "", ErrCvrtKompose(err) } - // Format Docker Compose file for Kompose - err = formatComposeFile(&dockerCompose) - if err != nil { + // Create a temporary file with the formatted compose data. + if err := utils.CreateFile(dockerCompose, "temp.data", mesheryDir); err != nil { return "", ErrCvrtKompose(err) } - // Check version compatibility - err = versionCheck(dockerCompose) - if err != nil { + // Check for version compatibility. + if err := versionCheck(dockerCompose); err != nil { return "", ErrCvrtKompose(err) } - // Initialize Kompose client + // Initialize the Kompose client. komposeClient, err := client.NewClient() if err != nil { return "", ErrCvrtKompose(err) } - // Set up Convert options + // Set up the conversion options. ConvertOpt := client.ConvertOptions{ InputFiles: []string{tempFilePath}, OutFile: resultFilePath, GenerateNetworkPolicies: true, } - // Convert using Kompose client - _, err = komposeClient.Convert(ConvertOpt) - if err != nil { + // Perform the conversion using the Kompose client. + if _, err = komposeClient.Convert(ConvertOpt); err != nil { return "", ErrCvrtKompose(err) } - // Read the result file + // Read the resulting Kubernetes manifest file. result, err := os.ReadFile(resultFilePath) if err != nil { return "", ErrCvrtKompose(err) @@ -100,31 +104,16 @@ func Convert(dockerCompose DockerComposeFile) (string, error) { return string(result), nil } -// cleanup removes temporary files -func cleanup(tempFilePath, resultFilePath string) error { - // Try to remove tempFilePath - if err := os.Remove(tempFilePath); err != nil { - return fmt.Errorf("failed to remove temp file %s: %w", tempFilePath, err) - } - - // Try to remove resultFilePath - if err := os.Remove(resultFilePath); err != nil { - return fmt.Errorf("failed to remove result file %s: %w", resultFilePath, err) - } - - return nil // No errors -} - -// formatComposeFile takes in a pointer to the compose file byte array and formats it -// so that it is compatible with Kompose. It expects a validated Docker Compose file. +// formatComposeFile takes a pointer to the compose file byte array and formats it +// to be compatible with Kompose by unmarshalling and marshalling it. +// It expects a validated Docker Compose file. func formatComposeFile(yamlManifest *DockerComposeFile) error { data := composeFile{} - err := yaml.Unmarshal(*yamlManifest, &data) - if err != nil { + if err := yaml.Unmarshal(*yamlManifest, &data); err != nil { return fmt.Errorf("failed to unmarshal compose file: %w", err) } - // Marshal it again to ensure it is in the correct format for Kompose + // Marshal it again to ensure it's in the correct format for Kompose. out, err := yaml.Marshal(data) if err != nil { return fmt.Errorf("failed to marshal compose file: %w", err) @@ -148,15 +137,17 @@ func versionCheck(dc DockerComposeFile) error { versionFloatVal, err := strconv.ParseFloat(cf.Version, 64) if err != nil { return utils.ErrExpectedTypeMismatch(err, "float") - } else { - if versionFloatVal > 3.9 { - return ErrIncompatibleVersion() - } } + + if versionFloatVal > 3.9 { + return ErrIncompatibleVersion() + } + return nil } -// composeFile represents the structure of the Docker Compose file version. +// composeFile represents the structure needed to extract the version +// from a Docker Compose file. type composeFile struct { Version string `yaml:"version,omitempty"` }