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.

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

BYOH agent systemd service #5

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
11 changes: 11 additions & 0 deletions .ci/build-and-push.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/usr/bin/env bash
set -o nounset
set -o errexit
set -o pipefail

project_root=$(realpath "$(dirname $0)/..")
build_dir=${project_root}/build
CONTAINER_TAG=${CONTAINER_TAG:-${build_dir}/manager-container-tag}
CONTAINER_FULL_TAG=${CONTAINER_FULL_TAG:-${build_dir}/manager-container-full-tag}
GO_VERSION=${GO_VERSION:-1.20.0}

111 changes: 105 additions & 6 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Ensure Make is run with bash shell as some syntax below is bash-specific

#Ensure Make is run with bash shell as some syntax below is bash-specific
SHELL:=/usr/bin/env bash

# Define registries
Expand All @@ -22,9 +23,11 @@ E2E_CONF_FILE ?= ${REPO_ROOT}/test/e2e/config/provider.yaml
ARTIFACTS ?= ${REPO_ROOT}/_artifacts
SKIP_RESOURCE_CLEANUP ?= false
USE_EXISTING_CLUSTER ?= false
EXISTING_CLUSTER_KUBECONFIG_PATH ?=
EXISTING_CLUSTER_BYOHOSTCONFIG_PATH ?=
GINKGO_NOCOLOR ?= false



TOOLS_DIR := $(REPO_ROOT)/hack/tools
BIN_DIR := bin
TOOLS_BIN_DIR := $(TOOLS_DIR)/$(BIN_DIR)
Expand All @@ -36,6 +39,29 @@ BYOH_TEMPLATES := $(REPO_ROOT)/test/e2e/data/infrastructure-provider-byoh
LDFLAGS := -w -s $(shell hack/version.sh)
STATIC=-extldflags '-static'

check-env:
$(shell ./check_env.sh)

Choose a reason for hiding this comment

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

Consider checking check_env.sh return value

Consider capturing and checking the return value of check_env.sh script execution. Currently, the shell command output is captured but not evaluated, which could mask potential environment setup issues.

Code suggestion
Check the AI-generated fix before applying
 @@ -40,2 +40,2 @@ STATIC=-extldflags '-static'
 check-env:
 	./check_env.sh || (echo "Environment check failed"; exit 1)

Code Review Run #dbd031


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged



BUILD_DIR=$(shell pwd)/build
$(BUILD_DIR):
mkdir -p $@

PF9_BYOHOST_SRCDIR := $(BUILD_DIR)/pf9-byohost
$(PF9_BYOHOST_SRCDIR):
echo "make PF9_BYOHOST_SRCDIR $(PF9_BYOHOST_SRCDIR)"
rm -fr $@
mkdir -p $@

AGENT_SRC_DIR := $(REPO_ROOT)
RPM_SRC_ROOT := $(PF9_BYOHOST_SRCDIR)/rpmsrc
DEB_SRC_ROOT := $(PF9_BYOHOST_SRCDIR)/debsrc
COMMON_SRC_ROOT := $(PF9_BYOHOST_SRCDIR)/common
PF9_BYOHOST_DEB_FILE := $(PF9_BYOHOST_SRCDIR)/debsrc/pf9-byohost-agent.deb
RPMBUILD_DIR := $(PF9_BYOHOST_SRCDIR)/rpmsrc
PF9_BYOHOST_RPM_FILE := $(PF9_BYOHOST_SRCDIR)/rpmsrc/pf9-byohost-agent.rpm
DEB_DEP_SRC_ROOT := $(PF9_BYOHOST_SRCDIR)/dependencies

# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
ifeq (,$(shell go env GOBIN))
GOBIN=$(shell go env GOPATH)/bin
Expand Down Expand Up @@ -146,7 +172,7 @@ test-e2e: take-user-input docker-build prepare-byoh-docker-host-image $(GINKGO)
-e2e.artifacts-folder="$(ARTIFACTS)" \
-e2e.config="$(E2E_CONF_FILE)" \
-e2e.skip-resource-cleanup=$(SKIP_RESOURCE_CLEANUP) -e2e.use-existing-cluster=$(USE_EXISTING_CLUSTER) \
-e2e.existing-cluster-kubeconfig-path=$(EXISTING_CLUSTER_KUBECONFIG_PATH)
-e2e.existing-cluster-kubeconfig-path=$(EXISTING_CLUSTER_BYOHOSTCONFIG_PATH)

cluster-templates: kustomize cluster-templates-v1beta1

Expand Down Expand Up @@ -257,8 +283,81 @@ build-metadata-yaml:

build-host-agent-binary: host-agent-binaries
cp bin/byoh-hostagent-linux-amd64 $(RELEASE_DIR)/byoh-hostagent-linux-amd64


##########################################################################


$(RPM_SRC_ROOT): | $(COMMON_SRC_ROOT)
echo "make RPM_SRC_ROOT: $(RPM_SRC_ROOT)"
cp -a $(COMMON_SRC_ROOT) $(RPM_SRC_ROOT)

$(PF9_BYOHOST_RPM_FILE): |$(RPM_SRC_ROOT)
echo "make PF9_BYOHOST_RPM_FILE $(PF9_BYOHOST_RPM_FILE) "
rpmbuild -bb \
--define "_topdir $(RPMBUILD_DIR)" \
--define "_src_dir $(RPM_SRC_ROOT)" \
--define "_githash $(AGENT_SRC_DIR)/scripts/pf9-byohost.spec "

Choose a reason for hiding this comment

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

Consider fixing githash macro definition

The _githash definition in the rpmbuild command appears to be pointing to a spec file path instead of an actual git hash value. Consider updating to use the actual git hash value for proper versioning.

Code suggestion
Check the AI-generated fix before applying
Suggested change
--define "_githash $(AGENT_SRC_DIR)/scripts/pf9-byohost.spec "
--define "_githash $(shell git rev-parse HEAD)" \

Code Review Run #dbd031


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

./$(AGENT_SRC_DIR)/scripts/sign_packages.sh $(PF9_BYOHOST_RPM_FILE)
md5sum $(PF9_BYOHOST_RPM_FILE) | cut -d' ' -f 1 > $(PF9_BYOHOST_RPM_FILE).md5

build-host-agent-rpm: $(PF9_BYOHOST_RPM_FILE)
echo "make agent-rpm pf9_byohost_rpm_file = $(PF9_BYOHOST_RPM_FILE)"

#########################################################################
$(COMMON_SRC_ROOT): build-host-agent-binary
echo "Building COMMON_SRC_ROOT"
mkdir -p $(COMMON_SRC_ROOT)
echo "BUILDING COMMON_SRC_ROOT/binary COPING binary pf9-byoh-hostagent-linux-amd64"
mkdir -p $(COMMON_SRC_ROOT)/binary
cp $(RELEASE_DIR)/byoh-hostagent-linux-amd64 $(COMMON_SRC_ROOT)/binary/pf9-byoh-hostagent-linux-amd64
echo "BUILDING dir for pf9-byohost-service , COPING service pf9-byoh-agent.service "
mkdir -p $(COMMON_SRC_ROOT)/lib/systemd/system/
cp $(AGENT_SRC_DIR)/service/pf9-byohostagent.service $(COMMON_SRC_ROOT)/lib/systemd/system/pf9-byohost-agent.service

$(DEB_SRC_ROOT): | $(COMMON_SRC_ROOT)
cp -a $(COMMON_SRC_ROOT) $(DEB_SRC_ROOT)

$(PF9_BYOHOST_DEB_FILE): $(DEB_SRC_ROOT)
fpm -t deb -s dir -n pf9-byohost-agent \
--description "Platform9 Bring Your Own Host deb package" \
--license "Commercial" --architecture all --url "http://www.platform9.net" --vendor Platform9 \
-d socat -d ethtool -d ebtables -d conntrack \
--after-install $(AGENT_SRC_DIR)/scripts/pf9-byohost-agent-after-install.sh \
--before-remove $(AGENT_SRC_DIR)/scripts/pf9-byohost-agent-before-remove.sh \
--after-remove $(AGENT_SRC_DIR)/scripts/pf9-byohost-agent-after-remove.sh \
-p $(PF9_BYOHOST_DEB_FILE) \
-C $(DEB_SRC_ROOT)/ .
./sign_packages_deb.sh $(PF9_BYOHOST_DEB_FILE)

Choose a reason for hiding this comment

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

Consider using absolute path for script

Consider using absolute path or variable for sign_packages_deb.sh script location. The script path has been changed from $(AGENT_SRC_DIR)/scripts/sign_packages_deb.sh to ./sign_packages_deb.sh which might cause issues if the Makefile is executed from a different directory.

Code suggestion
Check the AI-generated fix before applying
Suggested change
./sign_packages_deb.sh $(PF9_BYOHOST_DEB_FILE)
$(AGENT_SRC_DIR)/scripts/sign_packages_deb.sh $(PF9_BYOHOST_DEB_FILE)

Code Review Run #0ea183


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

md5sum $(PF9_BYOHOST_DEB_FILE) | cut -d' ' -f 1 > $(PF9_BYOHOST_DEB_FILE).md5

build-host-agent-deb: $(PF9_BYOHOST_DEB_FILE)


$(DEB_DEP_SRC_ROOT): build-host-agent-deb
echo "\n Building DEB for dependency package "
mkdir -p $(DEB_DEP_SRC_ROOT)
mkdir -p $(DEB_DEP_SRC_ROOT)/preriqui
echo "downlaoding deb packages - socat,ethtool,ebtables,conntrack"
sudo apt-get download socat
sudo apt-get download ethtool
sudo apt-get download ebtables
sudo apt-get download conntrack

Choose a reason for hiding this comment

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

Consider adding version constraints for dependencies

Consider adding version constraints for package dependencies. Without version constraints, the latest available versions will be downloaded which could introduce compatibility issues. Maybe consider keeping the version constraints for socat, ethtool, ebtables, and conntrack packages.

Code suggestion
Check the AI-generated fix before applying
Suggested change
sudo apt-get download socat
sudo apt-get download ethtool
sudo apt-get download ebtables
sudo apt-get download conntrack
sudo apt-get download socat=$SOCAT_VERSION
sudo apt-get download ethtool=$ETHTOOL_VERSION
sudo apt-get download ebtables=$EBTABLES_VERSION
sudo apt-get download conntrack=$CONNTRACK_VERSION

Code Review Run #0ea183


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

mv *socat* $(DEB_DEP_SRC_ROOT)/preriqui/socat.deb
mv *ethtool* $(DEB_DEP_SRC_ROOT)/preriqui/ethtool.deb
mv *ebtables* $(DEB_DEP_SRC_ROOT)/preriqui/ebtables.deb
mv *conntrack* $(DEB_DEP_SRC_ROOT)/preriqui/conntrack.deb
echo "Successfully Built DEB for dependency package and copied \n "
cp -r $(PF9_BYOHOST_DEB_FILE) $(DEB_DEP_SRC_ROOT)/pf9-byohost-agent.deb
echo "Successfully copied byoh-sgent deb pkg\n"
cp $(AGENT_SRC_DIR)/scripts/install.sh $(DEB_DEP_SRC_ROOT)/install.sh
echo "Successfully copied install.sh\n"
cp $(AGENT_SRC_DIR)/scripts/Dockerfile $(DEB_DEP_SRC_ROOT)/Dockerfile
echo "Successfully copied Dockerfile"

build-byoh-tar : | $(DEB_DEP_SRC_ROOT)
echo $(DEB_DEP_SRC_ROOT)


