Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions deploy/operator/capi/deploy_capi_cluster.sh
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ if [ "${DISCONNECTED}" = "true" ]; then
oc create secret generic "${ASSISTED_PULLSECRET_NAME}" --from-file=.dockerconfigjson="${ASSISTED_PULLSECRET_JSON}" --type=kubernetes.io/dockerconfigjson -n hypershift
# 2. mirrored hypershift operator image to local mirror registry
HYPERSHIFT_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${HYPERSHIFT_IMAGE}):hypershift"
oc image mirror -a "${PULL_SECRET_FILE}" "${HYPERSHIFT_IMAGE}" "${HYPERSHIFT_LOCAL_IMAGE}"
run_mirror_command_with_retry oc image mirror -a "${PULL_SECRET_FILE}" "${HYPERSHIFT_IMAGE}" "${HYPERSHIFT_LOCAL_IMAGE}"
export HYPERSHIFT_IMAGE="${HYPERSHIFT_LOCAL_IMAGE}"
# 3. mirrored CAPI provider agent image to local mirror registry
if [ ! -z "$PROVIDER_IMAGE" ]
then
export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fix shellcheck SC2155/SC2086 on PROVIDER_LOCAL_IMAGE assignment.

Command substitution's return value is masked by the export assignment, and ${PROVIDER_IMAGE} is unquoted inside $(...), risking word-splitting/globbing.

🔧 Proposed fix
-      export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi"
+      provider_repo=$(get_image_repository_only "${PROVIDER_IMAGE}")
+      export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/${provider_repo}:capi"
📝 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
export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi"
provider_repo=$(get_image_repository_only "${PROVIDER_IMAGE}")
export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/${provider_repo}:capi"
🧰 Tools
🪛 Shellcheck (0.11.0)

[warning] 83-83: Declare and assign separately to avoid masking return values.

(SC2155)


[info] 83-83: Double quote to prevent globbing and word splitting.

