Skip to content

Conversation

@coutinhop
Copy link
Member

Make some ASO settings configurable, like the kind cluster name and utilities versions.

Change the azure resource names from 'winfv-' to 'aso-'.

Description

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

Make some ASO settings configurable, like the kind cluster name and
utilities versions.

Change the azure resource names from 'winfv-' to 'aso-'.
@coutinhop coutinhop self-assigned this Jan 3, 2026
Copilot AI review requested due to automatic review settings January 3, 2026 00:55
@coutinhop coutinhop requested a review from a team as a code owner January 3, 2026 00:55
@marvin-tigera marvin-tigera added this to the Calico v3.32.0 milestone Jan 3, 2026
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jan 3, 2026
@coutinhop coutinhop added docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact and removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jan 3, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR renames Azure resources from 'winfv-' prefix to 'aso-' prefix and makes ASO (Azure Service Operator) settings configurable through environment variables. The changes support better configuration management and align naming with the ASO purpose.

  • Renames all Azure resources, namespaces, and usernames from 'winfv' to 'aso'
  • Introduces configurable versions for Kind cluster, cert-manager, and gomplate
  • Simplifies CI cleanup script by removing the BACKEND variable for Felix resources

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
process/testing/aso/vmss.sh Updates namespace from 'winfv' to 'aso', changes username in all SSH/SCP commands and resource references
process/testing/aso/install-kubeadm.sh Changes username from 'winfv' to 'aso' in SSH/SCP commands and file paths
process/testing/aso/install-aso.sh Adds configurable environment variables for KIND_CLUSTER_NAME, KINDEST_NODE_VERSION, and CERT_MANAGER_VERSION; generates kind-kubeconfig file
process/testing/aso/infra/templates/vnet.yaml Renames VirtualNetwork and VirtualNetworksSubnet from 'vnet-winfv' to 'vnet-aso' and updates namespace
process/testing/aso/infra/templates/vmss-windows.yaml Updates namespace, resource names, username, and secret references from 'winfv' to 'aso'
process/testing/aso/infra/templates/vmss-linux.yaml Updates namespace, resource names, username, secret references, and SSH path from 'winfv' to 'aso'
process/testing/aso/infra/templates/security-group.yaml Renames NetworkSecurityGroup and security rules from 'winfv-sg' to 'aso-sg' and updates namespace
process/testing/aso/infra/templates/resource-group.yaml Updates namespace from 'winfv' to 'aso'
process/testing/aso/infra/templates/password.yaml Renames secret from 'winfv-secret-windows' to 'aso-secret-windows' and updates namespace
process/testing/aso/export-env.sh Adds environment variable exports for KINDEST_NODE_VERSION, KIND_CLUSTER_NAME, and CERT_MANAGER_VERSION
process/testing/aso/Makefile Adds version variables for GOMPLATE_VERSION and KIND_CLUSTER_NAME; updates kind delete command to use configurable cluster name
.semaphore/cleanup.yml Simplifies resource group naming by removing BACKEND variable for Felix resources

Comment on lines +6 to +8
GOMPLATE_VERSION?=v3.11.7

KIND_CLUSTER_NAME?=kind
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The Makefile defines KIND_CLUSTER_NAME, KINDEST_NODE_VERSION, CERT_MANAGER_VERSION, and GOMPLATE_VERSION but doesn't export them to child processes. Since install-aso.sh uses these variables via shell parameter expansion but doesn't source export-env.sh, the Makefile values won't be used unless they're explicitly exported. Consider adding 'export' before these variable declarations in the Makefile to ensure they are passed to install-aso.sh.

Suggested change
GOMPLATE_VERSION?=v3.11.7
KIND_CLUSTER_NAME?=kind
export GOMPLATE_VERSION?=v3.11.7
export KIND_CLUSTER_NAME?=kind

Copilot uses AI. Check for mistakes.
set -o nounset
set -o pipefail

: "${ASO_DIR:="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"}"
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The script sets ASO_DIR but never uses it in this file except for line 36 where it writes kind-kubeconfig. Consider removing this variable assignment if it's not needed, or ensure it's properly used throughout the script for consistency. Note that other scripts in the ASO directory do use ASO_DIR extensively, so this might be intentional for future use.

Copilot uses AI. Check for mistakes.

export KINDEST_NODE_VERSION="${KINDEST_NODE_VERSION:="v1.31.0"}"
export KIND_CLUSTER_NAME="${KIND_CLUSTER_NAME:="kind"}"

Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The new configurable versions are added to export-env.sh, but this file is not sourced by install-aso.sh. This means these environment variable defaults will only be available to scripts that source export-env.sh (like vmss.sh) but not to install-aso.sh. For consistency, either install-aso.sh should source export-env.sh, or these variables should be documented as needing to be set when calling install-aso.sh directly.

Suggested change
# NOTE:
# - This default is only applied when this file is sourced (e.g., by vmss.sh).
# - If you invoke install-aso.sh directly (without sourcing export-env.sh),
# you must ensure CERT_MANAGER_VERSION is set in the environment.

Copilot uses AI. Check for mistakes.
Comment on lines +315 to +330
MASTER_CONNECT_COMMAND="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 aso@${LINUX_EIP}"
WINDOWS_CONNECT_COMMAND="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 aso@${WINDOWS_EIP} powershell"

# Create individual connect commands for each Linux node
for ((i=0; i<${LINUX_NODE_COUNT}; i++)); do
local var_name="LINUX_NODE_${i}_CONNECT"
local cmd="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 winfv@${LINUX_EIPS[$i]}"
local cmd="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 aso@${LINUX_EIPS[$i]}"
eval "export ${var_name}='${cmd}'"
done

# Create individual connect commands for each Windows node (both regular and powershell)
for ((i=0; i<${WINDOWS_NODE_COUNT}; i++)); do
local var_name="WINDOWS_NODE_${i}_CONNECT"
local var_name_ps="WINDOWS_NODE_${i}_CONNECT_PS"
local cmd="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 winfv@${WINDOWS_EIPS[$i]}"
local cmd_ps="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 winfv@${WINDOWS_EIPS[$i]} powershell"
local cmd="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 aso@${WINDOWS_EIPS[$i]}"
local cmd_ps="ssh -i ${SSH_KEY_FILE} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o ConnectTimeout=10 -o ServerAliveInterval=5 -o ServerAliveCountMax=3 aso@${WINDOWS_EIPS[$i]} powershell"
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The SSH helper commands here disable host key verification by using -o UserKnownHostsFile=/dev/null and -o StrictHostKeyChecking=no, which makes man-in-the-middle attacks against connections to your ASO test VMs trivial. An attacker who can intercept traffic between the automation host and these nodes could impersonate a VM, gain an interactive shell, and steal credentials or tamper with the cluster. Replace these options with proper host key pinning (for example by pre-populating known_hosts or using a stricter StrictHostKeyChecking mode) so that SSH verifies the server identity while still allowing non-interactive automation.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@song-jiang
Copy link
Member

song-jiang commented Jan 5, 2026

Looks good but CNI-plugin FV seems having issues. Not sure if it is related to your changes or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants