feat: faas engine support for env instance #59
Conversation
Summary of ChangesHello @lanmaoxinqing, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the system's capabilities by integrating a Function as a Service (FaaS) engine for environment instance management. It introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a FaaS (Function as a Service) engine, which is a significant new feature. The changes are extensive, including a new FaaS client, corresponding data models, and a generic HTTP client. My review has focused on ensuring correctness, maintainability, and security. I have identified several high-severity issues, including an unsafe type assertion that could lead to a panic, incorrect timestamp handling, and logging practices that could expose sensitive data. Additionally, I've suggested refactoring the JSON parsing logic to be more robust and maintainable. Please review the detailed comments for specific suggestions.
api-service/service/faas_client.go
Outdated
| // use datasource as runtime name | ||
| dynamicRuntimeName := "" | ||
| if name, ok := req.DeployConfig["dataSource"]; ok { | ||
| dynamicRuntimeName = name.(string) |
There was a problem hiding this comment.
The type assertion name.(string) is unsafe and will cause a panic if the value of dataSource in DeployConfig is not a string. This is a critical issue that could crash the service. You should use a two-value type assertion to handle this case gracefully and return an error if the type is incorrect.
s, ok := name.(string)
if !ok {
return nil, fmt.Errorf("value for 'dataSource' in DeployConfig must be a string, but got %T", name)
}
dynamicRuntimeName = s
api-service/service/faas_client.go
Outdated
| Labels: map[string]string{ | ||
| faas_model.LabelStatefulFunction: "true", | ||
| //faas-api-service receiver uses strconv.Atoi, using int here to prevent overflow | ||
| "custom.hcsfaas.hcs.io/idle-timeout": strconv.FormatInt(math.MaxInt, 10), |
There was a problem hiding this comment.
Using math.MaxInt for idle-timeout is architecture-dependent. If this service is compiled on a 64-bit system and the receiving faas-api-service is a 32-bit application that uses strconv.Atoi, it will cause an integer overflow. To ensure cross-platform compatibility, it's safer to use a value that is guaranteed to fit within a 32-bit signed integer, such as math.MaxInt32.
"custom.hcsfaas.hcs.io/idle-timeout": strconv.FormatInt(math.MaxInt32, 10),| var result []*models.EnvInstance | ||
| for _, inst := range resp.Instances { | ||
| result = append(result, &models.EnvInstance{ | ||
| ID: inst.InstanceID, | ||
| IP: inst.IP, | ||
| Status: convertStatus(inst.Status), | ||
| CreatedAt: time.Now().Format("2006-01-02 15:04:05"), // Could consider constructing from CreateTimestamp | ||
| UpdatedAt: time.Now().Format("2006-01-02 15:04:05"), | ||
| TTL: "", | ||
| Env: nil, // Cannot obtain full Env information from Instance | ||
| }) | ||
| } |
There was a problem hiding this comment.
In ListEnvInstances, CreatedAt and UpdatedAt are being set to the current time (time.Now()). This is misleading as it doesn't reflect the actual creation or update times of the instances.
Furthermore, the underlying ListInstances function has a bug where it doesn't parse the createTimestamp field from the API response, even though the faas_model.Instance struct defines this field. This should be fixed in ListInstances, and then ListEnvInstances should use that timestamp for CreatedAt. If UpdatedAt is not available from the API, it should probably be set to the same value as CreatedAt rather than time.Now().
A similar issue with UpdatedAt exists in GetEnvInstance on line 126.
| data, ok := funcResp.Data.(map[string]interface{}) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid response type for Function") | ||
| } | ||
|
|
||
| // Convert map to Function struct | ||
| function := &faas_model.Function{} | ||
| if name, ok := data["name"].(string); ok { | ||
| function.Name = name | ||
| } | ||
| if packageType, ok := data["packageType"].(string); ok { | ||
| function.PackageType = packageType | ||
| } | ||
| if description, ok := data["description"].(string); ok { | ||
| function.Description = description | ||
| } | ||
| if runtime, ok := data["runtime"].(string); ok { | ||
| function.Runtime = runtime | ||
| } | ||
| if memory, ok := data["memory"].(float64); ok { | ||
| function.Memory = int64(memory) | ||
| } | ||
| if timeout, ok := data["timeout"].(float64); ok { | ||
| function.Timeout = int64(timeout) | ||
| } |
There was a problem hiding this comment.
The functions GetFunction, GetRuntime, ListInstances, and GetInstance manually parse the JSON response from a map[string]interface{}. This approach is brittle, error-prone, and hard to maintain. A much safer and cleaner approach is to unmarshal the JSON directly into a typed struct. You can achieve this by passing a pointer to the target struct to the Into() method. This will make the code more robust, easier to read, and less prone to runtime errors from incorrect type assertions or missing keys.
| func (r *HTTPReq) Into(obj interface{}, e ...interface{}) error { | ||
| if len(r.errors) > 0 { | ||
| return r.errors[0] | ||
| } | ||
|
|
||
| if r.resp == nil { | ||
| return fmt.Errorf("response is not ready") | ||
| } | ||
|
|
||
| data, err := io.ReadAll(r.resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read response body: %v", err) | ||
| } | ||
|
|
||
| // 如果执行成功，则把 body 的内容解析到对象中；否则返回报错 | ||
| if r.resp.StatusCode >= http.StatusOK && r.resp.StatusCode < 300 { | ||
| if err := json.Unmarshal(data, obj); err != nil { | ||
| return fmt.Errorf("failed to unmarshal response data: %s. err: %v", data, err) | ||
| } | ||
| return nil | ||
| } else { | ||
| // try to unmarshal the error message into known struct | ||
| if len(e) > 0 { | ||
| if err := json.Unmarshal(data, e[0]); err != nil { | ||
| return fmt.Errorf("http request with non-200 status code: %d, body: %s", r.resp.StatusCode, string(data)) | ||
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("http request with non-200 status code: %d, body: %s", r.resp.StatusCode, string(data)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The Into method logs the full response body in error messages. This can leak sensitive information into logs and also create excessively large log entries, which can be problematic for log storage and analysis. The response body should be truncated before being logged. You could add a helper function for this, similar to the truncateBody function in api-service/service/env_instance.go.
| if req.TTL != "" { | ||
| backendEnv.DeployConfig["ttl"] = req.TTL | ||
| } |
There was a problem hiding this comment.
While it's good to avoid setting an empty TTL, this change might have an unintended side effect. If a user wants to explicitly remove a TTL by passing an empty string, this change would prevent that. The previous behavior would set the ttl key with an empty value. Please confirm if the intention is to disallow unsetting the TTL via an empty string.
| if envServiceController != nil { | ||
| mainRouter.POST("/env-service", | ||
| middleware.AuthTokenMiddleware(tokenEnabled, backendClient), | ||
| middleware.RateLimit(qps), | ||
| envServiceController.CreateEnvService) | ||
| mainRouter.GET("/env-service/:id/list", middleware.AuthTokenMiddleware(tokenEnabled, backendClient), envServiceController.ListEnvServices) | ||
| mainRouter.GET("/env-service/:id", middleware.AuthTokenMiddleware(tokenEnabled, backendClient), envServiceController.GetEnvService) | ||
| mainRouter.DELETE("/env-service/:id", middleware.AuthTokenMiddleware(tokenEnabled, backendClient), envServiceController.DeleteEnvService) | ||
| mainRouter.PUT("/env-service/:id", middleware.AuthTokenMiddleware(tokenEnabled, backendClient), envServiceController.UpdateEnvService) | ||
| } |
There was a problem hiding this comment.
The routes for /env-service are now conditionally registered based on whether envServiceController is initialized. This implies that standard and faas schedule types do not support environment services. While this seems reasonable given the context of FaaS, it's a significant functional change that should be documented or confirmed as intended behavior.
| return // Success, don't send error | ||
| } | ||
|
|
||
| fmt.Printf("Warmup retry: %v\n", lastErr) |
There was a problem hiding this comment.
fmt.Printf is used here for logging. For consistency with the rest of the application and to enable better log management (e.g., setting log levels, structured output), it's recommended to use a structured logger like logrus, which is already used in other parts of the service.
| default: | ||
| return models.EnvInstanceStatusRunning.String() | ||
| } |
There was a problem hiding this comment.
The default case in convertStatus returns models.EnvInstanceStatusRunning.String(). This could mask new or unexpected statuses from the FaaS API by incorrectly reporting them as "Running". It would be better to handle unknown statuses explicitly, for example by returning an "Unknown" status and logging a warning. This makes the system more robust to changes in the downstream API.
default:
return "Unknown"
}|
|
||
| // Constants for HTTP headers | ||
| const ( | ||
| HttpHeaderInstanceID = "Hcs-Faas-Instance-Id" |
There was a problem hiding this comment.
According to Go's naming conventions, common initialisms like "HTTP" and "ID" should be in all caps. This constant should be renamed to HTTPHeaderInstanceID for consistency with standard Go style and tooling.
| HttpHeaderInstanceID = "Hcs-Faas-Instance-Id" | |
| HTTPHeaderInstanceID = "Hcs-Faas-Instance-Id" |
No description provided.