(SC2086)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deploy/operator/capi/deploy_capi_cluster.sh` at line 83, The
PROVIDER_LOCAL_IMAGE export combines command substitution with an unquoted
PROVIDER_IMAGE expansion, triggering shellcheck SC2155/SC2086. Update the
assignment in deploy_capi_cluster.sh so the image repository lookup is performed
safely with PROVIDER_IMAGE quoted inside get_image_repository_only, and avoid
masking the command substitution result by separating the export from the
variable construction if needed; use the existing get_image_repository_only and
PROVIDER_LOCAL_IMAGE symbols to locate the fix.

Source: Linters/SAST tools

oc image mirror -a "${PULL_SECRET_FILE}" "${PROVIDER_IMAGE}" "${PROVIDER_LOCAL_IMAGE}"
run_mirror_command_with_retry oc image mirror -a "${PULL_SECRET_FILE}" "${PROVIDER_IMAGE}" "${PROVIDER_LOCAL_IMAGE}"
export PROVIDER_IMAGE="${PROVIDER_LOCAL_IMAGE}"
fi

Expand All @@ -98,7 +98,7 @@ mirror:
- name: ${RELEASE_IMAGE_HCP_OVERRIDE}
- name: ${CAPI_IMAGE}
EOM
oc-mirror --config isc.yaml --authfile "${PULL_SECRET_FILE}" --workspace file://$PWD/mirror docker://"${OCP_MIRROR_REGISTRY}" --v2
run_mirror_command_with_retry oc-mirror --config isc.yaml --authfile "${PULL_SECRET_FILE}" --workspace "file://${PWD}/mirror" docker://"${OCP_MIRROR_REGISTRY}" --v2

# 4. ImageDigestMirrorSet for local mirror registry (prerequisite is the openshift release is mirrored to the local
# registry). Note that older versions of OpenShift, before OpenShift 4.14, don't support this ImageDigestMirrorSet
Expand Down
67 changes: 64 additions & 3 deletions deploy/operator/mirror_utils.sh
Original file line number Diff line number Diff line change
@@ -1,3 +1,64 @@
function mirror_command_succeeded() {
log_file="${1}"

if grep -qE 'one or more errors occurred|errors during mirroring|error: unable to copy layer' "${log_file}"; then
return 1
fi

return 0
}

function mirror_command_name() {
local args=("$@")
local i=0

if [[ "${args[0]}" == "env" ]]; then
i=1
while [[ "${args[i]}" == *=* ]]; do
i=$((i + 1))
done
fi

echo "${args[i]}"
}

function run_mirror_command_with_retry() {
attempts="${MIRROR_RETRY_ATTEMPTS:-5}"
interval="${MIRROR_RETRY_INTERVAL:-60}"
log_file=""
rc=0
cmd_name=$(mirror_command_name "$@")
was_pipefail=false
shopt -q pipefail && was_pipefail=true

for attempt in $(seq 1 "${attempts}"); do
log_file=$(mktemp)
echo "Mirror attempt ${attempt}/${attempts}: ${cmd_name}"

set +o pipefail
"$@" 2>&1 | tee "${log_file}"
rc=${PIPESTATUS[0]}
if ${was_pipefail}; then
set -o pipefail
else
set +o pipefail
fi

if [[ "${rc}" -eq 0 ]] && mirror_command_succeeded "${log_file}"; then
rm -f "${log_file}"
return 0
fi

echo "Mirror failed (exit=${rc}), waiting ${interval}s before retry..."
rm -f "${log_file}"
if [[ "${attempt}" -lt "${attempts}" ]]; then
sleep "${interval}"
fi
done

return 1
}

function mirror_package() {
# Here we will do the next actions:
# 1. Create an index of specific packages from specific remote indexes
Expand Down Expand Up @@ -34,13 +95,13 @@ function mirror_package() {
--packages "${package}" \
--tag "${local_registry_index_tag}"

GODEBUG=x509ignoreCN=0 podman push \
run_mirror_command_with_retry env GODEBUG=x509ignoreCN=0 podman push \
--tls-verify=false \
"${local_registry_index_tag}" \
--authfile "${authfile}"

manifests_dir=$(mktemp -d -t manifests-XXXXXXXXXX)
GODEBUG=x509ignoreCN=0 oc adm catalog mirror \
run_mirror_command_with_retry env GODEBUG=x509ignoreCN=0 oc adm catalog mirror \
"${local_registry_index_tag}" \
"${local_registry_image_tag}" \
--registry-config="${authfile}" \
Expand Down Expand Up @@ -153,7 +214,7 @@ function ocp_mirror_release() {
source_image="${2}"
dest_mirror_repo="${3}"

oc adm -a "${pull_secret_file}" release mirror \
run_mirror_command_with_retry oc adm -a "${pull_secret_file}" release mirror \
--from="${source_image}" \
--to="${dest_mirror_repo}"
}
Expand Down
3 changes: 2 additions & 1 deletion deploy/operator/setup_hive.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
__dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
source ${__dir}/common.sh
source ${__dir}/utils.sh
source ${__dir}/mirror_utils.sh

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Quote ${__dir} in the source line.

🔧 Proposed fix
-source ${__dir}/mirror_utils.sh
+source "${__dir}/mirror_utils.sh"
📝 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
source ${__dir}/mirror_utils.sh
source "${__dir}/mirror_utils.sh"
🧰 Tools
🪛 Shellcheck (0.11.0)

[info] 6-6: Not following: ./mirror_utils.sh was not specified as input (see shellcheck -x).

(SC1091)


[info] 6-6: Double quote to prevent globbing and word splitting.

(SC2086)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deploy/operator/setup_hive.sh` at line 6, The source statement in
setup_hive.sh uses __dir unquoted, which can break when the path contains spaces
or special characters. Update the source line to quote the __dir expansion so
mirror_utils.sh is loaded safely, and keep the change localized to the source
invocation that references __dir.

Source: Linters/SAST tools


set -o xtrace

