Fix and optimize the counter demo #162
Conversation
|
@aAAaqwq: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
kubeedge-bot
added
the
size/XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @aAAaqwq, 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 enhances the KubeEdge device management practical case studies by introducing a new and optimized virtual counter demo. The primary goal is to provide a robust example of how KubeEdge can manage virtual devices, collect data, and interact with various data backends and frontends. This new demo offers a complete, end-to-end solution for simulating and controlling a counter device within a KubeEdge environment, demonstrating advanced data handling and user interaction capabilities.
Highlights
- New Virtual Counter Demo: Introduces a comprehensive, optimized virtual counter demo for KubeEdge, showcasing advanced device management capabilities.
- Web-based User Interface: Provides an interactive web UI for real-time control and monitoring of the virtual counter, including start/pause/shutdown operations and customizable start values.
- Flexible Data Handling: Integrates support for various data storage solutions (InfluxDB2, MySQL, Redis, TDengine) and data publishing methods (HTTP, MQTT, OpenTelemetry) within the virtual protocol mapper.
- Simplified Deployment: Includes Makefiles, Dockerfiles, and Kubernetes manifests to streamline the build and deployment process for both the web UI and the virtual protocol mapper.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new counter demo, which is a great addition. However, the implementation contains several critical security vulnerabilities, such as SQL injection risks in the mysql and tdengine database clients, and a critical bug in the mqtt client that causes the entire process to exit on error. There are also high-severity issues related to performance, such as creating new database and MQTT clients for every operation, and maintainability issues like using large, outdated Docker base images and hardcoding configuration values. I've provided specific comments and suggestions to address these critical and high-severity problems.
| insertSQL := fmt.Sprintf("INSERT INTO %s USING %s TAGS ('%s') VALUES('%v','%s', '%s', '%s', '%s');", | ||
| legalTag, legalTable, legalTag, datatime, tableName, data.PropertyName, data.Value, data.Type) |
There was a problem hiding this comment.
Constructing SQL queries using fmt.Sprintf with dynamic, unsanitized inputs is a critical security vulnerability that can lead to SQL injection attacks. You must use parameterized queries (with ? placeholders) to safely pass data to the database.
| if token := client.Connect(); token.Wait() && token.Error() != nil { | ||
| fmt.Println(token.Error()) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Calling os.Exit(1) on a connection failure is a critical issue. This will abruptly terminate the entire mapper process, making the system fragile. Instead of exiting, you should log the error and return it, allowing the caller to handle the failure gracefully (e.g., by retrying).
| if token := client.Connect(); token.Wait() && token.Error() != nil { | |
| fmt.Println(token.Error()) | |
| os.Exit(1) | |
| } | |
| if token := client.Connect(); token.Wait() && token.Error() != nil { | |
| klog.Errorf("failed to connect to mqtt broker: %v", token.Error()) | |
| return | |
| } |
| createTable := fmt.Sprintf("CREATE TABLE IF NOT EXISTS `%s` (id INT AUTO_INCREMENT PRIMARY KEY, ts DATETIME NOT NULL,field TEXT)", tableName) | ||
| _, err := DB.Exec(createTable) | ||
| if err != nil { | ||
| return fmt.Errorf("create tabe into mysql failed with err:%v", err) | ||
| } | ||
|
|
||
| stmt, err := DB.Prepare(fmt.Sprintf("INSERT INTO `%s` (ts,field) VALUES (?,?)", tableName)) | ||
| if err != nil { | ||
| return fmt.Errorf("prepare parament failed with err:%v", err) | ||
| } |
There was a problem hiding this comment.
This function has two major issues:
- Inefficiency: It executes a
CREATE TABLEandPREPAREstatement on every single data insertion. This is highly inefficient and will cause significant performance degradation under load. Table creation should be a one-time setup operation, and prepared statements should be cached and reused. - SQL Injection Risk: Constructing table names dynamically with
fmt.Sprintfis risky and can lead to SQL injection if the input components are not strictly sanitized. While the inputs here come from CRDs, it's a dangerous pattern. Table and column names should not be created from untrusted or dynamic user input.
| // The key to construct the ordered set, here DeviceID is used as the key | ||
| klog.V(4).Infof("tableName:%s", tableName) | ||
| // Check if the current ordered set exists | ||
| deviceData := "TimeStamp: " + strconv.FormatInt(data.TimeStamp, 10) + " PropertyName: " + data.PropertyName + " data: " + data.Value |
There was a problem hiding this comment.
Storing data as a concatenated string in a Redis sorted set member is not a robust practice. It makes the data difficult to parse and query reliably. The corresponding GetDataByDeviceID function attempts to unmarshal this string as JSON, which will always fail, breaking data retrieval. You should store the data in a structured format like JSON to ensure it can be correctly retrieved and parsed.
|
|
||
| RUN CGO_ENABLED=0 GOOS=linux go build -ldflags="-s -w" -o main ./cmd/main.go | ||
|
|
||
| FROM ubuntu:22.04 |
There was a problem hiding this comment.
The final image uses ubuntu:22.04, which is quite large for distributing a single, statically-linked binary. Using a smaller base image like gcr.io/distroless/static-debian11 or even alpine would significantly reduce the image size and potential attack surface, improving security and efficiency.
FROM gcr.io/distroless/static-debian11
| insertSQL := fmt.Sprintf("INSERT INTO %s USING %s TAGS ('%s') VALUES('%v','%s', '%s', '%s', '%s');", | ||
| legalTag, legalTable, legalTag, datatime, tableName, data.PropertyName, data.Value, data.Type) | ||
|
|
||
| rows, _ := DB.Query(stableName) |
There was a problem hiding this comment.
The error returned from DB.Query is being ignored by using the blank identifier _. This can hide important database issues, making the application unreliable and difficult to debug. You should always check and handle errors returned from database operations.
| rows, _ := DB.Query(stableName) | |
| rows, err := DB.Query(stableName) |
| ) | ||
|
|
||
| type PushMethod struct { | ||
| MQTT *MQTTConfig `json:"http"` |
There was a problem hiding this comment.
The JSON tag for the MQTT field is "http", which is incorrect and misleading. It should be "mqtt" to match the field's purpose.
| MQTT *MQTTConfig `json:"http"` | |
| MQTT *MQTTConfig `json:"mqtt"` |
| @@ -0,0 +1,157 @@ | |||
| package tdengine | |||
There was a problem hiding this comment.
This file starts with a UTF-8 Byte Order Mark (BOM), which is unnecessary for Go source files and can cause issues with some tools. Please remove it.
| package tdengine | |
| package tdengine |
| var ( | ||
| DB *sql.DB | ||
| ) |
There was a problem hiding this comment.
Using a global variable for the database connection (DB) is generally discouraged as it can make testing difficult and introduce potential concurrency issues. It's better to encapsulate the *sql.DB connection within a struct, such as the DataBaseConfig struct, and pass it as a receiver to the methods.
Signed-off-by: Daniel Li <[email protected]>
aAAaqwq
force-pushed
the
fix-counter-demo
branch
from
445dff6 to
04651ac
Compare
aAAaqwq
force-pushed
the
fix-counter-demo
branch
from
04651ac to
b189c4e
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new, comprehensive counter demo, including a virtual protocol mapper and a WebUI. While the feature set is impressive, the implementation contains several critical issues that need to be addressed to ensure functionality and maintainability. Key problems include incorrect hardcoded IP addresses, syntax errors, logic bugs that could lead to panics, and inefficiencies in Dockerfiles and data handling. Addressing these points will significantly improve the quality and usability of the demo.
| RUN apt-get update && \ | ||
| apt-get install -y bzip2 curl upx-ucl gcc-aarch64-linux-gnu libc6-dev-arm64-cross gcc-arm-linux-gnueabi libc6-dev-armel-cross libva-dev libva-drm2 libx11-dev libvdpau-dev libxext-dev libsdl1.2-dev libxcb1-dev libxau-dev libxdmcp-dev yasm | ||
|
|
||
| RUN curl -sLO https://ffmpeg.org/releases/ffmpeg-4.1.6.tar.bz2 && \ | ||
| tar -jx --strip-components=1 -f ffmpeg-4.1.6.tar.bz2 && \ | ||
| ./configure && make && \ | ||
| make install | ||
|
|
||
| RUN GOOS=linux go build -o main cmd/main.go | ||
|
|
||
| FROM ubuntu:18.04 | ||
|
|
||
| RUN mkdir -p kubeedge | ||
|
|
||
| RUN apt-get update && \ | ||
| apt-get install -y bzip2 curl upx-ucl gcc-aarch64-linux-gnu libc6-dev-arm64-cross gcc-arm-linux-gnueabi libc6-dev-armel-cross libva-dev libva-drm2 libx11-dev libvdpau-dev libxext-dev libsdl1.2-dev libxcb1-dev libxau-dev libxdmcp-dev yasm | ||
|
|
||
| RUN curl -sLO https://ffmpeg.org/releases/ffmpeg-4.1.6.tar.bz2 && \ | ||
| tar -jx --strip-components=1 -f ffmpeg-4.1.6.tar.bz2 && \ | ||
| ./configure && make && \ | ||
| make install |
There was a problem hiding this comment.
This Dockerfile is inefficient and can be optimized significantly:
- Redundant installations: The
apt-get installandffmpegcompilation steps are repeated in both the builder and the final stage. Build-time dependencies should only be in the builder stage, and the final image should only copy the necessary artifacts. - Outdated base image: The final stage uses
ubuntu:18.04, which is quite old. It's better to use a more recent LTS version likeubuntu:22.04for security and maintenance benefits, consistent withDockerfile_nostream. - Missing cache cleanup: The
RUN apt-get update && apt-get installcommands should be followed by&& rm -rf /var/lib/apt/lists/*to reduce the image size. - Outdated FFMPEG:
ffmpeg-4.1.6is from 2019. Consider using a newer version or installing it from a package manager if possible to simplify the Dockerfile.
aAAaqwq
force-pushed
the
fix-counter-demo
branch
from
b189c4e to
eb4e6e4
Compare
kubeedge-bot
added
size/XS
kubeedge-bot
added
size/XXL
aAAaqwq
force-pushed
the
fix-counter-demo
branch
from
27ca9cf to
04651ac
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Shelley-BaoYue The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubeedge-bot
added
the
approved
4 participants
What type of PR is this?
/kind fix
What this PR does / why we need it:
To resolve issue #6315 :Optimize KubeEdge Device Management Practical Case Studies.
2. Optimize the counter demo.
Which issue(s) this PR fixes:
Fixes #6315
Special notes for your reviewer:
Does this PR introduce a user-facing change?: