Refactor DefaultZotConfig
#75
Conversation
WalkthroughThis pull request updates the configuration management for container runtime commands by changing the type of the configuration variable. The variable previously declared as Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@mviswanathsai, it would be great if you could add support for extensions in Zot as well. Please take a look at this doc: https://zotregistry.dev/v2.1.2/admin-guide/admin-configuration/#extensions. You can skip the UI extension; the most required ones are Scrub and Sync. |
|
@Mehul-Kumar-27 I see... scrub seems super useful. I will add the config options in the config struct. And then parse it and update the zot config accordingly. Will that be it, or am I missing something? |
Yes, scrub only has one field |
|
Right, thanks for the heads up. As you said, scrub is pretty straight-forward. I will have to take a deeper look at sync. |
|
@Mehul-Kumar-27 it seems like |
|
Should we add a check to satellite to check if an artifact is already present in the local repository before pulling from the upstream to handle these cases? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
registry/default_config.go (7)
82-88: Consider handling IPv6 addresses in URL generation.The
GetLocalRegistryURLmethod might not handle IPv6 addresses correctly as they need to be enclosed in square brackets.func (c *ZotConfig) GetLocalRegistryURL() string { address := c.HTTP.Address if !strings.HasPrefix(address, "http://") && !strings.HasPrefix(address, "https://") { address = "http://" + address } - return fmt.Sprintf("%s:%s", address, c.HTTP.Port) + // Handle IPv6 addresses + if strings.Contains(address, ":") && !strings.Contains(address, "://") { + parts := strings.Split(address, "://") + if len(parts) == 2 { + return fmt.Sprintf("%s://[%s]:%s", parts[0], parts[1], c.HTTP.Port) + } + return fmt.Sprintf("http://[%s]:%s", address, c.HTTP.Port) + } + return fmt.Sprintf("%s:%s", address, c.HTTP.Port)
90-110: Consider adding file existence check before opening.The
ReadZotConfigfunction could benefit from checking if the file exists before attempting to open it.func ReadZotConfig(filePath string, zotConfig *ZotConfig) error { + if _, err := os.Stat(filePath); os.IsNotExist(err) { + return fmt.Errorf("config file does not exist: %s", filePath) + } file, err := os.Open(filePath)
151-179: Consider caching reflection results for better performance.The
validateExtensionsOrderfunction uses reflection for each validation. Consider caching the field information if this function is called frequently.+var extensionFieldsCache struct { + once sync.Once + fields []reflect.StructField +} + func validateExtensionsOrder(extensions *ZotExtensions) error { - // Extract the non-nil extension fields in the order they appear var usedExtensions []string val := reflect.ValueOf(extensions) - typ := val.Type() + + // Initialize cache if needed + extensionFieldsCache.once.Do(func() { + typ := reflect.TypeOf((*ZotExtensions)(nil)).Elem() + fields := make([]reflect.StructField, typ.NumField()) + for i := 0; i < typ.NumField(); i++ { + fields[i] = typ.Field(i) + } + extensionFieldsCache.fields = fields + }) for i := 0; i < val.NumField(); i++ { field := val.Field(i) if !field.IsNil() { // Only include non-nil fields (enabled extensions) - usedExtensions = append(usedExtensions, typ.Field(i).Tag.Get("json")) + usedExtensions = append(usedExtensions, extensionFieldsCache.fields[i].Tag.Get("json")) } }
181-229: Consider adding TLS certificate validation.The
validateSyncConfigfunction could include validation of TLS certificates whenTLSVerifyis true andCertDiris specified.if reg.TLSVerify && reg.CertDir != "" { + if _, err := os.Stat(reg.CertDir); os.IsNotExist(err) { + return fmt.Errorf("sync.registries[%d].certDir does not exist: %s", i, reg.CertDir) + } + // Optionally, validate certificate files + certFiles, err := os.ReadDir(reg.CertDir) + if err != nil { + return fmt.Errorf("sync.registries[%d].certDir is not readable: %w", i, err) + } + if len(certFiles) == 0 { + return fmt.Errorf("sync.registries[%d].certDir contains no certificates", i) + } }
239-249: Consider validating port number range.The
validateHTTPConfigfunction should check if the port number is within the valid range (1-65535).portPattern := `^\d{1,5}$` if matched, _ := regexp.MatchString(portPattern, http.Port); !matched { return fmt.Errorf("http.port must be a valid numeric string") } + port, _ := strconv.Atoi(http.Port) + if port < 1 || port > 65535 { + return fmt.Errorf("http.port must be between 1 and 65535") + }
251-257: Consider case-insensitive log level validation.The
validateLogConfigfunction should handle log levels case-insensitively for better user experience.func validateLogConfig(log ZotLogConfig) error { validLevels := map[string]bool{"debug": true, "info": true, "warn": true, "error": true} - if !validLevels[log.Level] { + if !validLevels[strings.ToLower(log.Level)] { return fmt.Errorf("log.level must be one of: debug, info, warn, error") }
259-265: Consider adding minimum interval validation.The
validateIntervalfunction should enforce a minimum interval to prevent excessive resource usage.func validateInterval(interval string) error { - _, err := time.ParseDuration(interval) + duration, err := time.ParseDuration(interval) if err != nil { return fmt.Errorf("invalid interval format: %w", err) } + if duration < time.Minute { + return fmt.Errorf("interval must be at least 1 minute") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/root.go(1 hunks)registry/default_config.go(2 hunks)
🔇 Additional comments (6)
cmd/root.go (3)
100-104: LGTM! Type and method name changes improve clarity.The update from
DefaultZotConfigtoZotConfigandReadConfigtoReadZotConfigbetter reflects the purpose of these entities.
106-108: LGTM! Added validation improves robustness.The new validation step ensures configuration correctness before proceeding with registry setup.
110-110: LGTM! Method call updated for consistency.The
GetLocalRegistryURLmethod call is correctly updated to work with the newZotConfigtype.registry/default_config.go (3)
15-18: LGTM! Well-defined extension order.The predefined order of extensions helps maintain consistency and prevents configuration errors.
20-80: LGTM! Well-structured configuration types.The new configuration types are well-organized and properly tagged for JSON serialization. The modular structure improves maintainability and makes the configuration more extensible.
114-148: LGTM! Comprehensive validation logic.The
Validatemethod thoroughly checks all configuration aspects, including required fields and nested structures.
We discussed the sync and scrub features in one of our weekly meetings, and we wanted to include them in the Zot configuration. However, there are some nuances to this, as Zot does not provide these extensions in the way we currently start it. For these extensions to work, Zot requires the full binary, as they fall under specific build tags. A possible workaround is to build the satellite with these build tags. While we can add build tags later, for now, these configs need to be included in the Zot configuration. |
|
Right, I wanted to ask about the mention of "full binary" in the zot docs too. Thanks. We can handle that in another PR if you want. We'll need to check for the extensions field being not nil (Edit: and check that the extensions are enabled), and then pull the right binary for zot accordingly. There might be other hidden nuances to this change though. Do you see anything else? |
Makes sense yes |
While reviewing the satellite code, I realized that DefaultZotConfig is a misleading name. Initially, I mistook it for a constant representing a default configuration for the ZotConfig type.
As we continue building the satellite, we may introduce additional configuration options within DefaultZotConfig to facilitate marshaling config.json. Given this evolution, the name DefaultZotConfig could become increasingly ambiguous.
To improve clarity, I have refactored the structure of ZotConfig (previously DefaultZotConfig) for better readability.
Additionally, I will reword certain parts for further clarity. Would love to hear what you think about this change. We can further refine by adding more configurable fields (that can be un-marshalled), using pointers for optional fields, etc.
Summary by CodeRabbit