Webhook fix to allow service account user #8
Conversation
- bundled repo for 1.26 kube - Sample yamls for byoh config with service account - Webhook fix to allow sevice account user - Fix to avoid same secret name for install & bootstrap
Code Review Agent Run Status
|
jayanth-tjvrr
changed the title
There was a problem hiding this comment.
@jayanth-tjvrr there are a lot of changes here that seem unrelated to the webhook change. Is all this needed?
| @@ -109,7 +109,7 @@ run: manifests generate fmt vet ## Run a controller from your host. | |||
| go run ./main.go | |||
|
|
|||
| docker-build: ## Build docker image with the manager. | |||
| docker build -t ${IMG} . | |||
| docker build --network host -t ${IMG} . | |||
| //if !strings.Contains(byoHost.Name, substrs[2]) { | ||
| // return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | ||
| //} |
There was a problem hiding this comment.
Don't comment - remove the lines.
Code Review Agent Run #021764Actionable Suggestions - 11
Additional Suggestions - 3
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
| @@ -16,7 +16,7 @@ | |||
| # // Build and store a BYOH bundle | |||
| # docker run --rm -v <INGREDIENTS_HOST_ABS_PATH>:/ingredients -v <BUNDLE_OUTPUT_ABS_PATH>:/bundle --env <THIS_IMAGE> | |||
|
|
|||
| FROM harbor-repo.vmware.com/dockerhub-proxy-cache/k14s/image | |||
| FROM k14s/image | |||
There was a problem hiding this comment.
Consider using a specific version tag for the base image k14s/image instead of implicitly using latest to ensure reproducible builds and avoid potential breaking changes.
Code suggestion
Check the AI-generated fix before applying
| FROM k14s/image | |
| FROM k14s/image:v0.40.0 |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @@ -3,7 +3,7 @@ | |||
| # Copyright 2021 VMware, Inc. All Rights Reserved. | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| set -e | |||
| set -ex | |||
There was a problem hiding this comment.
Consider if enabling shell debug mode with -x is intentional here. The -x flag will print each command before execution which could expose sensitive information in logs, especially when dealing with bundle operations that may contain credentials or sensitive paths.
Code suggestion
Check the AI-generated fix before applying
| set -ex | |
| set -e |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| NAMESPACE=byoh | ||
| SERVICE_ACCOUNT=default | ||
| CLUSTER_NAME=$(kubectl config view --minify -o jsonpath='{.clusters[0].name}') | ||
| CLUSTER_SERVER=https://10.149.106.220:443 |
There was a problem hiding this comment.
Consider making the CLUSTER_SERVER URL configurable through an environment variable or command line argument instead of hardcoding it. This would make the script more flexible and reusable across different environments.
Code suggestion
Check the AI-generated fix before applying
| CLUSTER_SERVER=https://10.149.106.220:443 | |
| CLUSTER_SERVER=${CLUSTER_SERVER:-$(kubectl config view --minify -o jsonpath='{.clusters[0].server}')} |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @@ -9,14 +9,16 @@ echo Update the apt package index and install packages needed to use the Kubern | |||
| sudo apt-get update | |||
| sudo apt-get install -y apt-transport-https ca-certificates curl | |||
|
|
|||
| mkdir -p /etc/apt/keyrings/ | |||
There was a problem hiding this comment.
Consider adding error handling when creating the /etc/apt/keyrings/ directory. The mkdir command may fail if the script is not run with sufficient permissions.
Code suggestion
Check the AI-generated fix before applying
| mkdir -p /etc/apt/keyrings/ | |
| if ! sudo mkdir -p /etc/apt/keyrings/; then | |
| echo "Error: Failed to create /etc/apt/keyrings/ directory" | |
| exit 1 | |
| fi |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| echo Download containerd | ||
| curl -LOJR https://github.com/containerd/containerd/releases/download/v${CONTAINERD_VERSION}/cri-containerd-cni-${CONTAINERD_VERSION}-linux-amd64.tar.gz | ||
|
|
||
| echo Download the Google Cloud public signing key | ||
| sudo curl -fsSLo /usr/share/keyrings/kubernetes-archive-keyring.gpg https://dl.k8s.io/apt/doc/apt-key.gpg | ||
| curl -fsSL https://pkgs.k8s.io/core:/stable:/${KUBERENETES_MAJOR_VERSION}/deb/Release.key | sudo gpg --dearmor -o /etc/apt/keyrings/kubernetes-apt-keyring.gpg |
There was a problem hiding this comment.
There appears to be a typo in the environment variable name KUBERENETES_MAJOR_VERSION. It should be KUBERNETES_MAJOR_VERSION.
Code suggestion
Check the AI-generated fix before applying
| curl -fsSL https://pkgs.k8s.io/core:/stable:/${KUBERENETES_MAJOR_VERSION}/deb/Release.key | sudo gpg --dearmor -o /etc/apt/keyrings/kubernetes-apt-keyring.gpg | |
| curl -fsSL https://pkgs.k8s.io/core:/stable:/${KUBERNETES_MAJOR_VERSION}/deb/Release.key | sudo gpg --dearmor -o /etc/apt/keyrings/kubernetes-apt-keyring.gpg |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
|
||
| echo Add the Kubernetes apt repository | ||
| echo "deb [signed-by=/usr/share/keyrings/kubernetes-archive-keyring.gpg] https://apt.kubernetes.io/ kubernetes-xenial main" | sudo tee /etc/apt/sources.list.d/kubernetes.list | ||
| echo "deb [signed-by=/etc/apt/keyrings/kubernetes-apt-keyring.gpg] https://pkgs.k8s.io/core:/stable:/${KUBERENETES_MAJOR_VERSION}/deb/ /" | sudo tee /etc/apt/sources.list.d/kubernetes.list |
There was a problem hiding this comment.
Consider validating if ${KUBERENETES_MAJOR_VERSION} is defined before using it in the repository URL. The variable appears to have a typo ('KUBERENETES' instead of 'KUBERNETES') which could cause issues.
Code suggestion
Check the AI-generated fix before applying
| echo "deb [signed-by=/etc/apt/keyrings/kubernetes-apt-keyring.gpg] https://pkgs.k8s.io/core:/stable:/${KUBERENETES_MAJOR_VERSION}/deb/ /" | sudo tee /etc/apt/sources.list.d/kubernetes.list | |
| if [ -z "${KUBERNETES_MAJOR_VERSION}" ]; then echo "Error: KUBERNETES_MAJOR_VERSION is not set"; exit 1; fi | |
| echo "deb [signed-by=/etc/apt/keyrings/kubernetes-apt-keyring.gpg] https://pkgs.k8s.io/core:/stable:/${KUBERNETES_MAJOR_VERSION}/deb/ /" | sudo tee /etc/apt/sources.list.d/kubernetes.list |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| name: default | ||
| namespace: byoh | ||
| roleRef: | ||
| kind: Role | ||
| name: default | ||
| apiGroup: rbac.authorization.k8s.io | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: csr-manager-binding | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: default | ||
| namespace: byoh | ||
| roleRef: | ||
| kind: ClusterRole | ||
| name: csr-manager | ||
| apiGroup: rbac.authorization.k8s.io | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: byohosts-clusterrolebinding | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: default |
There was a problem hiding this comment.
Consider using a more descriptive service account name than default for the byohosts-clusterrolebinding. Using the default service account for cluster-wide permissions could be a security concern.
Code suggestion
Check the AI-generated fix before applying
- name: default
+ name: byoh-manager
namespace: byoh
@@ -20,8 +20,8 @@
- name: default
+ name: byoh-manager
namespace: byoh
@@ -33,8 +33,8 @@
- name: default
+ name: byoh-manager
namespace: byoh
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| - kind: ServiceAccount | ||
| name: default | ||
| namespace: byoh |
There was a problem hiding this comment.
Consider creating a dedicated service account for certificate management operations instead of using the default service account. This would follow security best practices by providing more granular access control.
Code suggestion
Check the AI-generated fix before applying
- kind: ServiceAccount
- name: default
- namespace: byoh
+ kind: ServiceAccount
+ name: cert-manager
+ namespace: byoh
---
apiVersion: v1
kind: ServiceAccount
metadata:
name: cert-manager
namespace: byoh
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| //if !strings.Contains(byoHost.Name, substrs[2]) { | ||
| // return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | ||
| //} |
There was a problem hiding this comment.
The commented out code block that checks host name validation appears to be important for security. Consider keeping this validation to ensure only authorized agents can create/update resources for their assigned hosts.
Code suggestion
Check the AI-generated fix before applying
| //if !strings.Contains(byoHost.Name, substrs[2]) { | |
| // return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | |
| //} | |
| if !strings.Contains(byoHost.Name, substrs[2]) { | |
| return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | |
| } |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| //if !strings.Contains(byoHost.Name, substrs[2]) { | ||
| // return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | ||
| //} |
There was a problem hiding this comment.
Consider adding a space between the comment marker // and the comment text for better readability. The comment should be formatted as // if instead of //if.
Code suggestion
Check the AI-generated fix before applying
| //if !strings.Contains(byoHost.Name, substrs[2]) { | |
| // return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | |
| //} | |
| // if !strings.Contains(byoHost.Name, substrs[2]) { | |
| // return admission.Denied(fmt.Sprintf("%s cannot create/update resource %s", userName, byoHost.Name)) | |
| // } |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| matchLabels: null | ||
| template: |
There was a problem hiding this comment.
The matchLabels field is set to null which could cause issues with label selector matching. Consider either removing this field if not needed or specifying appropriate label selectors that match the template labels defined in lines 48-49.
Code suggestion
Check the AI-generated fix before applying
| matchLabels: null | |
| template: | |
| matchLabels: | |
| nodepool: pool1 | |
| template: |
Code Review Run #021764
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
This PR fixes KAAP-283 and includes fixes from previous branches regarding pushing the k8s bundle.
Fixes:
Edited create/update function to exclude the hostname check. This check is causing issues in our case. The previous code required the username to include the hostname in it for update/create. Since we don't follow that pattern, it was causing issues.
Changed the Installationsecret name to byoinstall-* so that it won't interfere with bootstrapSecret.
Testing:
published these changes in my private image and edited the byoh controller image to my private image.
Tried running the byoh agent with create/update calls in the webhookconfiguration. But still the byoh Host got registered successfully.
Webhook config:
Byoh agent logs:
Summary by Bito
This PR removes hostname validation in the webhook for service account resource management, updates installation secret naming to prevent bootstrap secret conflicts, adds sample configurations for RBAC and cluster setup, and revises Kubernetes package installation with new repository configurations. Several security-related changes are needed including re-enabling hostname validation and implementing dedicated service accounts.Unit tests added: False
Estimated effort to review (1-5, lower is better): 4