From 8810b98890303a009acaae3a622211319a4baa67 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Thu, 1 Aug 2024 18:32:30 +0300 Subject: [PATCH 1/3] fix: Add binary plugin url support Signed-off-by: Suleiman Dibirov --- .../plugin/installer/http_installer_test.go | 40 +++++++++---- internal/plugin/installer/installer.go | 58 +++++++++++-------- internal/plugin/installer/installer_test.go | 10 +++- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/internal/plugin/installer/http_installer_test.go b/internal/plugin/installer/http_installer_test.go index be40b1b90..16ff0e305 100644 --- a/internal/plugin/installer/http_installer_test.go +++ b/internal/plugin/installer/http_installer_test.go @@ -27,6 +27,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sort" "strings" "syscall" "testing" @@ -66,22 +67,37 @@ func TestStripName(t *testing.T) { } } -func mockArchiveServer() *httptest.Server { +func mockArchiveServer(extensionToContentType map[string]string) *httptest.Server { + // Extract and sort keys by length in descending order + extensions := make([]string, 0, len(extensionToContentType)) + for ext := range extensionToContentType { + extensions = append(extensions, ext) + } + sort.Slice(extensions, func(i, j int) bool { + return len(extensions[i]) > len(extensions[j]) + }) + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !strings.HasSuffix(r.URL.Path, ".tar.gz") { - w.Header().Add("Content-Type", "text/html") - fmt.Fprintln(w, "broken") - return + for _, ext := range extensions { + contentType := extensionToContentType[ext] + if strings.HasSuffix(r.URL.Path, ext) { + w.Header().Add("Content-Type", contentType) + fmt.Fprintln(w, "test") + return + } } - w.Header().Add("Content-Type", "application/gzip") - fmt.Fprintln(w, "test") + + w.Header().Add("Content-Type", "text/html") + fmt.Fprintln(w, "broken") })) } func TestHTTPInstaller(t *testing.T) { ensure.HelmHome(t) - srv := mockArchiveServer() + srv := mockArchiveServer(map[string]string{ + ".tar.gz": "application/gzip", + }) defer srv.Close() source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" @@ -129,7 +145,9 @@ func TestHTTPInstaller(t *testing.T) { func TestHTTPInstallerNonExistentVersion(t *testing.T) { ensure.HelmHome(t) - srv := mockArchiveServer() + srv := mockArchiveServer(map[string]string{ + ".tar.gz": "application/gzip", + }) defer srv.Close() source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" @@ -161,7 +179,9 @@ func TestHTTPInstallerNonExistentVersion(t *testing.T) { } func TestHTTPInstallerUpdate(t *testing.T) { - srv := mockArchiveServer() + srv := mockArchiveServer(map[string]string{ + ".tar.gz": "application/gzip", + }) defer srv.Close() source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" ensure.HelmHome(t) diff --git a/internal/plugin/installer/installer.go b/internal/plugin/installer/installer.go index a6599c443..c4aeebc35 100644 --- a/internal/plugin/installer/installer.go +++ b/internal/plugin/installer/installer.go @@ -174,41 +174,51 @@ func isLocalReference(source string) bool { // It works by checking whether the source looks like a URL and, if it does, running a // HEAD operation to see if the remote resource is a file that we understand. func isRemoteHTTPArchive(source string) bool { - if strings.HasPrefix(source, "http://") || strings.HasPrefix(source, "https://") { - // First, check if the URL ends with a known archive suffix - // This is more reliable than content-type detection + if !isHTTPURL(source) { + return false + } + + contentType, err := getRemoteContentType(source) + if err != nil { + return false + } + + // Handle octet-stream specially by checking file extension + if contentType == "application/octet-stream" { for suffix := range Extractors { if strings.HasSuffix(source, suffix) { return true } } - // If no suffix match, try HEAD request to check content type - res, err := http.Head(source) - if err != nil { - // If we get an error at the network layer, we can't install it. So - // we return false. - return false - } - - // Next, we look for the content type or content disposition headers to see - // if they have matching extractors. - contentType := res.Header.Get("content-type") - foundSuffix, ok := mediaTypeToExtension(contentType) - if !ok { - // Media type not recognized - return false - } + return false + } - for suffix := range Extractors { - if strings.HasSuffix(foundSuffix, suffix) { - return true - } - } + // Check if we have an extractor for this media type + if suffix, ok := mediaTypeToExtension(contentType); ok { + _, hasExtractor := Extractors[suffix] + return hasExtractor } + return false } +// isHTTPURL checks if the source is an HTTP or HTTPS URL +func isHTTPURL(source string) bool { + return strings.HasPrefix(source, "http://") || strings.HasPrefix(source, "https://") +} + +// getRemoteContentType performs a HEAD request and returns the content-type +func getRemoteContentType(url string) (string, error) { + res, err := http.Head(url) + if err != nil { + return "", err + } + defer res.Body.Close() + + return res.Header.Get("content-type"), nil +} + // isPlugin checks if the directory contains a plugin.yaml file. func isPlugin(dirname string) bool { _, err := os.Stat(filepath.Join(dirname, plugin.PluginFileName)) diff --git a/internal/plugin/installer/installer_test.go b/internal/plugin/installer/installer_test.go index dcd76fe9c..9714e72ce 100644 --- a/internal/plugin/installer/installer_test.go +++ b/internal/plugin/installer/installer_test.go @@ -18,9 +18,13 @@ package installer import "testing" func TestIsRemoteHTTPArchive(t *testing.T) { - srv := mockArchiveServer() + srv := mockArchiveServer(map[string]string{ + ".tar.gz": "application/gzip", + ".binary.tar.gz": "application/octet-stream", + }) defer srv.Close() source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" + binarySource := srv.URL + "/plugins/fake-plugin-0.0.1.binary.tar.gz" if isRemoteHTTPArchive("/not/a/URL") { t.Errorf("Expected non-URL to return false") @@ -44,4 +48,8 @@ func TestIsRemoteHTTPArchive(t *testing.T) { if isRemoteHTTPArchive(source + "-not-an-extension") { t.Error("Expected media type match to fail") } + + if !isRemoteHTTPArchive(binarySource) { + t.Errorf("Expected %q to be a valid archive URL", binarySource) + } } From e2cbf044cc6b7d5bba9b11a39358325e46b06bad Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Sat, 18 Oct 2025 15:01:36 +0300 Subject: [PATCH 2/3] Fixes Signed-off-by: Suleiman Dibirov --- internal/plugin/installer/extractor.go | 4 ++ internal/plugin/installer/gzip.go | 50 +++++++++++++++ internal/plugin/installer/installer.go | 61 +++++++++---------- internal/plugin/installer/oci_installer.go | 2 +- .../plugin/installer/oci_installer_test.go | 2 +- 5 files changed, 84 insertions(+), 35 deletions(-) create mode 100644 internal/plugin/installer/gzip.go diff --git a/internal/plugin/installer/extractor.go b/internal/plugin/installer/extractor.go index 407138197..aadcbfefa 100644 --- a/internal/plugin/installer/extractor.go +++ b/internal/plugin/installer/extractor.go @@ -68,6 +68,10 @@ func NewExtractor(source string) (Extractor, error) { return extractor, nil } } + if strings.HasPrefix(source, "http") && isGzipArchiveFromURL(source) { + return &TarGzExtractor{}, nil + } + return nil, fmt.Errorf("no extractor implemented yet for %s", source) } diff --git a/internal/plugin/installer/gzip.go b/internal/plugin/installer/gzip.go new file mode 100644 index 000000000..d247d542f --- /dev/null +++ b/internal/plugin/installer/gzip.go @@ -0,0 +1,50 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package installer + +import "net/http" + +// isGzipArchive checks if data represents a gzip archive by checking the magic bytes +func isGzipArchive(data []byte) bool { + return len(data) >= 2 && data[0] == 0x1f && data[1] == 0x8b +} + +// isGzipArchiveFromURL checks if a URL points to a gzip archive by reading the magic bytes +func isGzipArchiveFromURL(url string) bool { + // Make a GET request to read the first few bytes + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return false + } + + // Request only the first 512 bytes to check magic bytes + req.Header.Set("Range", "bytes=0-511") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return false + } + defer resp.Body.Close() + + // Read the first few bytes + buf := make([]byte, 2) + n, err := resp.Body.Read(buf) + if err != nil || n < 2 { + return false + } + + return isGzipArchive(buf) +} diff --git a/internal/plugin/installer/installer.go b/internal/plugin/installer/installer.go index c4aeebc35..62654f5ac 100644 --- a/internal/plugin/installer/installer.go +++ b/internal/plugin/installer/installer.go @@ -174,49 +174,44 @@ func isLocalReference(source string) bool { // It works by checking whether the source looks like a URL and, if it does, running a // HEAD operation to see if the remote resource is a file that we understand. func isRemoteHTTPArchive(source string) bool { - if !isHTTPURL(source) { - return false - } - - contentType, err := getRemoteContentType(source) - if err != nil { - return false - } - - // Handle octet-stream specially by checking file extension - if contentType == "application/octet-stream" { + if strings.HasPrefix(source, "http://") || strings.HasPrefix(source, "https://") { + // First, check if the URL ends with a known archive suffix + // This is more reliable than content-type detection for suffix := range Extractors { if strings.HasSuffix(source, suffix) { return true } } - return false - } - - // Check if we have an extractor for this media type - if suffix, ok := mediaTypeToExtension(contentType); ok { - _, hasExtractor := Extractors[suffix] - return hasExtractor - } + // If no suffix match, try HEAD request to check content type + res, err := http.Head(source) + if err != nil { + // If we get an error at the network layer, we can't install it. So + // we return false. + return false + } - return false -} + // Next, we look for the content type or content disposition headers to see + // if they have matching extractors. + contentType := res.Header.Get("content-type") + foundSuffix, ok := mediaTypeToExtension(contentType) + if !ok { + if contentType == "application/octet-stream" && isGzipArchiveFromURL(source) { + // For generic binary content, try to detect the actual file type + // by reading the first few bytes (magic bytes) + return true + } -// isHTTPURL checks if the source is an HTTP or HTTPS URL -func isHTTPURL(source string) bool { - return strings.HasPrefix(source, "http://") || strings.HasPrefix(source, "https://") -} + return false + } -// getRemoteContentType performs a HEAD request and returns the content-type -func getRemoteContentType(url string) (string, error) { - res, err := http.Head(url) - if err != nil { - return "", err + for suffix := range Extractors { + if strings.HasSuffix(foundSuffix, suffix) { + return true + } + } } - defer res.Body.Close() - - return res.Header.Get("content-type"), nil + return false } // isPlugin checks if the directory contains a plugin.yaml file. diff --git a/internal/plugin/installer/oci_installer.go b/internal/plugin/installer/oci_installer.go index afbb42ca5..856ebb5c8 100644 --- a/internal/plugin/installer/oci_installer.go +++ b/internal/plugin/installer/oci_installer.go @@ -128,7 +128,7 @@ func (i *OCIInstaller) Install() error { } // Check if this is a gzip compressed file - if len(i.pluginData) < 2 || i.pluginData[0] != 0x1f || i.pluginData[1] != 0x8b { + if !isGzipArchive(i.pluginData) { return fmt.Errorf("plugin data is not a gzip compressed archive") } diff --git a/internal/plugin/installer/oci_installer_test.go b/internal/plugin/installer/oci_installer_test.go index 1280cf97d..aa58ef2d3 100644 --- a/internal/plugin/installer/oci_installer_test.go +++ b/internal/plugin/installer/oci_installer_test.go @@ -792,7 +792,7 @@ func TestOCIInstaller_Install_ValidationErrors(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test the gzip validation logic that's used in the Install method - if len(tt.layerData) < 2 || tt.layerData[0] != 0x1f || tt.layerData[1] != 0x8b { + if !isGzipArchive(tt.layerData) { // This matches the validation in the Install method if !tt.expectError { t.Error("expected valid gzip data") From e248d394d862a9a858042dffe2be72bada72b918 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Wed, 22 Oct 2025 20:00:20 +0300 Subject: [PATCH 3/3] fixes Signed-off-by: Suleiman Dibirov --- internal/plugin/installer/extractor.go | 11 ++++++-- internal/plugin/installer/gzip.go | 36 +++++++++++++++++++------- internal/plugin/installer/installer.go | 16 +++++++++--- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/internal/plugin/installer/extractor.go b/internal/plugin/installer/extractor.go index aadcbfefa..9865bea40 100644 --- a/internal/plugin/installer/extractor.go +++ b/internal/plugin/installer/extractor.go @@ -68,8 +68,15 @@ func NewExtractor(source string) (Extractor, error) { return extractor, nil } } - if strings.HasPrefix(source, "http") && isGzipArchiveFromURL(source) { - return &TarGzExtractor{}, nil + if strings.HasPrefix(source, "http") { + isGzip, err := isGzipArchiveFromURL(source) + if err != nil { + return nil, err + } + + if isGzip { + return &TarGzExtractor{}, nil + } } return nil, fmt.Errorf("no extractor implemented yet for %s", source) diff --git a/internal/plugin/installer/gzip.go b/internal/plugin/installer/gzip.go index d247d542f..0b69d277b 100644 --- a/internal/plugin/installer/gzip.go +++ b/internal/plugin/installer/gzip.go @@ -15,7 +15,13 @@ limitations under the License. package installer -import "net/http" +import ( + "context" + "fmt" + "io" + "net/http" + "time" +) // isGzipArchive checks if data represents a gzip archive by checking the magic bytes func isGzipArchive(data []byte) bool { @@ -23,11 +29,15 @@ func isGzipArchive(data []byte) bool { } // isGzipArchiveFromURL checks if a URL points to a gzip archive by reading the magic bytes -func isGzipArchiveFromURL(url string) bool { +func isGzipArchiveFromURL(url string) (bool, error) { + // Use a short timeout context to avoid hanging on slow servers + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // Make a GET request to read the first few bytes - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { - return false + return false, err } // Request only the first 512 bytes to check magic bytes @@ -35,16 +45,22 @@ func isGzipArchiveFromURL(url string) bool { resp, err := http.DefaultClient.Do(req) if err != nil { - return false + return false, err } defer resp.Body.Close() - // Read the first few bytes + // Check for valid status codes early + // 206 = Partial Content (range supported) + // 200 = OK (range not supported, full content returned) + if resp.StatusCode != http.StatusPartialContent && resp.StatusCode != http.StatusOK { + return false, fmt.Errorf("unexpected status code %d when checking gzip archive at %s", resp.StatusCode, url) + } + + // Read exactly 2 bytes for gzip magic number check buf := make([]byte, 2) - n, err := resp.Body.Read(buf) - if err != nil || n < 2 { - return false + if _, err := io.ReadAtLeast(resp.Body, buf, len(buf)); err != nil { + return false, fmt.Errorf("failed to read magic bytes from %s: %w", url, err) } - return isGzipArchive(buf) + return isGzipArchive(buf), nil } diff --git a/internal/plugin/installer/installer.go b/internal/plugin/installer/installer.go index 62654f5ac..f279f26b5 100644 --- a/internal/plugin/installer/installer.go +++ b/internal/plugin/installer/installer.go @@ -196,10 +196,18 @@ func isRemoteHTTPArchive(source string) bool { contentType := res.Header.Get("content-type") foundSuffix, ok := mediaTypeToExtension(contentType) if !ok { - if contentType == "application/octet-stream" && isGzipArchiveFromURL(source) { - // For generic binary content, try to detect the actual file type - // by reading the first few bytes (magic bytes) - return true + if contentType == "application/octet-stream" { + isGzip, err := isGzipArchiveFromURL(source) + if err != nil { + slog.Debug("isGzipArchiveFromURL", slog.Any("error", err)) + return false + } + + if isGzip { + // For generic binary content, try to detect the actual file type + // by reading the first few bytes (magic bytes) + return true + } } return false