########################################################################
# go-get-tool will 'go get' any package $2 and install it to $1.
PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
define go-get-tool
Expand All @@ -268,7 +367,7 @@ TMP_DIR=$$(mktemp -d) ;\
cd $$TMP_DIR ;\
go mod init tmp ;\
echo "Downloading $(2)" ;\
GOBIN=$(PROJECT_DIR)/bin go install $(2) ;\
GOBIN=$(PROJECT_DIR)/bi go install $(2) ;\

Choose a reason for hiding this comment

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

Possible typo in GOBIN path directory

There appears to be a typo in the GOBIN path. The directory path bi seems incomplete and should likely be bin based on the original code.

Code suggestion
Check the AI-generated fix before applying
 -GOBIN=$(PROJECT_DIR)/bi go install $(2) ;\n+GOBIN=$(PROJECT_DIR)/bin go install $(2) ;\

Code Review Run #f3f8d6


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

rm -rf $$TMP_DIR ;\
}
endef
7 changes: 6 additions & 1 deletion agent/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func setupflags() {
// clear any discard loggers set by dependecies
klog.ClearLogger()

flag.StringVar(&namespace, "namespace", "default", "Namespace in the management cluster where you would like to register this host")
flag.StringVar(&namespace, "namespace" ,"default","Namespace in the management cluster where you would like to register this host")
flag.Int64Var(&certExpiryDuration, "certExpiryDuration", registration.ExpirationSeconds, "Duration (in seconds) for the expiration of the host certificates")
flag.Var(&labels, "label", "labels to attach to the ByoHost CR in the form labelname=labelVal for e.g. '--label site=apac --label cores=2'")
flag.StringVar(&metricsbindaddress, "metricsbindaddress", ":8080", "metricsbindaddress is the TCP address that the controller should bind to for serving prometheus metrics.It can be set to \"0\" to disable the metrics serving")
Expand Down Expand Up @@ -129,6 +129,9 @@ var (
bootstrapKubeConfig string
certExpiryDuration int64
)
const (
DefaultNamespacePath = "/namespace"
)

// TODO - fix logging
func main() {
Expand Down Expand Up @@ -300,3 +303,5 @@ func getClient(logger logr.Logger, config *rest.Config) client.Client {

return k8sClient
}


2 changes: 1 addition & 1 deletion apis/infrastructure/v1beta1/byohost_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type ByoHostValidator struct {
}

// To allow byoh manager service account to patch ByoHost CR
const managerServiceAccount = "system:serviceaccount:byoh-system:byoh-controller-manager"
const managerServiceAccount = "system:serviceaccount:kaapi:byoh-controller-manager"

//nolint: gocritic
// Handle handles all the requests for ByoHost resource
Expand Down
75 changes: 75 additions & 0 deletions chart-generator/byoh-chart/kustomization.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Adds namespace to all resources.
namespace: byoh-system

# Value of this field is prepended to the
# names of all resources, e.g. a deployment named
# "wordpress" becomes "alices-wordpress".
# Note that it should also match with the prefix (text before '-') of the namespace
# field above.
namePrefix: byoh-

# Labels to add to all resources and selectors.
commonLabels:
cluster.x-k8s.io/provider: "infrastructure-byoh"

bases:
- ../../config/crd
- ../../config/rbac
- ../../config/manager
- ../../config/webhook
- ../../config/certmanager
# [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.
#- ../prometheus


patchesStrategicMerge:
#Protect the /metrics endpoint by putting it behind auth.
# If you want your controller-manager to expose the /metrics
# endpoint w/o any authn/z, please comment the following line.
- manager_auth_proxy_patch.yaml

# Mount the controller config file for loading manager configurations
# through a ComponentConfig type
#- manager_config_patch.yaml

# [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
# crd/kustomization.yaml
- manager_webhook_patch.yaml

# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'.
# Uncomment 'CERTMANAGER' sections in crd/kustomization.yaml to enable the CA injection in the admission webhooks.
# 'CERTMANAGER' needs to be enabled to use ca injection
- webhookcainjection_patch.yaml

# the following config is for teaching kustomize how to do var substitution
vars:
# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix.
- name: CERTIFICATE_NAMESPACE # namespace of the certificate CR
objref:
kind: Certificate
group: cert-manager.io
version: v1
name: serving-cert # this name should match the one in certificate.yaml
fieldref:
fieldpath: metadata.namespace
- name: CERTIFICATE_NAME
objref:
kind: Certificate
group: cert-manager.io
version: v1
name: serving-cert # this name should match the one in certificate.yaml
- name: SERVICE_NAMESPACE # namespace of the service
objref:
kind: Service
version: v1
name: webhook-service
fieldref:
fieldpath: metadata.namespace
- name: SERVICE_NAME
objref:
kind: Service
version: v1
name: webhook-service

configurations:
- kustomizeconfig.yaml
4 changes: 4 additions & 0 deletions chart-generator/byoh-chart/kustomizeconfig.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# This configuration is for teaching kustomize how to update name ref and var substitution
varReference:
- kind: Deployment
path: spec/template/spec/volumes/secret/secretName
25 changes: 25 additions & 0 deletions chart-generator/byoh-chart/manager_auth_proxy_patch.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# This patch injects a sidecar container which is an HTTP proxy for the
# controller manager, it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.
apiVersion: apps/v1
kind: Deployment
metadata:
name: controller-manager
namespace: system
spec:
template:
spec:
containers:
- name: kube-rbac-proxy
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0

Choose a reason for hiding this comment

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

Consider updating kube-rbac-proxy image version

Consider updating the kube-rbac-proxy image version from v0.8.0 to a newer version as v0.8.0 may have known security vulnerabilities. It would be worth checking if a newer version is available.

Code suggestion
Check the AI-generated fix before applying
Suggested change
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.13.0

Code Review Run #dbd031


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

args:
- "--secure-listen-address=0.0.0.0:8443"
- "--upstream=http://127.0.0.1:8080/"
- "--logtostderr=true"
- "--v=10"
ports:
- containerPort: 8443
name: https
- name: manager
args:
- "--metrics-addr=127.0.0.1:8080"
- "--enable-leader-election"
20 changes: 20 additions & 0 deletions chart-generator/byoh-chart/manager_config_patch.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: controller-manager
namespace: system
spec:
template:
spec:
containers:
- name: manager
args:
- "--config=controller_manager_config.yaml"
volumeMounts:
- name: manager-config
mountPath: /controller_manager_config.yaml
subPath: controller_manager_config.yaml
volumes:
- name: manager-config
configMap:
name: manager-config
23 changes: 23 additions & 0 deletions chart-generator/byoh-chart/manager_webhook_patch.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: controller-manager
namespace: system
spec:
template:
spec:
containers:
- name: manager
ports:
- containerPort: 9443
name: webhook-server
protocol: TCP
volumeMounts:
- mountPath: /tmp/k8s-webhook-server/serving-certs
name: cert
readOnly: true
volumes:
- name: cert
secret:
defaultMode: 420
secretName: $(SERVICE_NAME)-cert
17 changes: 17 additions & 0 deletions chart-generator/byoh-chart/metrics_service.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
apiVersion: v1
kind: Service
metadata:
labels:
control-plane: controller-manager
app.kubernetes.io/name: barista
app.kubernetes.io/managed-by: kustomize
name: controller-manager-metrics-service
namespace: system
spec:
ports:
- name: https
port: 8443
protocol: TCP
targetPort: 8443
selector:
control-plane: controller-manager
9 changes: 9 additions & 0 deletions chart-generator/byoh-chart/webhookcainjection_patch.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# This patch adds annotation to admission webhook config and
# the variables $(CERTIFICATE_NAMESPACE) and $(CERTIFICATE_NAME) will be substituted by kustomize.
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
name: validating-webhook-configuration
annotations:
cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
Loading