Add support for remote manifest #13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for downloading Kubernetes manifests from remote HTTP(S) URLs in addition to local files, enabling users to directly apply or explain manifests hosted on GitHub, GitLab, or other web servers. The implementation introduces a new common.go file with helper functions and updates both the apply and explain commands to handle remote manifests transparently.
Key Changes:
- Introduced remote URL detection and download functionality via
isRemoteFile()anddownloadRemoteFile()helper functions - Updated
applyandexplaincommands to support both local files and HTTP(S) URLs through the existing-f/--filenameflag - Added automatic temporary file management with proper cleanup for downloaded manifests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cmd/common.go | New file containing isRemoteFile() to detect HTTP(S) URLs and downloadRemoteFile() to fetch and store remote manifests in temporary files |
| cmd/explain.go | Updated to detect remote URLs and download them before processing; added documentation examples for remote URL usage |
| cmd/apply.go | Updated to detect remote URLs and download them before transformation; added documentation examples for remote URL usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestDownloadRemoteFile_Timeout(t *testing.T) { | ||
| // This test takes 31+ seconds to run, so it's marked as slow | ||
| if testing.Short() { | ||
| t.Skip("Skipping timeout test in short mode") | ||
| } | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| // Sleep longer than the 30s timeout | ||
| time.Sleep(35 * time.Second) | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| start := time.Now() | ||
| _, err := downloadRemoteFile(server.URL) | ||
| elapsed := time.Since(start) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("Expected timeout error, got nil") | ||
| } | ||
|
|
||
| // Should timeout around 30 seconds, not wait 35+ seconds | ||
| if elapsed > 32*time.Second { | ||
| t.Errorf("Timeout took too long: %v (expected ~30s)", elapsed) | ||
| } | ||
|
|
||
| if !strings.Contains(err.Error(), "timeout") && !strings.Contains(err.Error(), "deadline") { | ||
| t.Logf("Warning: Error message doesn't explicitly mention timeout: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test will fail because it uses httptest.NewServer which creates servers with direct IP addresses (127.0.0.1), blocked by the SSRF protection at URL validation time. The test won't reach the timeout handling it's designed to test.
| actualManifestFile := manifestFile | ||
| var tempFile string | ||
| if isRemoteFile(manifestFile) { | ||
| fmt.Printf("Downloading remote manifest: %s\n", manifestFile) |
There was a problem hiding this comment.
[nitpick] Inconsistent user messaging: the explain command uses an emoji "📥 Downloading remote manifest" while the apply command uses plain text "Downloading remote manifest". For consistency, both commands should use the same message format. Consider either adding the emoji to apply.go or removing it from explain.go.
| fmt.Printf("Downloading remote manifest: %s\n", manifestFile) | |
| fmt.Printf("📥 Downloading remote manifest: %s\n", manifestFile) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bpradipt
force-pushed
the
remote-file
branch
from
11ad7c6 to
28c8737
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return errors.New("redirect to direct IP address blocked") | ||
| } | ||
|
|
||
| // Resolve hostname to IP addresses for SSRF protection |
There was a problem hiding this comment.
[nitpick] The DNS lookup in CheckRedirect uses net.LookupIP without context, while the DialContext at line 155 uses net.DefaultResolver.LookupIP(ctx, "ip", host) with context support. While this is unavoidable (CheckRedirect doesn't provide a context), you might consider adding a comment explaining why the context-less version is used here, to clarify the difference for future maintainers.
| // Resolve hostname to IP addresses for SSRF protection | |
| // Resolve hostname to IP addresses for SSRF protection | |
| // Note: We use net.LookupIP without context here because http.Client's CheckRedirect callback | |
| // does not provide a context parameter. Elsewhere (e.g., in DialContext), we use context-aware DNS lookups. |
| _ = os.Remove(tempFile) | ||
| }() | ||
| actualManifestFile = tempFile | ||
| fmt.Printf("Downloaded to: %s\n", tempFile) |
There was a problem hiding this comment.
[nitpick] Consider using consistent formatting with the explain command. The explain command uses " Downloaded to: %s\n\n" (line 129 in explain.go) with indentation and an extra newline for visual separation, while this uses "Downloaded to: %s\n" without indentation or extra spacing. For consistency, you might want to match the format used in explain.go.
| fmt.Printf("Downloaded to: %s\n", tempFile) | |
| fmt.Printf(" Downloaded to: %s\n\n", tempFile) |
Supports http(s) urls for manifest. It implements the following safeguards - HTTP timeout (30s) - File size validation (10MB) - YAML validation - YAML document limit (10) - Prevent redirect loops Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Adds cmd/common_test.go with 51 test cases covering: - isRemoteFile() URL detection (16 test cases) - HTTP/HTTPS URLs with ports, query params, fragments - File paths (absolute, relative, parent directories) - Invalid URLs and edge cases - isPrivateIP() SSRF protection (21 test cases) - IPv4/IPv6 loopback and link-local addresses - Private IP ranges (10.x, 172.16-31.x, 192.168.x, fc00::, fd00::) - Public IPs and edge cases - downloadRemoteFile() with mock HTTP servers (14 test cases) - Success/error scenarios (404, 500) - Size limit enforcement (Content-Length and actual size) - YAML validation and document limits - Redirect handling with SSRF protection - Timeout handling (30s) - Various Content-Type headers Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
bpradipt
force-pushed
the
remote-file
branch
from
28c8737 to
9f91c38
Compare
1 participant
Supports http(s) urls for manifest