Expand Down Expand Up @@ -139,7 +140,7 @@ function from_upstream() {

if [ "${DISCONNECTED}" = "true" ]; then
export IMG="${LOCAL_REGISTRY}/localimages/hive:latest"
oc image mirror \
run_mirror_command_with_retry oc image mirror \
-a ${AUTHFILE} \
${HIVE_IMAGE} \
${IMG}
Expand Down
1 change: 1 addition & 0 deletions internal/bminventory/inventory.go
Original file line number Diff line number Diff line change
Expand Up @@ -4669,6 +4669,7 @@ func (b *bareMetalInventory) validateIgnitionEndpoint(ignitionEndpoint *models.I
if ignitionEndpoint == nil || ignitionEndpoint.URL == nil {
return nil
}
*ignitionEndpoint.URL = pkgvalidations.NormalizeHTTPURL(*ignitionEndpoint.URL)
if err := pkgvalidations.ValidateHTTPFormat(*ignitionEndpoint.URL); err != nil {
log.WithError(err).Errorf("Invalid Ignition endpoint URL: %s", *ignitionEndpoint.URL)
return common.NewApiError(http.StatusBadRequest, err)
Expand Down
22 changes: 22 additions & 0 deletions internal/bminventory/inventory_ignition_endpoint_unit_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package bminventory

import (
"testing"

"github.com/openshift/assisted-service/models"
"github.com/sirupsen/logrus"
)

func TestValidateIgnitionEndpointNormalizesUnbracketedIPv6(t *testing.T) {
bm := &bareMetalInventory{}
url := "https://fd2e:6f44:5dd8:c956::14:31187"
ignitionEndpoint := &models.IgnitionEndpoint{URL: &url}

if err := bm.validateIgnitionEndpoint(ignitionEndpoint, logrus.New()); err != nil {
t.Fatalf("validateIgnitionEndpoint returned error: %v", err)
}
want := "https://[fd2e:6f44:5dd8:c956::14]:31187"
if *ignitionEndpoint.URL != want {
t.Fatalf("got URL %q, want %q", *ignitionEndpoint.URL, want)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import (
"github.com/openshift/assisted-service/pkg/auth"
logutil "github.com/openshift/assisted-service/pkg/log"
"github.com/openshift/assisted-service/pkg/mirrorregistries"
pkgvalidations "github.com/openshift/assisted-service/pkg/validations"
"github.com/openshift/assisted-service/restapi/operations/installer"
operations "github.com/openshift/assisted-service/restapi/operations/manifests"
hivev1 "github.com/openshift/hive/apis/hive/v1"
Expand Down Expand Up @@ -905,7 +906,7 @@ func (r *ClusterDeploymentsReconciler) parseIgnitionEndpoint(ctx context.Context
kubeapiIgnitionEndpoint *hiveext.IgnitionEndpoint) (*models.IgnitionEndpoint, error) {

ignitionEndpoint := &models.IgnitionEndpoint{}
ignitionEndpoint.URL = swag.String(kubeapiIgnitionEndpoint.Url)
ignitionEndpoint.URL = swag.String(pkgvalidations.NormalizeHTTPURL(kubeapiIgnitionEndpoint.Url))

caCertificateReference := kubeapiIgnitionEndpoint.CaCertificateReference
if caCertificateReference != nil {
Expand Down
4 changes: 3 additions & 1 deletion internal/host/hostutil/host_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/openshift/assisted-service/internal/constants"
"github.com/openshift/assisted-service/models"
"github.com/openshift/assisted-service/pkg/conversions"
pkgvalidations "github.com/openshift/assisted-service/pkg/validations"
"github.com/pkg/errors"
"github.com/samber/lo"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -372,7 +373,8 @@ func GetIgnitionEndpointAndCert(cluster *common.Cluster, host *models.Host, logg

// Use custom ignition endpoint if provided
if cluster.IgnitionEndpoint != nil && cluster.IgnitionEndpoint.URL != nil {
url, err := url.Parse(*cluster.IgnitionEndpoint.URL)
normalizedURL := pkgvalidations.NormalizeHTTPURL(*cluster.IgnitionEndpoint.URL)
url, err := url.Parse(normalizedURL)
if err != nil {
return "", nil, err
}
Expand Down
31 changes: 31 additions & 0 deletions internal/host/hostutil/host_utils_ignition_url_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package hostutil

import (
"testing"

"github.com/openshift/assisted-service/internal/common"
"github.com/openshift/assisted-service/models"
"github.com/sirupsen/logrus"
)

func TestGetIgnitionEndpointAndCertUnbracketedIPv6CustomEndpoint(t *testing.T) {
customEndpoint := "https://fd2e:6f44:5dd8:c956::14:31187"
cluster := common.Cluster{
Cluster: models.Cluster{
IgnitionEndpoint: &models.IgnitionEndpoint{URL: &customEndpoint},
},
}
host := models.Host{Role: models.HostRoleWorker}

url, cert, err := GetIgnitionEndpointAndCert(&cluster, &host, logrus.New())
if err != nil {
t.Fatalf("GetIgnitionEndpointAndCert returned error: %v", err)
}
if cert != nil {
t.Fatalf("expected no certificate, got %v", cert)
}
want := "https://[fd2e:6f44:5dd8:c956::14]:31187/worker"
if url != want {
t.Fatalf("got URL %q, want %q", url, want)
}
}
9 changes: 9 additions & 0 deletions internal/host/hostutil/host_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,15 @@ var _ = Describe("Ignition endpoint URL generation", func() {
Expect(cert).Should(BeNil())
Expect(err).ShouldNot(HaveOccurred())
})
It("for cluster with unbracketed IPv6 custom IgnitionEndpoint", func() {
customEndpoint := "https://fd2e:6f44:5dd8:c956::14:31187"
Expect(db.Model(&cluster).Update("ignition_endpoint_url", customEndpoint).Error).ShouldNot(HaveOccurred())

url, cert, err := GetIgnitionEndpointAndCert(&cluster, &host, logrus.New())
Expect(url).Should(Equal("https://[fd2e:6f44:5dd8:c956::14]:31187/worker"))
Expect(cert).Should(BeNil())
Expect(err).ShouldNot(HaveOccurred())
})
It("failing for cluster with wrong IgnitionEndpoint", func() {
customEndpoint := "https\\://foo.bar:33735/acme"
Expect(db.Model(&cluster).Update("ignition_endpoint_url", customEndpoint).Error).ShouldNot(HaveOccurred())
Expand Down
39 changes: 39 additions & 0 deletions pkg/validations/validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,47 @@ func ValidateNTPSource(ntpSource string) bool {
return false
}

// NormalizeHTTPURL rewrites HTTP(S) URLs that contain an unbracketed IPv6 literal host
// so they can be parsed by net/url (Go 1.26+).
func NormalizeHTTPURL(theurl string) string {
if theurl == "" {
return theurl
}
const schemeSep = "://"
schemeIdx := strings.Index(theurl, schemeSep)
if schemeIdx < 0 {
return theurl
}
scheme := theurl[:schemeIdx+len(schemeSep)]
remainder := theurl[schemeIdx+len(schemeSep):]

path := ""
if slash := strings.Index(remainder, "/"); slash >= 0 {
path = remainder[slash:]
remainder = remainder[:slash]
}

if strings.HasPrefix(remainder, "[") {
return theurl
}

lastColon := strings.LastIndex(remainder, ":")
if lastColon < 0 {
return theurl
}

host := remainder[:lastColon]
port := remainder[lastColon+1:]
if port == "" || net.ParseIP(host) == nil || !strings.Contains(host, ":") {
return theurl
}

return scheme + net.JoinHostPort(host, port) + path
}

// ValidateHTTPFormat validates the HTTP and HTTPS format
func ValidateHTTPFormat(theurl string) error {
theurl = NormalizeHTTPURL(theurl)
u, err := url.Parse(theurl)
if err != nil {
return fmt.Errorf("URL '%s' format is not valid: %w", theurl, err)
Expand Down
17 changes: 17 additions & 0 deletions pkg/validations/validations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ var _ = Describe("URL validations", func() {
{"http://10.9.8.7:123/config", ""},
{"http://[1080::8:800:200c:417a]:123", ""},
{"http://[1080::8:800:200c:417a]:123/config", ""},
{"https://fd2e:6f44:5dd8:c956::14:31187", ""},
{"https://fd2e:6f44:5dd8:c956::14:31187/ignition", ""},
{
"http://[1080:0:0:0:8:800:200C:417A]:8888 ",
"URL 'http://[1080:0:0:0:8:800:200C:417A]:8888 ' format is not valid: parse \"http://[1080:0:0:0:8:800:200C:417A]:8888 \": invalid port \":8888 \" after host",
Expand Down Expand Up @@ -130,6 +132,21 @@ var _ = Describe("URL validations", func() {
})
})

Context("NormalizeHTTPURL", func() {
It("brackets unbracketed IPv6 ignition URLs", func() {
Expect(NormalizeHTTPURL("https://fd2e:6f44:5dd8:c956::14:31187")).
To(Equal("https://[fd2e:6f44:5dd8:c956::14]:31187"))
Expect(NormalizeHTTPURL("https://fd2e:6f44:5dd8:c956::14:31187/ignition")).
To(Equal("https://[fd2e:6f44:5dd8:c956::14]:31187/ignition"))
})

It("leaves IPv4 and already bracketed IPv6 URLs unchanged", func() {
Expect(NormalizeHTTPURL("https://1.2.3.4:555/ignition")).To(Equal("https://1.2.3.4:555/ignition"))
Expect(NormalizeHTTPURL("http://[1080::8:800:200c:417a]:123/config")).
To(Equal("http://[1080::8:800:200c:417a]:123/config"))
})
})

DescribeTable("ValidateNoProxyFormat",
func(noProxy string, expectSuccess bool) {
err := ValidateNoProxyFormat(noProxy)
Expand Down