ko: get ovn leader pod on necessary#6273
Conversation
Summary of ChangesHello @zhangzujian, 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 introduces significant refactoring to the Highlights
Changelog
Activity
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
|
Signed-off-by: zhangzujian <[email protected]>
![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 refactors the shell script kubectl-ko to improve OVN leader pod retrieval by breaking down the getOvnCentralPod function into smaller, more specific functions, enhancing clarity and efficiency. However, a critical security vulnerability has been identified: the script uses unquoted variables, such as $KUBE_OVN_NS and new leader pod variables, in sensitive kubectl commands (exec, cp, delete), which exposes it to argument injection and unexpected shell behavior. Furthermore, a critical runtime error exists due to a remaining call to the deleted getOvnCentralPod function, and there are high-severity issues with inconsistent pod status checks that could result in selecting non-ready pods. Please ensure all variable expansions are quoted for security, and address the identified functional issues to improve the script's reliability and robustness.
|
|
||
| getOvnCentralPod(){ | ||
| getNBLeaderPod(){ | ||
| NB_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-nb-leader=true | grep ovn-central | awk '{if($2=="1/1" && $3=="Running") print $1}' | head -n 1 ) |
There was a problem hiding this comment.
This refactoring of getOvnCentralPod into smaller functions is a good improvement for modularity. However, it introduces a critical issue: the perf subcommand (at line 1700) still calls the now-deleted getOvnCentralPod function. This will cause the script to fail. Please ensure all call sites are updated.
Additionally, this command can be made more robust and efficient by using a single awk command with a more specific label selector and --no-headers.
| NB_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-nb-leader=true | grep ovn-central | awk '{if($2=="1/1" && $3=="Running") print $1}' | head -n 1 ) | |
| NB_POD=$(kubectl get pod -n "$KUBE_OVN_NS" -l ovn-nb-leader=true,app=ovn-central --no-headers | awk '$2 == "1/1" && $3 == "Running" {print $1; exit}') |
| } | ||
|
|
||
| getNorthdLeaderPod(){ | ||
| NORTHD_POD=$(kubectl get pod -n kube-system -l ovn-northd-leader=true | grep ovn-central | head -n 1 | awk '{print $1}') |
There was a problem hiding this comment.
This function is missing a check to ensure the pod is ready and running (1/1 and Running status), which is present in getNBLeaderPod. This inconsistency can lead to selecting a pod that is not yet ready to serve traffic. Please add the status check for consistency and robustness.
The command can also be improved to be more efficient.
| NORTHD_POD=$(kubectl get pod -n kube-system -l ovn-northd-leader=true | grep ovn-central | head -n 1 | awk '{print $1}') | |
| NORTHD_POD=$(kubectl get pod -n kube-system -l ovn-northd-leader=true,app=ovn-central --no-headers | awk '$2 == "1/1" && $3 == "Running" {print $1; exit}') |
| OVN_IC_NB_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-ic-nb-leader=true | grep ovn-ic-server | head -n 1 | awk '{print $1}') | ||
| if [ -z "$OVN_IC_NB_POD" ]; then | ||
| getICNBLeaderPod(){ | ||
| OVN_IC_NB_LEADER_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-ic-nb-leader=true | grep ovn-ic-server | head -n 1 | awk '{print $1}') |
There was a problem hiding this comment.
This function is missing a check to ensure the pod is ready and running (1/1 and Running status), which is present in getNBLeaderPod. This inconsistency can lead to selecting a pod that is not yet ready to serve traffic. Please add the status check for consistency and robustness.
The command can also be improved to be more efficient by combining grep, head and awk.
| OVN_IC_NB_LEADER_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-ic-nb-leader=true | grep ovn-ic-server | head -n 1 | awk '{print $1}') | |
| OVN_IC_NB_LEADER_POD=$(kubectl get pod -n "$KUBE_OVN_NS" -l ovn-ic-nb-leader=true --no-headers | grep ovn-ic-server | awk '$2 == "1/1" && $3 == "Running" {print $1; exit}') |
| OVN_IC_SB_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-ic-sb-leader=true | grep ovn-ic-server | head -n 1 | awk '{print $1}') | ||
| if [ -z "$OVN_IC_SB_POD" ]; then | ||
| getICSBLeaderPod(){ | ||
| OVN_IC_SB_LEADER_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-ic-sb-leader=true | grep ovn-ic-server | head -n 1 | awk '{print $1}') |
There was a problem hiding this comment.
This function is missing a check to ensure the pod is ready and running (1/1 and Running status), which is present in getNBLeaderPod. This inconsistency can lead to selecting a pod that is not yet ready to serve traffic. Please add the status check for consistency and robustness.
The command can also be improved to be more efficient by combining grep, head and awk.
| OVN_IC_SB_LEADER_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-ic-sb-leader=true | grep ovn-ic-server | head -n 1 | awk '{print $1}') | |
| OVN_IC_SB_LEADER_POD=$(kubectl get pod -n "$KUBE_OVN_NS" -l ovn-ic-sb-leader=true --no-headers | grep ovn-ic-server | awk '$2 == "1/1" && $3 == "Running" {print $1; exit}') |
| set +o pipefail | ||
| if [ $af -eq 4 ]; then | ||
| dstMac=$(kubectl exec $OVN_NB_POD -n $KUBE_OVN_NS -c ovn-central -- ovn-nbctl --data=bare --no-heading --columns=addresses list logical_switch_port | grep -w "$(echo $dst | tr . '\.')" | awk '{print $1}') | ||
| dstMac=$(kubectl exec $OVN_NB_LEADER_POD -n $KUBE_OVN_NS -c ovn-central -- ovn-nbctl --data=bare --no-heading --columns=addresses list logical_switch_port | grep -w "$(echo $dst | tr . '\.')" | awk '{print $1}') |
There was a problem hiding this comment.
The variable $KUBE_OVN_NS is used unquoted in a kubectl exec command. Unquoted variables in bash are subject to word splitting and glob expansion, which can lead to argument injection if the variable contains spaces or other shell-special characters. Although Kubernetes namespaces have restricted characters, it is a security best practice to always quote variables used in command arguments to prevent unexpected behavior and potential injection vulnerabilities.
| dstMac=$(kubectl exec $OVN_NB_LEADER_POD -n $KUBE_OVN_NS -c ovn-central -- ovn-nbctl --data=bare --no-heading --columns=addresses list logical_switch_port | grep -w "$(echo $dst | tr . '\.')" | awk '{print $1}') | |
| dstMac=$(kubectl exec "$OVN_NB_LEADER_POD" -n "$KUBE_OVN_NS" -c ovn-central -- ovn-nbctl --data=bare --no-heading --columns=addresses list logical_switch_port | grep -w "$(echo $dst | tr . '\.')" | awk '{print $1}') |
| kubectl cp $KUBE_OVN_NS/$OVN_NB_POD:/etc/ovn/ovnnb_db.$suffix.backup $(pwd)/ovnnb_db.$suffix.backup | ||
| kubectl exec "$OVN_NB_POD" -n $KUBE_OVN_NS -c ovn-central -- rm -f /etc/ovn/ovnnb_db.$suffix.backup | ||
| kubectl exec "$OVN_NB_LEADER_POD" -n $KUBE_OVN_NS -c ovn-central -- ovsdb-tool cluster-to-standalone /etc/ovn/ovnnb_db.$suffix.backup /etc/ovn/ovnnb_db.db | ||
| kubectl cp $KUBE_OVN_NS/$OVN_NB_LEADER_POD:/etc/ovn/ovnnb_db.$suffix.backup $(pwd)/ovnnb_db.$suffix.backup |
There was a problem hiding this comment.
The variable $OVN_NB_LEADER_POD is used unquoted in a kubectl cp command. This can lead to argument injection if the variable content is manipulated. Always quote variables in bash scripts to ensure they are treated as a single argument and to prevent shell expansion issues.
| kubectl cp $KUBE_OVN_NS/$OVN_NB_LEADER_POD:/etc/ovn/ovnnb_db.$suffix.backup $(pwd)/ovnnb_db.$suffix.backup | |
| kubectl cp "$KUBE_OVN_NS/$OVN_NB_LEADER_POD":/etc/ovn/ovnnb_db.$suffix.backup $(pwd)/ovnnb_db.$suffix.backup |
| echo "Delete ovn central $component_name pod" | ||
| if [[ $component_name == "nb" ]]; then | ||
| kubectl delete pod $OVN_NB_POD -n kube-system | ||
| kubectl delete pod $OVN_NB_LEADER_POD -n kube-system |
There was a problem hiding this comment.
The variable $OVN_NB_LEADER_POD is used unquoted in a kubectl delete command. In bash, unquoted variables can be exploited for argument injection. Even if the source of the variable (e.g., kubectl get pods output) is currently considered safe, quoting is a critical defense-in-depth measure in shell scripting.
| kubectl delete pod $OVN_NB_LEADER_POD -n kube-system | |
| kubectl delete pod "$OVN_NB_LEADER_POD" -n kube-system |
| getOvnCentralPod | ||
| kubectl exec "$OVN_NB_POD" -n $KUBE_OVN_NS -c ovn-central -- ovn-nbctl "$@" | ||
| getNBLeaderPod | ||
| kubectl exec "$OVN_NB_LEADER_POD" -n $KUBE_OVN_NS -c ovn-central -- ovn-nbctl "$@" |
There was a problem hiding this comment.
The variable $KUBE_OVN_NS is used unquoted in a kubectl exec command within the nbctl subcommand handler. This pattern is repeated throughout the script and should be corrected by quoting all variable expansions to prevent argument injection.
| kubectl exec "$OVN_NB_LEADER_POD" -n $KUBE_OVN_NS -c ovn-central -- ovn-nbctl "$@" | |
| kubectl exec "$OVN_NB_LEADER_POD" -n "$KUBE_OVN_NS" -c ovn-central -- ovn-nbctl "$@" |
| } | ||
|
|
||
| getSBLeaderPod(){ | ||
| SB_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-sb-leader=true | grep ovn-central | awk '{if($2=="1/1" && $3=="Running") print $1}' | head -n 1 ) |
There was a problem hiding this comment.
This command can be made more robust and efficient by using a single awk command with a more specific label selector and --no-headers to avoid parsing kubectl's default output and remove the dependency on grep and head.
| SB_POD=$(kubectl get pod -n $KUBE_OVN_NS -l ovn-sb-leader=true | grep ovn-central | awk '{if($2=="1/1" && $3=="Running") print $1}' | head -n 1 ) | |
| SB_POD=$(kubectl get pod -n "$KUBE_OVN_NS" -l ovn-sb-leader=true,app=ovn-central --no-headers | awk '$2 == "1/1" && $3 == "Running" {print $1; exit}') |
Pull Request Test Coverage Report for Build 21743145260Details
💛 - Coveralls |
2 participants
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #(issue-number)