refactor kmeshctl secret command#1545
Conversation
Summary of ChangesHello @LiZhenCheng9527, 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 refactors the Highlights
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. A tool once heavy, now light and free, Its secrets managed, for all to see. Through server's hand, the tasks now flow, Dependencies shed, a cleaner glow. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the kmeshctl secret command by moving the implementation from the kmeshctl CLI tool to the kmesh status server. This is a good architectural improvement that reduces dependencies in the client tool and centralizes the logic on the server-side. The changes in kmeshctl replace the Kubernetes client interactions with HTTP requests to the status server, and the status_server now includes handlers for creating, retrieving, and deleting secrets. The necessary RBAC permissions for the kmesh daemon to manage secrets have also been correctly added.
My review identifies a couple of high-severity issues in the new server-side implementation. Specifically, the Kubernetes API calls use context.TODO() instead of the request's context, which can lead to resource leaks. Additionally, one of the handlers manually constructs a JSON response, which is not robust and can produce invalid JSON. I have provided suggestions to address these points.
pkg/status/status_server.go
Outdated
| func (s *Server) createSecret(w http.ResponseWriter, r *http.Request) { | ||
| var ipSecKey, ipSecKeyOld ipsec.IpSecKey | ||
| var err error | ||
|
|
||
| ipSecKey.AeadKeyName = AeadAlgoName | ||
|
|
||
| // get key from query param | ||
| var aeadKey []byte | ||
| keyParam := r.URL.Query().Get("key") | ||
|
|
||
| if keyParam != "" { | ||
| aeadKey, err = hex.DecodeString(keyParam) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to decode hex string: %v", err), http.StatusBadRequest) | ||
| return | ||
| } | ||
| if len(aeadKey) != AeadKeyLength { | ||
| http.Error(w, "invalid key length: expected 36 bytes (256-bit key + 32-bit salt)", http.StatusBadRequest) | ||
| return | ||
| } | ||
| } else { | ||
| // generate random key if not provided | ||
| aeadKey = make([]byte, AeadKeyLength) | ||
| _, err := rand.Read(aeadKey) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to generate random key: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| ipSecKey.AeadKey = aeadKey | ||
| ipSecKey.Length = AeadAlgoICVLength | ||
|
|
||
| // Get Kubernetes client. | ||
| clientset, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to connect k8s client: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| // Get existing secret. | ||
| secretOld, err := clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Get(context.TODO(), secretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| http.Error(w, fmt.Sprintf("failed to get secret: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| ipSecKey.Spi = 1 | ||
| } else { | ||
| err = json.Unmarshal(secretOld.Data["ipSec"], &ipSecKeyOld) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to unmarshal secret: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| ipSecKey.Spi = ipSecKeyOld.Spi + 1 | ||
| } | ||
|
|
||
| secretData, err := json.Marshal(ipSecKey) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to convert ipsec key to secret data: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| // Create or update secret. | ||
| secret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: secretName, | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| Data: map[string][]byte{ | ||
| "ipSec": secretData, | ||
| }, | ||
| } | ||
|
|
||
| if ipSecKey.Spi == 1 { | ||
| _, err = clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Create(context.TODO(), secret, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to create secret: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| } else { | ||
| _, err = clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Update(context.TODO(), secret, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to update secret: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprintf(w, "Successfully created/updated secret %s in namespace %s with SPI %d\n", secretName, utils.KmeshNamespace, ipSecKey.Spi) | ||
| } |
There was a problem hiding this comment.
The Kubernetes API calls in this function use context.TODO(). In a server context, it's crucial to use the request's context (r.Context()) instead. This ensures that if the client request is canceled (e.g., client disconnects or timeout), the in-flight operations on the server are also canceled. Using context.TODO() can lead to goroutine leaks and unnecessary resource usage, as the operations will continue to run until they complete, ignoring the request's lifecycle.
func (s *Server) createSecret(w http.ResponseWriter, r *http.Request) {
var ipSecKey, ipSecKeyOld ipsec.IpSecKey
var err error
ipSecKey.AeadKeyName = AeadAlgoName
// get key from query param
var aeadKey []byte
keyParam := r.URL.Query().Get("key")
if keyParam != "" {
aeadKey, err = hex.DecodeString(keyParam)
if err != nil {
http.Error(w, fmt.Sprintf("failed to decode hex string: %v", err), http.StatusBadRequest)
return
}
if len(aeadKey) != AeadKeyLength {
http.Error(w, "invalid key length: expected 36 bytes (256-bit key + 32-bit salt)", http.StatusBadRequest)
return
}
} else {
// generate random key if not provided
aeadKey = make([]byte, AeadKeyLength)
_, err := rand.Read(aeadKey)
if err != nil {
http.Error(w, fmt.Sprintf("failed to generate random key: %v", err), http.StatusInternalServerError)
return
}
}
ipSecKey.AeadKey = aeadKey
ipSecKey.Length = AeadAlgoICVLength
// Get Kubernetes client.
clientset, err := utils.CreateKubeClient()
if err != nil {
http.Error(w, fmt.Sprintf("failed to connect k8s client: %v", err), http.StatusInternalServerError)
return
}
// Get existing secret.
secretOld, err := clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Get(r.Context(), secretName, metav1.GetOptions{})
if err != nil {
if !apierrors.IsNotFound(err) {
http.Error(w, fmt.Sprintf("failed to get secret: %v", err), http.StatusInternalServerError)
return
}
ipSecKey.Spi = 1
} else {
err = json.Unmarshal(secretOld.Data["ipSec"], &ipSecKeyOld)
if err != nil {
http.Error(w, fmt.Sprintf("failed to unmarshal secret: %v", err), http.StatusInternalServerError)
return
}
ipSecKey.Spi = ipSecKeyOld.Spi + 1
}
secretData, err := json.Marshal(ipSecKey)
if err != nil {
http.Error(w, fmt.Sprintf("failed to convert ipsec key to secret data: %v", err), http.StatusInternalServerError)
return
}
// Create or update secret.
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
},
Type: corev1.SecretTypeOpaque,
Data: map[string][]byte{
"ipSec": secretData,
},
}
if ipSecKey.Spi == 1 {
_, err = clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Create(r.Context(), secret, metav1.CreateOptions{})
if err != nil {
http.Error(w, fmt.Sprintf("failed to create secret: %v", err), http.StatusInternalServerError)
return
}
} else {
_, err = clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Update(r.Context(), secret, metav1.UpdateOptions{})
if err != nil {
http.Error(w, fmt.Sprintf("failed to update secret: %v", err), http.StatusInternalServerError)
return
}
}
w.WriteHeader(http.StatusOK)
fmt.Fprintf(w, "Successfully created/updated secret %s in namespace %s with SPI %d\n", secretName, utils.KmeshNamespace, ipSecKey.Spi)
}
pkg/status/status_server.go
Outdated
| func (s *Server) getSecret(w http.ResponseWriter, r *http.Request) { | ||
| log.Infof("get secret") | ||
| clientset, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to connect k8s client: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| secret, err := clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Get(context.TODO(), secretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| http.Error(w, fmt.Sprintf("secret %s not found", secretName), http.StatusNotFound) | ||
| return | ||
| } | ||
| http.Error(w, fmt.Sprintf("failed to get secret: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| log.Infof("secret: %+v", secret) | ||
| if secret.Data == nil || secret.Data["ipSec"] == nil { | ||
| http.Error(w, "invalid secret data: missing ipSec field", http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| var ipSecKey ipsec.IpSecKey | ||
| if err := json.Unmarshal(secret.Data["ipSec"], &ipSecKey); err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to unmarshal secret data: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| displayKey := struct { | ||
| Spi int `json:"spi"` | ||
| AeadKeyName string `json:"aeadKeyName"` | ||
| AeadKey string `json:"aeadKey"` | ||
| Length int `json:"length"` | ||
| }{ | ||
| Spi: ipSecKey.Spi, | ||
| AeadKeyName: ipSecKey.AeadKeyName, | ||
| AeadKey: hex.EncodeToString(ipSecKey.AeadKey), | ||
| Length: ipSecKey.Length, | ||
| } | ||
|
|
||
| displayData, err := json.MarshalIndent(displayKey, "", " ") | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to marshal display data: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprintf(w, "{\n \"secretName\": \"%s\",\n \"namespace\": \"%s\",\n \"created\": \"%s\",\n \"ipSecConfig\": %s\n}", | ||
| secretName, utils.KmeshNamespace, secret.CreationTimestamp.Format("2006-01-02 15:04:05"), string(displayData)) | ||
| } |
There was a problem hiding this comment.
The Kubernetes API call in this function uses context.TODO(). In a server context, it's crucial to use the request's context (r.Context()) instead. This ensures that if the client request is canceled (e.g., client disconnects or timeout), the in-flight operation on the server is also canceled. Using context.TODO() can lead to goroutine leaks and unnecessary resource usage.
Additionally, the JSON response is constructed manually using fmt.Fprintf. This is fragile and can lead to invalid JSON if any of the string components contain special characters that need escaping. It's much safer and more idiomatic to define a struct for the response and use json.Marshal to generate the JSON output.
func (s *Server) getSecret(w http.ResponseWriter, r *http.Request) {
log.Infof("get secret")
clientset, err := utils.CreateKubeClient()
if err != nil {
http.Error(w, fmt.Sprintf("failed to connect k8s client: %v", err), http.StatusInternalServerError)
return
}
secret, err := clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Get(r.Context(), secretName, metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
http.Error(w, fmt.Sprintf("secret %s not found", secretName), http.StatusNotFound)
return
}
http.Error(w, fmt.Sprintf("failed to get secret: %v", err), http.StatusInternalServerError)
return
}
log.Infof("secret: %+v", secret)
if secret.Data == nil || secret.Data["ipSec"] == nil {
http.Error(w, "invalid secret data: missing ipSec field", http.StatusInternalServerError)
return
}
var ipSecKey ipsec.IpSecKey
if err := json.Unmarshal(secret.Data["ipSec"], &ipSecKey); err != nil {
http.Error(w, fmt.Sprintf("failed to unmarshal secret data: %v", err), http.StatusInternalServerError)
return
}
type displayKeyStruct struct {
Spi int `json:"spi"`
AeadKeyName string `json:"aeadKeyName"`
AeadKey string `json:"aeadKey"`
Length int `json:"length"`
}
displayKey := displayKeyStruct{
Spi: ipSecKey.Spi,
AeadKeyName: ipSecKey.AeadKeyName,
AeadKey: hex.EncodeToString(ipSecKey.AeadKey),
Length: ipSecKey.Length,
}
type getSecretResponse struct {
SecretName string `json:"secretName"`
Namespace string `json:"namespace"`
Created string `json:"created"`
IPSecConfig displayKeyStruct `json:"ipSecConfig"`
}
response := getSecretResponse{
SecretName: secretName,
Namespace: utils.KmeshNamespace,
Created: secret.CreationTimestamp.Format("2006-01-02 15:04:05"),
IPSecConfig: displayKey,
}
responseBody, err := json.MarshalIndent(response, "", " ")
if err != nil {
http.Error(w, fmt.Sprintf("failed to marshal response data: %v", err), http.StatusInternalServerError)
return
}
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
_, _ = w.Write(responseBody)
}
pkg/status/status_server.go
Outdated
| func (s *Server) deleteSecret(w http.ResponseWriter, r *http.Request) { | ||
| clientset, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("failed to connect k8s client: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| err = clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Delete(context.TODO(), secretName, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| http.Error(w, fmt.Sprintf("secret %s not found", secretName), http.StatusNotFound) | ||
| return | ||
| } | ||
| http.Error(w, fmt.Sprintf("failed to delete secret: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprintf(w, "Successfully deleted secret %s from namespace %s\n", secretName, utils.KmeshNamespace) | ||
| } |
There was a problem hiding this comment.
The Kubernetes API call in this function uses context.TODO(). In a server context, it's crucial to use the request's context (r.Context()) instead. This ensures that if the client request is canceled (e.g., client disconnects or timeout), the in-flight operation on the server is also canceled. Using context.TODO() can lead to goroutine leaks and unnecessary resource usage.
func (s *Server) deleteSecret(w http.ResponseWriter, r *http.Request) {
clientset, err := utils.CreateKubeClient()
if err != nil {
http.Error(w, fmt.Sprintf("failed to connect k8s client: %v", err), http.StatusInternalServerError)
return
}
err = clientset.Kube().CoreV1().Secrets(utils.KmeshNamespace).Delete(r.Context(), secretName, metav1.DeleteOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
http.Error(w, fmt.Sprintf("secret %s not found", secretName), http.StatusNotFound)
return
}
http.Error(w, fmt.Sprintf("failed to delete secret: %v", err), http.StatusInternalServerError)
return
}
w.WriteHeader(http.StatusOK)
fmt.Fprintf(w, "Successfully deleted secret %s from namespace %s\n", secretName, utils.KmeshNamespace)
}Signed-off-by: LiZhenCheng9527 <lizhencheng6@huawei.com>
00e2251 to
97d2ee9
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Previously, the implementation of the
kmeshctl secretcommand resided withinkmeshctlitself. This causekmeshctlintroducing excessive dependencies. #1541Therefore, this PR moved the location of the
IpsecKeystructure to avoid import BPF-related dependencies.Which issue(s) this PR fixes:
Fixes #1541
Special notes for your reviewer:
Does this PR introduce a user-facing change?: