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

add values and checks #454

Merged
merged 1 commit into from
Nov 4, 2024
Merged

add values and checks #454

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
72 changes: 51 additions & 21 deletions hack/e2e.application.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,29 @@ YELLOW='\033[0;33m'
ROOT_NS="tenant-root"
TEST_TENANT="tenant-e2e"

function clean() {
kubectl delete helmrelease.helm.toolkit.fluxcd.io $TEST_TENANT -n $ROOT_NS
if true; then
echo -e "${GREEN}Cleanup successful!${RESET}"
values_base_path="/hack/testdata/"
checks_base_path="/hack/testdata/"
Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix path definitions to be relative to script location.

The hardcoded paths with leading slashes will cause issues when the script is run from different directories. Use relative paths based on the script's location instead.

Apply this diff:

-values_base_path="/hack/testdata/"
-checks_base_path="/hack/testdata/"
+# Get script directory
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+values_base_path="${SCRIPT_DIR}/testdata/"
+checks_base_path="${SCRIPT_DIR}/testdata/"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
values_base_path="/hack/testdata/"
checks_base_path="/hack/testdata/"
# Get script directory
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
values_base_path="${SCRIPT_DIR}/testdata/"
checks_base_path="${SCRIPT_DIR}/testdata/"


function delete_hr() {
local release_name="$1"
local namespace="$2"

if [[ -z "$release_name" ]]; then
echo -e "${RED}Error: Release name is required.${RESET}"
exit 1
fi

if [[ -z "$namespace" ]]; then
echo -e "${RED}Error: Namespace name is required.${RESET}"
exit 1
fi

if [[ "$release_name" == "tenant-e2e" ]]; then
echo -e "${YELLOW}Skipping deletion for release tenant-e2e.${RESET}"
return 0
else
echo -e "${RED}Cleanup failed!${RESET}"
return 1
fi

kubectl delete helmrelease $release_name -n $namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Quote variables in commands to prevent potential word splitting or globbing issues.

It's recommended to quote variables when used in commands to handle cases where variables might contain spaces or special characters.

Apply this diff to quote the variables:

-    kubectl delete helmrelease $release_name -n $namespace
+    kubectl delete helmrelease "$release_name" -n "$namespace"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
kubectl delete helmrelease $release_name -n $namespace
kubectl delete helmrelease "$release_name" -n "$namespace"

}

function install_helmrelease() {
Expand All @@ -43,6 +57,11 @@ function install_helmrelease() {
exit 1
fi

if [[ -n "$values_file" && -f "$values_file" ]]; then
local values_section
values_section=$(echo " values:" && sed 's/^/ /' "$values_file")
fi
Comment on lines +60 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure correct YAML indentation when constructing values_section.

The current method of constructing values_section by prepending spaces using sed 's/^/ /' may not handle existing indentation or complex structures in the values_file, potentially leading to invalid YAML formatting.

Consider using a YAML-aware tool like yq to merge the values file into the HelmRelease manifest to ensure proper YAML formatting.


local helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)
{
echo "apiVersion: helm.toolkit.fluxcd.io/v2"
Expand All @@ -64,11 +83,7 @@ function install_helmrelease() {
echo " version: '*'"
echo " interval: 1m0s"
echo " timeout: 5m0s"

if [[ -n "$values_file" && -f "$values_file" ]]; then
echo " values:"
cat "$values_file" | sed 's/^/ /'
fi
[[ -n "$values_section" ]] && echo "$values_section"
} > "$helmrelease_file"

kubectl apply -f "$helmrelease_file"
Expand All @@ -79,26 +94,38 @@ function install_helmrelease() {
function install_tenant (){
local release_name="$1"
local namespace="$2"
local values_file="${3:-tenant.yaml}"
local values_file="${values_base_path}tenant/values.yaml"
local repo_name="cozystack-apps"
local repo_ns="cozy-public"

install_helmrelease "$release_name" "$namespace" "tenant" "$repo_name" "$repo_ns" "$values_file"
}

function make_extra_checks(){
local checks_file="$1"
echo "after exec make $checks_file"
if [[ -n "$checks_file" && -f "$checks_file" ]]; then
echo -e "${YELLOW}Start extra checks with file: ${checks_file}${RESET}"

fi
}
Comment on lines +103 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

make_extra_checks function does not execute the checks file.

The make_extra_checks function currently only prints messages but does not execute any checks from the provided checks_file. If the intention is to run the checks script, you need to execute it.

Modify the function to execute the checks file:

function make_extra_checks(){
    local checks_file="$1"
-    echo "after exec make $checks_file"
    if [[ -n "$checks_file" && -f "$checks_file" ]]; then
        echo -e "${YELLOW}Start extra checks with file: ${checks_file}${RESET}"
+       bash "$checks_file"
    fi
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function make_extra_checks(){
local checks_file="$1"
echo "after exec make $checks_file"
if [[ -n "$checks_file" && -f "$checks_file" ]]; then
echo -e "${YELLOW}Start extra checks with file: ${checks_file}${RESET}"
fi
}
function make_extra_checks(){
local checks_file="$1"
if [[ -n "$checks_file" && -f "$checks_file" ]]; then
echo -e "${YELLOW}Start extra checks with file: ${checks_file}${RESET}"
bash "$checks_file"
fi
}


function check_helmrelease_status() {
local release_name="$1"
local namespace="$2"
local checks_file="$3"
local timeout=300 # Timeout in seconds
local interval=5 # Interval between checks in seconds
local elapsed=0


while [[ $elapsed -lt $timeout ]]; do
local status_output
status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json | jq -r '.status.conditions[-1].reason')

if [[ "$status_output" == "InstallSucceeded" ]]; then
if [[ "$status_output" == "InstallSucceeded" || "$status_output" == "UpgradeSucceeded" ]]; then
echo -e "${GREEN}Helm release '$release_name' is ready.${RESET}"
make_extra_checks "$checks_file"
delete_hr $release_name $namespace
Comment on lines +125 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid immediate deletion of HelmRelease after it becomes ready.

Deleting the HelmRelease immediately after it reaches a ready state might not allow enough time for all resources to stabilize or for any dependent processes to complete.

Consider adding a delay or additional validation steps before deleting the HelmRelease to ensure all resources are fully operational.

return 0
elif [[ "$status_output" == "InstallFailed" ]]; then
echo -e "${RED}Helm release '$release_name': InstallFailed${RESET}"
Expand All @@ -122,14 +149,17 @@ if [ -z "$chart_name" ]; then
exit 1
fi

echo "Running tests for chart: $chart_name"
install_tenant $TEST_TENANT $ROOT_NS
check_helmrelease_status $TEST_TENANT $ROOT_NS

checks_file="${checks_base_path}${chart_name}/check.sh"
repo_name="cozystack-apps"
repo_ns="cozy-public"

release_name="$chart_name-e2e"
install_helmrelease "$release_name" "$TEST_TENANT" "$chart_name" "$repo_name" "$repo_ns"
values_file="${values_base_path}${chart_name}/values.yaml"

install_tenant $TEST_TENANT $ROOT_NS
check_helmrelease_status $TEST_TENANT $ROOT_NS "${checks_base_path}tenant/check.sh"

echo -e "${YELLOW}Running tests for chart: $chart_name${RESET}"

check_helmrelease_status "$release_name" "$TEST_TENANT"
install_helmrelease $release_name $TEST_TENANT $chart_name $repo_name $repo_ns $values_file
check_helmrelease_status $release_name $TEST_TENANT $checks_file
Comment on lines +153 to +165
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add validation for required files.

The script should validate that the values and checks files exist before proceeding with the installation and checks. Missing files could lead to silent failures.

Add validation before the installation:

 checks_file="${checks_base_path}${chart_name}/check.sh"
 values_file="${values_base_path}${chart_name}/values.yaml"
+
+# Validate required files exist
+if [[ ! -f "$values_file" ]]; then
+    echo -e "${RED}Values file not found: $values_file${RESET}"
+    exit 1
+fi
+
+if [[ ! -f "$checks_file" ]]; then
+    echo -e "${RED}Checks file not found: $checks_file${RESET}"
+    exit 1
+fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
checks_file="${checks_base_path}${chart_name}/check.sh"
repo_name="cozystack-apps"
repo_ns="cozy-public"
release_name="$chart_name-e2e"
install_helmrelease "$release_name" "$TEST_TENANT" "$chart_name" "$repo_name" "$repo_ns"
values_file="${values_base_path}${chart_name}/values.yaml"
install_tenant $TEST_TENANT $ROOT_NS
check_helmrelease_status $TEST_TENANT $ROOT_NS "${checks_base_path}tenant/check.sh"
echo -e "${YELLOW}Running tests for chart: $chart_name${RESET}"
check_helmrelease_status "$release_name" "$TEST_TENANT"
install_helmrelease $release_name $TEST_TENANT $chart_name $repo_name $repo_ns $values_file
check_helmrelease_status $release_name $TEST_TENANT $checks_file
checks_file="${checks_base_path}${chart_name}/check.sh"
repo_name="cozystack-apps"
repo_ns="cozy-public"
release_name="$chart_name-e2e"
values_file="${values_base_path}${chart_name}/values.yaml"
# Validate required files exist
if [[ ! -f "$values_file" ]]; then
echo -e "${RED}Values file not found: $values_file${RESET}"
exit 1
fi
if [[ ! -f "$checks_file" ]]; then
echo -e "${RED}Checks file not found: $checks_file${RESET}"
exit 1
fi
install_tenant $TEST_TENANT $ROOT_NS
check_helmrelease_status $TEST_TENANT $ROOT_NS "${checks_base_path}tenant/check.sh"
echo -e "${YELLOW}Running tests for chart: $chart_name${RESET}"
install_helmrelease $release_name $TEST_TENANT $chart_name $repo_name $repo_ns $values_file
check_helmrelease_status $release_name $TEST_TENANT $checks_file

1 change: 1 addition & 0 deletions hack/testdata/kubernetes/check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
return 0
62 changes: 62 additions & 0 deletions hack/testdata/kubernetes/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
## @section Common parameters

## @param host The hostname used to access the Kubernetes cluster externally (defaults to using the cluster name as a subdomain for the tenant host).
## @param controlPlane.replicas Number of replicas for Kubernetes contorl-plane components
## @param storageClass StorageClass used to store user data
##
host: ""
controlPlane:
replicas: 2
storageClass: replicated

## @param nodeGroups [object] nodeGroups configuration
##
nodeGroups:
md0:
minReplicas: 0
maxReplicas: 10
instanceType: "u1.medium"
ephemeralStorage: 20Gi
Comment on lines +15 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Node group configuration needs adjustment for production use

The current configuration requires changes based on the following findings:

  1. minReplicas: 0 is not recommended as:

    • Other system components like ingress-nginx use minReplicas: 1 as a safe minimum
    • The codebase shows that minReplicas: 1 is standard practice across multiple components
    • Zero replicas could impact system availability

  2. instanceType: "u1.medium" is a valid value as it's consistently used across multiple components in the codebase (vm-instance, virtual-machine, kubernetes packages)

  3. The relationship between minReplicas: 0 and maxReplicas: 10 should be adjusted to ensure:

    • A non-zero minimum for high availability
    • Alignment with other system components that typically use minReplicas: 1

Recommendation:

  • Set minReplicas: 1 to ensure continuous availability
  • Keep maxReplicas: 10 as it provides sufficient scaling headroom
🔗 Analysis chain

Validate node group configuration parameters

Several configuration values need validation:

  1. minReplicas: 0 might not be suitable for production, especially for a node group running critical components like ingress-nginx
  2. The relationship between minReplicas and maxReplicas should be validated
  3. Consider documenting supported instanceType values
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for instance type definitions and validations
rg -l 'instanceType.*u1\.medium'

# Check for any existing node group configurations
rg -l 'minReplicas.*maxReplicas'

Length of output: 299

Script:

#!/bin/bash
# Check the content of files containing instance type references
rg -A 5 -B 5 'instanceType.*u1\.medium' packages/apps/vm-instance/README.md packages/apps/virtual-machine/README.md

# Search for node group validation logic
rg -l 'validate.*[Nn]ode[Gg]roup|[Nn]ode[Gg]roup.*validate'

# Look for any minReplicas validation or usage
rg -A 3 'minReplicas'

# Search for instance type validation or enum definitions
ast-grep --pattern 'const $_ = {
  $$$
  u1.medium: $_,
  $$$
}'

Length of output: 20165

roles:
- ingress-nginx

resources:
cpu: ""
memory: ""
Comment on lines +23 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Specify default resource limits

Empty resource quotas could lead to unbounded resource consumption. Consider setting default values based on your workload requirements.

    resources:
-      cpu: ""
-      memory: ""
+      cpu: "1000m"
+      memory: "2Gi"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resources:
cpu: ""
memory: ""
resources:
cpu: "1000m"
memory: "2Gi"


## @section Cluster Addons
##
addons:

## Cert-manager: automatically creates and manages SSL/TLS certificate
##
certManager:
## @param addons.certManager.enabled Enables the cert-manager
## @param addons.certManager.valuesOverride Custom values to override
enabled: true
valuesOverride: {}

## Ingress-NGINX Controller
##
ingressNginx:
## @param addons.ingressNginx.enabled Enable Ingress-NGINX controller (expect nodes with 'ingress-nginx' role)
## @param addons.ingressNginx.valuesOverride Custom values to override
##
enabled: true
## @param addons.ingressNginx.hosts List of domain names that should be passed through to the cluster by upper cluster
## e.g:
## hosts:
## - example.org
## - foo.example.net
##
hosts: []
valuesOverride: {}

## Flux CD
##
fluxcd:
## @param addons.fluxcd.enabled Enables Flux CD
## @param addons.fluxcd.valuesOverride Custom values to override
##
enabled: true
valuesOverride: {}
1 change: 1 addition & 0 deletions hack/testdata/nats/check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add shebang line for shell specification.

The script is missing a shebang line which is required for proper shell specification and portability.

Add this at the beginning of the file:

+#!/bin/bash
 return 0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return 0
#!/bin/bash
return 0
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

10 changes: 10 additions & 0 deletions hack/testdata/nats/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

## @section Common parameters

## @param external Enable external access from outside the cluster
## @param replicas Persistent Volume size for NATS
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect parameter description for replicas

The description "Persistent Volume size for NATS" does not match the parameter's purpose. The replicas parameter determines the number of NATS instances to run, not the volume size.

-## @param replicas Persistent Volume size for NATS
+## @param replicas Number of NATS instances to run in the cluster
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## @param replicas Persistent Volume size for NATS
## @param replicas Number of NATS instances to run in the cluster

## @param storageClass StorageClass used to store the data
##
external: false
replicas: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Based on the analysis of the schema files across the repository, I can now provide the final response:

Add minimum and maximum constraints for replicas in schema validation

Looking at the repository's patterns, while many applications define replica counts in their schema, none currently implement minimum/maximum constraints. However, for NATS, which is a critical messaging system, adding these constraints would be beneficial:

  1. The current schema in packages/apps/nats/values.schema.json only defines type and default:
"replicas": {
    "type": "number",
    "description": "Persistent Volume size for NATS",
    "default": 2
}
  1. Update it to include constraints:
"replicas": {
    "type": "integer",
    "description": "Number of NATS replicas",
    "minimum": 1,
    "maximum": 5,
    "default": 2
}

Also note that the current description "Persistent Volume size for NATS" is incorrect and should be updated to "Number of NATS replicas" to match the convention used in other charts.

🔗 Analysis chain

Consider adding validation for replicas count

The replicas value should have a minimum of 1 and a reasonable maximum limit to prevent resource exhaustion. Consider using Helm's built-in validation capabilities.

Add schema validation in the values.yaml:

replicas:
  type: integer
  minimum: 1
  maximum: 5
  default: 2

Let me check the content of the found file to understand how replica constraints are implemented in this repository.

Let me check if there are any schema validations in the NATS chart or similar Helm charts in the repository.

Let me check the schema validation for replicas in the NATS chart to see how it's implemented there.

Let me fix the regex and check for replica validation patterns in other schemas.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other NATS deployments in the repo have replica constraints
rg -l "replicas.*minimum|maximum.*replicas" --type yaml

Length of output: 131

Script:

#!/bin/bash
# Check the content of the found file focusing on replica validation
rg -A 5 -B 5 "replicas.*minimum|maximum.*replicas" packages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yaml

Length of output: 729

Script:

#!/bin/bash
# Search for schema validation patterns in Helm charts
rg -g 'values.schema.json' -l . 

# Also check for any validation in NATS related files
fd -e yaml -e json . hack/testdata/nats/ --exec cat {}

Length of output: 1870

Script:

#!/bin/bash
# Check the content of NATS schema validation
cat packages/apps/nats/values.schema.json

# Also check if there are any other examples of replica validation in other schemas
rg -A 5 -B 5 '"replicas":\s*{' packages/apps/**/values.schema.json

Length of output: 1400

Script:

#!/bin/bash
# Check for replica validation patterns in other schema files with fixed regex
rg -A 5 -B 5 '"replicas": \{' packages/apps/**/values.schema.json

# Also check for minimum/maximum constraints in other schema files
rg -A 2 -B 2 '"minimum": |"maximum": ' packages/apps/**/values.schema.json

Length of output: 12402

storageClass: ""
1 change: 1 addition & 0 deletions hack/testdata/tenant/check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
return 0
0 hack/values/tenant.yaml → hack/testdata/tenant/values.yaml
View file
Open in desktop
File renamed without changes.
1 change: 0 additions & 1 deletion packages/core/testing/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ test: wait-for-sandbox copy-hack-dir ## Run the end-to-end tests in existing san

test-applications: wait-for-sandbox copy-hack-dir ## Run the end-to-end tests in existing sandbox for applications.
for app in $(TESTING_APPS); do \
echo "Running tests for $${app}"; \
kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "/hack/e2e.application.sh $${app}"; \
done
kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "kubectl get hr -A | grep -v 'True'"
Expand Down
Loading