Skip to content

Commit 6355e13

Browse files
rdimitrovrdimitrov
authored
Simplify the registry auto-detection (#2976)
* Clear cached registry Signed-off-by: Radoslav Dimitrov <[email protected]> * Simplify auto-detect logic for registry types Signed-off-by: Radoslav Dimitrov <[email protected]> * Use a minimal GET call instead of HEAD Signed-off-by: Radoslav Dimitrov <[email protected]> * Respect the insecure flag when building an HTTP client for the registry Signed-off-by: Radoslav Dimitrov <[email protected]> * Add validation that the file (local and remote) is in the toolhive format Signed-off-by: Radoslav Dimitrov <[email protected]> * Respect URL encoded server names Signed-off-by: Radoslav Dimitrov <[email protected]> * Update the TestRegistryAPI_PutEndpoint test Signed-off-by: Radoslav Dimitrov <[email protected]> * Use remote_servers instead of remoteServers Signed-off-by: Radoslav Dimitrov <[email protected]> * Use the actual registry type to validate the format Signed-off-by: Radoslav Dimitrov <[email protected]> --------- Signed-off-by: Radoslav Dimitrov <[email protected]>
1 parent a5b59c8 commit 6355e13

File tree

10 files changed

+417
-115
lines changed

10 files changed

+417
-115
lines changed

‎cmd/thv/app/config.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ func setRegistryCmdFunc(_ *cobra.Command, args []string) error {
173173
}
174174
// Reset the cached provider so it re-initializes with the new config
175175
registry.ResetDefaultProvider()
176-
fmt.Printf("Successfully set static registry file: %s\n", cleanPath)
176+
fmt.Printf("Successfully set a remote registry file: %s\n", cleanPath)
177177
if allowPrivateRegistryIp {
178178
fmt.Print("Successfully enabled use of private IP addresses for the remote registry\n")
179179
fmt.Print("Caution: allowing registry URLs containing private IP addresses may decrease your security.\n" +
@@ -219,7 +219,7 @@ func getRegistryCmdFunc(_ *cobra.Command, _ []string) error {
219219
case config.RegistryTypeAPI:
220220
fmt.Printf("Current registry: %s (API endpoint)\n", url)
221221
case config.RegistryTypeURL:
222-
fmt.Printf("Current registry: %s (remote URL)\n", url)
222+
fmt.Printf("Current registry: %s (remote file)\n", url)
223223
case config.RegistryTypeFile:
224224
fmt.Printf("Current registry: %s (local file)\n", localPath)
225225
// Check if the file still exists

‎pkg/api/v1/registry.go‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"encoding/json"
55
"fmt"
66
"net/http"
7+
"net/url"
78

89
"github.com/go-chi/chi/v5"
910

@@ -257,6 +258,8 @@ func (rr *RegistryRoutes) updateRegistry(w http.ResponseWriter, r *http.Request)
257258

258259
// Reset the default provider to pick up configuration changes
259260
regpkg.ResetDefaultProvider()
261+
// Reset the config singleton to clear cached configuration
262+
config.ResetSingleton()
260263

261264
response := UpdateRegistryResponse{
262265
Message: message,
@@ -308,17 +311,17 @@ func (rr *RegistryRoutes) handleRegistryReset() (string, string, error) {
308311
}
309312

310313
// handleRegistryURL updates the registry URL
311-
func (rr *RegistryRoutes) handleRegistryURL(url string, allowPrivateIP *bool) (string, string, error) {
314+
func (rr *RegistryRoutes) handleRegistryURL(registryURL string, allowPrivateIP *bool) (string, string, error) {
312315
allow := false
313316
if allowPrivateIP != nil {
314317
allow = *allowPrivateIP
315318
}
316319

317-
if err := rr.configProvider.SetRegistryURL(url, allow); err != nil {
320+
if err := rr.configProvider.SetRegistryURL(registryURL, allow); err != nil {
318321
logger.Errorf("Failed to set registry URL: %v", err)
319322
return "", "", fmt.Errorf("failed to set registry URL: %v", err)
320323
}
321-
return "url", fmt.Sprintf("Successfully set registry URL: %s", url), nil
324+
return "url", fmt.Sprintf("Successfully set registry URL: %s", registryURL), nil
322325
}
323326

324327
// handleRegistryAPIURL updates the registry API URL
@@ -438,6 +441,14 @@ func (rr *RegistryRoutes) getServer(w http.ResponseWriter, r *http.Request) {
438441
registryName := chi.URLParam(r, "name")
439442
serverName := chi.URLParam(r, "serverName")
440443

444+
// URL-decode the server name to handle special characters like forward slashes
445+
// Chi should decode automatically, but we do it explicitly for safety
446+
decodedServerName, err := url.QueryUnescape(serverName)
447+
if err != nil {
448+
// If decoding fails, use the original name
449+
decodedServerName = serverName
450+
}
451+
441452
// Only "default" registry is supported currently
442453
if registryName != defaultRegistryName {
443454
http.Error(w, "Registry not found", http.StatusNotFound)
@@ -450,9 +461,9 @@ func (rr *RegistryRoutes) getServer(w http.ResponseWriter, r *http.Request) {
450461
}
451462

452463
// Try to get the server (could be container or remote)
453-
server, err := provider.GetServer(serverName)
464+
server, err := provider.GetServer(decodedServerName)
454465
if err != nil {
455-
logger.Errorf("Failed to get server '%s': %v", serverName, err)
466+
logger.Errorf("Failed to get server '%s': %v", decodedServerName, err)
456467
http.Error(w, "Server not found", http.StatusNotFound)
457468
return
458469
}

‎pkg/api/v1/registry_test.go‎

Lines changed: 83 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -111,46 +111,114 @@ func TestGetRegistryInfo(t *testing.T) {
111111
}
112112
}
113113

114+
//nolint:paralleltest,tparallel // Subtests cannot run in parallel as they share a mock HTTP server
114115
func TestRegistryAPI_PutEndpoint(t *testing.T) {
115116
t.Parallel()
116117

117118
logger.Initialize()
118119

120+
// Create a mock HTTP server that serves valid registry JSON
121+
validRegistryServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
122+
w.Header().Set("Content-Type", "application/json")
123+
registryData := map[string]interface{}{
124+
"servers": map[string]interface{}{
125+
"test-server": map[string]interface{}{
126+
"command": []string{"test"},
127+
"args": []string{},
128+
},
129+
},
130+
}
131+
if err := json.NewEncoder(w).Encode(registryData); err != nil {
132+
t.Logf("Failed to encode registry data: %v", err)
133+
}
134+
}))
135+
defer validRegistryServer.Close()
136+
119137
tests := []struct {
120138
name string
121-
requestBody string
139+
setupFunc func(t *testing.T) string // Returns the request body
122140
expectedCode int
123141
description string
124142
}{
125143
{
126-
name: "valid URL registry",
127-
requestBody: `{"url":"https://example.com/test-registry.json"}`,
144+
name: "valid URL registry",
145+
setupFunc: func(t *testing.T) string {
146+
t.Helper()
147+
// Use the mock server URL with allow_private_ip to enable HTTP for localhost
148+
return `{"url":"` + validRegistryServer.URL + `","allow_private_ip":true}`
149+
},
150+
expectedCode: http.StatusOK,
151+
description: "Valid URL with actual registry data should be accepted",
152+
},
153+
{
154+
name: "valid local file registry",
155+
setupFunc: func(t *testing.T) string {
156+
t.Helper()
157+
// Create a temporary file with valid registry JSON
158+
tempFile := filepath.Join(t.TempDir(), "valid-registry.json")
159+
validJSON := `{"servers": {"test-server": {"command": ["test"], "args": []}}}`
160+
err := os.WriteFile(tempFile, []byte(validJSON), 0600)
161+
require.NoError(t, err)
162+
return `{"local_path":"` + tempFile + `"}`
163+
},
128164
expectedCode: http.StatusOK,
129-
description: "Valid HTTPS URL should be accepted",
165+
description: "Valid local file with proper registry structure should be accepted",
130166
},
131167
{
132-
name: "invalid local path registry",
133-
requestBody: `{"local_path":"/tmp/test-registry.json"}`,
168+
name: "invalid local file - non-existent",
169+
setupFunc: func(t *testing.T) string {
170+
t.Helper()
171+
return `{"local_path":"/tmp/non-existent-registry-file-12345.json"}`
172+
},
134173
expectedCode: http.StatusBadRequest,
135174
description: "Non-existent local file should return 400",
136175
},
137176
{
138-
name: "invalid JSON",
139-
requestBody: `{"invalid":json}`,
177+
name: "invalid local file - wrong structure",
178+
setupFunc: func(t *testing.T) string {
179+
t.Helper()
180+
// Create a file with invalid registry structure
181+
tempFile := filepath.Join(t.TempDir(), "invalid-registry.json")
182+
invalidJSON := `{"test": "registry"}`
183+
err := os.WriteFile(tempFile, []byte(invalidJSON), 0600)
184+
require.NoError(t, err)
185+
return `{"local_path":"` + tempFile + `"}`
186+
},
187+
expectedCode: http.StatusBadRequest,
188+
description: "Local file with invalid registry structure should return 400",
189+
},
190+
{
191+
name: "invalid URL - unreachable",
192+
setupFunc: func(t *testing.T) string {
193+
t.Helper()
194+
return `{"url":"https://invalid-url-that-does-not-exist-12345.example.com/test.json"}`
195+
},
196+
expectedCode: http.StatusBadRequest,
197+
description: "Unreachable URL should return 400",
198+
},
199+
{
200+
name: "invalid JSON",
201+
setupFunc: func(t *testing.T) string {
202+
t.Helper()
203+
return `{"invalid":json}`
204+
},
140205
expectedCode: http.StatusBadRequest,
141206
description: "Invalid JSON should return 400",
142207
},
143208
{
144-
name: "empty body",
145-
requestBody: `{}`,
209+
name: "empty body",
210+
setupFunc: func(t *testing.T) string {
211+
t.Helper()
212+
return `{}`
213+
},
146214
expectedCode: http.StatusOK,
147215
description: "Empty request resets registry (returns 200)",
148216
},
149217
}
150218

151219
for _, tt := range tests {
152220
t.Run(tt.name, func(t *testing.T) {
153-
t.Parallel()
221+
// Note: Not using t.Parallel() here because subtests share the mock server
154222

155223
// Create a temporary config for this test
156224
tempDir := t.TempDir()
@@ -166,7 +234,10 @@ func TestRegistryAPI_PutEndpoint(t *testing.T) {
166234
// Create routes with the test config provider
167235
routes := NewRegistryRoutesWithProvider(configProvider)
168236

169-
req := httptest.NewRequest("PUT", "/default", strings.NewReader(tt.requestBody))
237+
// Get the request body from the setup function
238+
requestBody := tt.setupFunc(t)
239+
240+
req := httptest.NewRequest("PUT", "/default", strings.NewReader(requestBody))
170241
req.Header.Set("Content-Type", "application/json")
171242
rctx := chi.NewRouteContext()
172243
rctx.URLParams.Add("name", "default")

‎pkg/config/interface_test.go‎

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -270,8 +270,6 @@ func TestProviderRegistryOperations(t *testing.T) {
270270
t.Parallel()
271271
logger.Initialize()
272272

273-
tempDir := t.TempDir()
274-
275273
t.Run("DefaultProvider_RegistryOperations", func(t *testing.T) {
276274
t.Parallel()
277275

@@ -284,60 +282,66 @@ func TestProviderRegistryOperations(t *testing.T) {
284282
_, err := pathProvider.LoadOrCreateConfig()
285283
require.NoError(t, err)
286284

287-
// Test SetRegistryURL
285+
// Test SetRegistryURL with invalid URL (validation will fail)
288286
err = pathProvider.SetRegistryURL("https://example.com", true)
289-
assert.NoError(t, err)
287+
// URL validation now checks that the URL returns valid ToolHive registry JSON
288+
// This will fail for non-existent URLs
289+
assert.Error(t, err, "Non-existent URL should fail validation")
290290

291-
// Test GetRegistryConfig after setting URL
292-
url, localPath, allowPrivateIP, registryType := pathProvider.GetRegistryConfig()
293-
assert.Equal(t, "https://example.com", url)
294-
assert.Equal(t, "", localPath)
295-
assert.True(t, allowPrivateIP)
296-
assert.Equal(t, "url", registryType)
297-
298-
// Test SetRegistryFile (must be a JSON file)
291+
// Test SetRegistryFile (must be a JSON file with valid registry structure)
299292
registryFilePath := filepath.Join(tempDir, "registry.json")
300-
err = os.WriteFile(registryFilePath, []byte(`{"test": "registry"}`), 0600)
293+
validRegistryJSON := `{"servers": {"test-server": {"command": ["test"], "args": []}}}`
294+
err = os.WriteFile(registryFilePath, []byte(validRegistryJSON), 0600)
301295
require.NoError(t, err)
302296
err = pathProvider.SetRegistryFile(registryFilePath)
303297
assert.NoError(t, err)
304298

299+
// Test GetRegistryConfig after setting file
300+
url, localPath, allowPrivateIP, registryType := pathProvider.GetRegistryConfig()
301+
assert.Equal(t, "", url)
302+
assert.NotEmpty(t, localPath) // Should have the absolute path
303+
assert.False(t, allowPrivateIP)
304+
assert.Equal(t, "file", registryType)
305+
305306
// Test UnsetRegistry
306307
err = pathProvider.UnsetRegistry()
307308
assert.NoError(t, err)
308309
})
309310

310311
t.Run("PathProvider_RegistryOperations", func(t *testing.T) {
311312
t.Parallel()
313+
tempDir := t.TempDir() // Use separate temp dir for this test
312314
configPath := filepath.Join(tempDir, "path_config.yaml")
313315
provider := NewPathProvider(configPath)
314316

315317
// Create initial config
316318
_, err := provider.LoadOrCreateConfig()
317319
require.NoError(t, err)
318320

319-
// Test SetRegistryURL
321+
// Test SetRegistryURL with invalid URL (validation will fail)
320322
err = provider.SetRegistryURL("https://path-example.com", false)
321-
assert.NoError(t, err)
323+
// URL validation now checks that the URL returns valid ToolHive registry JSON
324+
assert.Error(t, err, "Non-existent URL should fail validation")
322325

323-
// Test GetRegistryConfig after setting URL
324-
url, localPath, allowPrivateIP, registryType := provider.GetRegistryConfig()
325-
assert.Equal(t, "https://path-example.com", url)
326-
assert.Equal(t, "", localPath)
327-
assert.False(t, allowPrivateIP)
328-
assert.Equal(t, "url", registryType)
326+
// Test SetRegistryFile with invalid structure (should fail)
327+
invalidFilePath := filepath.Join(tempDir, "invalid_registry.json")
328+
err = os.WriteFile(invalidFilePath, []byte(`{"test": "registry"}`), 0600)
329+
require.NoError(t, err)
330+
err = provider.SetRegistryFile(invalidFilePath)
331+
assert.Error(t, err, "Invalid registry structure should fail validation")
329332

330-
// Test SetRegistryFile (must be a JSON file)
331-
registryFilePath := filepath.Join(tempDir, "path_registry.json")
332-
err = os.WriteFile(registryFilePath, []byte(`{"test": "registry"}`), 0600)
333+
// Test SetRegistryFile with valid structure (should succeed)
334+
validFilePath := filepath.Join(tempDir, "path_registry.json")
335+
validRegistryJSON := `{"servers": {"test-server": {"command": ["test"], "args": []}}}`
336+
err = os.WriteFile(validFilePath, []byte(validRegistryJSON), 0600)
333337
require.NoError(t, err)
334-
err = provider.SetRegistryFile(registryFilePath)
338+
err = provider.SetRegistryFile(validFilePath)
335339
assert.NoError(t, err)
336340

337341
// Test GetRegistryConfig after setting file
338-
url, localPath, allowPrivateIP, registryType = provider.GetRegistryConfig()
342+
url, localPath, allowPrivateIP, registryType := provider.GetRegistryConfig()
339343
assert.Equal(t, "", url)
340-
assert.Equal(t, registryFilePath, localPath)
344+
assert.NotEmpty(t, localPath) // Should have the absolute path
341345
assert.False(t, allowPrivateIP)
342346
assert.Equal(t, "file", registryType)
343347

0 commit comments

Comments
 (0)