Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix shellcheck failure in cluster/pre-existing/util.sh #82059

Closed
wants to merge 1 commit into from
Closed

Fix shellcheck failure in cluster/pre-existing/util.sh #82059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions cluster/pre-existing/util.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,14 @@ function create-certs {
rm /tmp/kubeconfig

execute-cmd-on-pre-existing-master-with-retries "sudo cat /etc/kubernetes/admin.conf" > /tmp/kubeconfig
CA_CERT_BASE64=$(cat /tmp/kubeconfig | grep certificate-authority | awk '{print $2}' | head -n 1)
KUBELET_CERT_BASE64=$(cat /tmp/kubeconfig | grep client-certificate-data | awk '{print $2}' | head -n 1)
KUBELET_KEY_BASE64=$(cat /tmp/kubeconfig | grep client-key-data | awk '{print $2}' | head -n 1)

# Local kubeconfig.kubemark vars
KUBECFG_CERT_BASE64="${KUBELET_CERT_BASE64}"
KUBECFG_KEY_BASE64="${KUBELET_KEY_BASE64}"

# The pre-existing Kubernetes master already has these setup
# Set these vars but don't use them
CA_KEY_BASE64=$(dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null)
MASTER_CERT_BASE64=$(dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null)
MASTER_KEY_BASE64=$(dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null)
KUBEAPISERVER_CERT_BASE64=$(dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null)
KUBEAPISERVER_KEY_BASE64=$(dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null)

# CA_CERT_BASE64, KUBELET_CERT_BASE64 and KUBELET_KEY_BASE64 might be used in test/kubemark/iks/util.sh
# So, we leave the variables and export in PR #82059.
# If it becomes clear that the variables are not used anywhere, then we can remove.
CA_CERT_BASE64=$(grep certificate-authority /tmp/kubeconfig | awk '{print $2}' | head -n 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a comment about what uses these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CA_CERT_BASE64 defined at this line is not used by anywhere.

Because, cluster/gce/util.sh use it, but it is defined by cluster/gce/util.sh
https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/util.sh#L1598
Also, test/kubemark/iks/startup.sh and test/kubemark/iks/util.s use it, but create-certs which defines CA_CERT_BASE64 is not used by those files.

From these things, I will remove create_certs in this PR. Do we need to add any comments to the file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we remove it we don't need comments, but if we're exporting something ideally we note that so we can eliminate it in the future potentially (or avoid doing so because we already did the work to discover what depends on it!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to leave those variables safely and add comments to the file.
Could you check it?

export CA_CERT_BASE64
KUBELET_CERT_BASE64=$(grep client-certificate-data /tmp/kubeconfig | awk '{print $2}' | head -n 1)
export KUBELET_CERT_BASE64
KUBELET_KEY_BASE64=$(grep client-key-data /tmp/kubeconfig | awk '{print $2}' | head -n 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shellcheck detects SC2002 in line 47 - 50.
SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

export KUBELET_KEY_BASE64
}
1 change: 0 additions & 1 deletion hack/.shellcheck_failures
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,3 @@
./cluster/gce/upgrade.sh
./cluster/gce/util.sh
./cluster/log-dump/log-dump.sh
./cluster/pre-existing/util.sh