MCO-2208: MCO-2125: Migrate mco registry units#6079
Open
ptalgulk01 wants to merge 3 commits into
Open
Conversation
Migrated 9 test cases from mco_registry.go: - 43405: node drain is not needed for mirror config change - 42682: change container registry config on ocp 4.6+ - 42680: change pull secret in the openshift-config namespace - 52520: Configure unqualified-search-registries - 57595: Use empty pull-secret - 61555: ImageDigestMirrorSet test - 61558: ImageTagMirrorSet test - 66046: Check image registry certificates - 68736: machine config server supports bootstrap with IR certs Added helper functions and utilities: - Added MCDCrioReloadedRegexp constant to const.go - Added GetClusterVersion to util/clusters.go - Added NewImageTagMirrorSet, skipTestIfExtensionsAreUsed, GetImageRegistryCertificates, GetManagedMergedTrustedImageRegistryCertificates, and GetDataFromConfigMap to util.go - Added skipTestIfClusterVersion to mco.go - Added GetExtensions method to MachineConfig in machineconfig.go Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added bootstrap package for SSH connectivity to bootstrap nodes: - util/bootstrap/bootstrap.go: Core bootstrap infrastructure - util/bootstrap/aws.go: AWS-specific bootstrap instance discovery - util/bootstrap/azure.go: Azure-specific bootstrap instance discovery - util/ssh_client.go: SSH client utilities for remote command execution Migrated test case from mco_units.go: - 53960: No failed units in the bootstrap machine - Platform-specific test for AWS and Azure - Validates systemd units status on bootstrap machine - Uses SSH to connect and run systemctl commands This infrastructure enables testing bootstrap machine state and configuration, particularly for validating systemd unit health during cluster deployment. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adjusted function order to match otp3 source files: - mco.go: Moved skipTestIfClusterVersion to come after skipTestIfRHELVersion (was at end of file, now follows source order from mco.go line 1424) - machineconfig.go: Moved GetExtensions to come before GetIgnitionVersion (follows source order from machineconfig.go line 117) This maintains consistency with the source repository structure and makes future migrations easier to track. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@ptalgulk01: This pull request references MCO-2208 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR introduces comprehensive test infrastructure for the Machine Config Operator, adding bootstrap machine discovery and SSH utilities, version-aware test skipping, and a large registry configuration test suite covering ImageContentSourcePolicy, mirrors, pull-secrets, and certificates. ChangesMCO Registry and Bootstrap Test Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 6❌ Failed checks (5 warnings, 1 inconclusive)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
test/extended-priv/mco_registry.go (1)
185-185: ⚡ Quick winAvoid the
bash -cshell wrapper for the file copy.
secretFile/newSecretFileare generated locally, so this is not exploitable today, but invokingbash -c "cp …"with string concatenation is a footgun if either path ever contains a space or shell metacharacter, and it tripped the static analyzer (coderabbit.command-injection.go-exec-command). Invokecpdirectly (or useio.Copyoveros.Open/os.Create).♻️ Proposed refactor
- _, copyErr := exec.Command("bash", "-c", "cp "+secretFile+" "+newSecretFile).Output() + _, copyErr := exec.Command("cp", secretFile, newSecretFile).Output()🤖 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 `@test/extended-priv/mco_registry.go` at line 185, Replace the unsafe shell-invocation exec.Command call that builds a "bash -c" string with a direct file-copy implementation: either call exec.Command("cp", secretFile, newSecretFile) to invoke cp without a shell or, preferably, open secretFile and newSecretFile with os.Open/os.Create and copy contents using io.Copy; update the site where exec.Command("bash", "-c", "cp "+secretFile+" "+newSecretFile) is used to reference secretFile and newSecretFile directly to avoid shell interpolation and satisfy the static analyzer.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/extended-priv/mco_registry.go`:
- Around line 66-67: The assertion message uses a malformed format verb ("%") so
the node name from node.GetName() will not be printed and fmt will error; update
the format string in the o.Expect(...).To(o.BeFalse(...)) call to use a proper
verb (e.g. "%s") or otherwise build the message with the node name, referencing
the existing call sites node.IsCordoned(), o.Expect, o.BeFalse(), and
node.GetName() so the node name is interpolated correctly into the error
message.
- Around line 545-547: The failure message for the assertion using nodeEvents
and HaveEventsSequence("Drain") is inverted; update the message passed to
o.Expect(...).To(HaveEventsSequence("Drain"), ...) to reflect that we expected a
Drain event but none occurred (e.g. "Error, expected a Drain event to be
triggered but it wasn't") so the log correctly describes the failing condition
for the positive assertion in the nodeEvents/HaveEventsSequence call.
- Around line 627-636: The assertion messages for the checks using
GetManagedMergedTrustedImageRegistryCertificates reference "%s" but don't supply
certFile, causing missing-format tokens; update the
o.Eventually(...).Should(...) and the final o.Expect(...).To(...) calls to
include the certFile in their failure messages (e.g., pass certFile as the
format arg or pre-format the message with fmt.Sprintf), ensure the message still
names merged-trusted-image-registry-ca and the certFile/ certValue variables are
used to construct the assertion text, and keep using
GetManagedMergedTrustedImageRegistryCertificates(oc.AsAdmin()) with the separate
explicit call that checks err as shown.
- Around line 324-334: The master assertions incorrectly reference the worker
and omit format args: in the HaveEventsSequence assertion replace
firstUpdatedWorker.GetName() with firstUpdatedMaster.GetName(), and for the two
o.Expect(...BeTemporally(...)) failure messages add the missing node name
arguments—pass firstUpdatedWorker.GetName() to the worker uptime assertion and
firstUpdatedMaster.GetName() to the master uptime assertion so the "%s"
placeholders are satisfied; look for symbols firstUpdatedMaster.GetEvents,
HaveEventsSequence, firstUpdatedWorker.GetUptime, and
firstUpdatedMaster.GetUptime to locate the lines to edit.
In `@test/extended-priv/mco.go`:
- Around line 86-95: The helper skipTestIfClusterVersion currently calls
o.Expect(err).NotTo(...), which reports failures at this helper; change it to
use o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred()) so failures point to the
caller; keep the rest of the logic (calls to exutil.GetClusterVersion,
CompareVersions and g.Skip with clusterVersion, operator, constraintVersion)
unchanged.
In `@test/extended-priv/util/bootstrap/azure.go`:
- Around line 19-22: The code discards the error returned by
oc.WithoutNamespace().AsAdmin().Run("get").Args(...).Output(), which can leave
infraName empty and produce a misleading bootstrapName and InstanceNotFound
name; change the call that assigns infraName to capture the error (e.g.,
infraName, err := ...Output()), check err and if non-nil log it with context via
logger.Errorf or return a more accurate error, and only construct bootstrapName
and return &InstanceNotFound{bootstrapName} when you have a reliable infraName
(or fall back to a clear placeholder like "<unknown-infra>"); update references
to infraName, bootstrapName, and the InstanceNotFound return accordingly.
In `@test/extended-priv/util/clusters.go`:
- Around line 23-32: GetClusterVersion currently splits clusterBuild into
splitValues and directly indexes [0] and [1], which can panic if the version is
malformed; add bounds checking after strings.Split to verify len(splitValues) >=
2 and return a descriptive error (including clusterBuild) if not, otherwise
construct clusterVersion from splitValues[0] + "." + splitValues[1] and return
as before; ensure you still return the original err (or nil) on success.
In `@test/extended-priv/util/ssh_client.go`:
- Around line 60-86: The SSH dial address in SshClient.RunOutput is built with
fmt.Sprintf("%v:%v", sshClient.Host, sshClient.Port) which breaks for IPv6;
change the address construction to use net.JoinHostPort(sshClient.Host,
strconv.Itoa(sshClient.Port)) (importing net and strconv) and pass that result
to ssh.Dial so IPv4 and IPv6 hosts are handled correctly.
---
Nitpick comments:
In `@test/extended-priv/mco_registry.go`:
- Line 185: Replace the unsafe shell-invocation exec.Command call that builds a
"bash -c" string with a direct file-copy implementation: either call
exec.Command("cp", secretFile, newSecretFile) to invoke cp without a shell or,
preferably, open secretFile and newSecretFile with os.Open/os.Create and copy
contents using io.Copy; update the site where exec.Command("bash", "-c", "cp
"+secretFile+" "+newSecretFile) is used to reference secretFile and
newSecretFile directly to avoid shell interpolation and satisfy the static
analyzer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a7674f12-1469-4c50-9a47-96179fd4fc59
📒 Files selected for processing (11)
test/extended-priv/const.gotest/extended-priv/machineconfig.gotest/extended-priv/mco.gotest/extended-priv/mco_registry.gotest/extended-priv/mco_units.gotest/extended-priv/util.gotest/extended-priv/util/bootstrap/aws.gotest/extended-priv/util/bootstrap/azure.gotest/extended-priv/util/bootstrap/bootstrap.gotest/extended-priv/util/clusters.gotest/extended-priv/util/ssh_client.go
Comment on lines
+66
to
+67
| o.Expect(node.IsCordoned()).To(o.BeFalse(), "% is tainted and cordoned. There should be no cordoned worker node after applying the configuration.", | ||
| node.GetName()) |
There was a problem hiding this comment.
Broken format specifier in assertion message.
The format string uses % (without verb) instead of %s, so the node name will not be rendered and Go's fmt will report a malformed verb on failure.
🐛 Proposed fix
- o.Expect(node.IsCordoned()).To(o.BeFalse(), "% is tainted and cordoned. There should be no cordoned worker node after applying the configuration.",
- node.GetName())
+ o.Expect(node.IsCordoned()).To(o.BeFalse(), "%s is tainted and cordoned. There should be no cordoned worker node after applying the configuration.",
+ node.GetName())📝 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
| o.Expect(node.IsCordoned()).To(o.BeFalse(), "% is tainted and cordoned. There should be no cordoned worker node after applying the configuration.", | |
| node.GetName()) | |
| o.Expect(node.IsCordoned()).To(o.BeFalse(), "%s is tainted and cordoned. There should be no cordoned worker node after applying the configuration.", | |
| node.GetName()) |
🤖 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 `@test/extended-priv/mco_registry.go` around lines 66 - 67, The assertion
message uses a malformed format verb ("%") so the node name from node.GetName()
will not be printed and fmt will error; update the format string in the
o.Expect(...).To(o.BeFalse(...)) call to use a proper verb (e.g. "%s") or
otherwise build the message with the node name, referencing the existing call
sites node.IsCordoned(), o.Expect, o.BeFalse(), and node.GetName() so the node
name is interpolated correctly into the error message.
Comment on lines
+324
to
+334
| exutil.By("Verify that a drain and reboot events were triggered for master node") | ||
| mEvents, meErr := firstUpdatedMaster.GetEvents() | ||
| o.Expect(meErr).ShouldNot(o.HaveOccurred(), "Error getting drain events for node %s", firstUpdatedMaster.GetName()) | ||
| o.Expect(mEvents).To(HaveEventsSequence("Drain", "Reboot"), | ||
| "Error, the expected sequence of events is not found in node %s", firstUpdatedWorker.GetName()) | ||
|
|
||
| exutil.By("Verify that the node was actually rebooted") | ||
| o.Expect(firstUpdatedWorker.GetUptime()).Should(o.BeTemporally(">", startTime), | ||
| "The node %s should have been rebooted after the configurion. Uptime didnt happen after start config time.") | ||
| o.Expect(firstUpdatedMaster.GetUptime()).Should(o.BeTemporally(">", startTime), | ||
| "The node %s should have been rebooted after the configurion. Uptime didnt happen after start config time.") |
There was a problem hiding this comment.
Master assertions reference the worker node and miss format args.
Two issues:
- Line 328: failure message for the master event sequence reports
firstUpdatedWorker.GetName()— should befirstUpdatedMaster.GetName(). - Lines 332 and 334: the messages contain
%sbut no arguments are passed, so failures will print a%!s(MISSING)token instead of the node name.
🐛 Proposed fix
o.Expect(mEvents).To(HaveEventsSequence("Drain", "Reboot"),
- "Error, the expected sequence of events is not found in node %s", firstUpdatedWorker.GetName())
+ "Error, the expected sequence of events is not found in node %s", firstUpdatedMaster.GetName())
exutil.By("Verify that the node was actually rebooted")
o.Expect(firstUpdatedWorker.GetUptime()).Should(o.BeTemporally(">", startTime),
- "The node %s should have been rebooted after the configurion. Uptime didnt happen after start config time.")
+ "The node %s should have been rebooted after the configuration. Uptime didn't happen after start config time.",
+ firstUpdatedWorker.GetName())
o.Expect(firstUpdatedMaster.GetUptime()).Should(o.BeTemporally(">", startTime),
- "The node %s should have been rebooted after the configurion. Uptime didnt happen after start config time.")
+ "The node %s should have been rebooted after the configuration. Uptime didn't happen after start config time.",
+ firstUpdatedMaster.GetName())📝 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
| exutil.By("Verify that a drain and reboot events were triggered for master node") | |
| mEvents, meErr := firstUpdatedMaster.GetEvents() | |
| o.Expect(meErr).ShouldNot(o.HaveOccurred(), "Error getting drain events for node %s", firstUpdatedMaster.GetName()) | |
| o.Expect(mEvents).To(HaveEventsSequence("Drain", "Reboot"), | |
| "Error, the expected sequence of events is not found in node %s", firstUpdatedWorker.GetName()) | |
| exutil.By("Verify that the node was actually rebooted") | |
| o.Expect(firstUpdatedWorker.GetUptime()).Should(o.BeTemporally(">", startTime), | |
| "The node %s should have been rebooted after the configurion. Uptime didnt happen after start config time.") | |
| o.Expect(firstUpdatedMaster.GetUptime()).Should(o.BeTemporally(">", startTime), | |
| "The node %s should have been rebooted after the configurion. Uptime didnt happen after start config time.") | |
| exutil.By("Verify that a drain and reboot events were triggered for master node") | |
| mEvents, meErr := firstUpdatedMaster.GetEvents() | |
| o.Expect(meErr).ShouldNot(o.HaveOccurred(), "Error getting drain events for node %s", firstUpdatedMaster.GetName()) | |
| o.Expect(mEvents).To(HaveEventsSequence("Drain", "Reboot"), | |
| "Error, the expected sequence of events is not found in node %s", firstUpdatedMaster.GetName()) | |
| exutil.By("Verify that the node was actually rebooted") | |
| o.Expect(firstUpdatedWorker.GetUptime()).Should(o.BeTemporally(">", startTime), | |
| "The node %s should have been rebooted after the configuration. Uptime didn't happen after start config time.", | |
| firstUpdatedWorker.GetName()) | |
| o.Expect(firstUpdatedMaster.GetUptime()).Should(o.BeTemporally(">", startTime), | |
| "The node %s should have been rebooted after the configuration. Uptime didn't happen after start config time.", | |
| firstUpdatedMaster.GetName()) |
🤖 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 `@test/extended-priv/mco_registry.go` around lines 324 - 334, The master
assertions incorrectly reference the worker and omit format args: in the
HaveEventsSequence assertion replace firstUpdatedWorker.GetName() with
firstUpdatedMaster.GetName(), and for the two o.Expect(...BeTemporally(...))
failure messages add the missing node name arguments—pass
firstUpdatedWorker.GetName() to the worker uptime assertion and
firstUpdatedMaster.GetName() to the master uptime assertion so the "%s"
placeholders are satisfied; look for symbols firstUpdatedMaster.GetEvents,
HaveEventsSequence, firstUpdatedWorker.GetUptime, and
firstUpdatedMaster.GetUptime to locate the lines to edit.
Comment on lines
+545
to
+547
| exutil.By("Check that drain events were triggered") | ||
| o.Expect(nodeEvents).To(HaveEventsSequence("Drain"), "Error, a Drain event was triggered but it shouldn't") | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
Inverted assertion message.
The assertion verifies a Drain event was triggered, but the failure message reads "a Drain event was triggered but it shouldn't" — that wording belongs to the opposite case (NotTo). On failure this will mislead the triage.
🐛 Proposed fix
- o.Expect(nodeEvents).To(HaveEventsSequence("Drain"), "Error, a Drain event was triggered but it shouldn't")
+ o.Expect(nodeEvents).To(HaveEventsSequence("Drain"), "Error, a Drain event was expected but none was triggered")🤖 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 `@test/extended-priv/mco_registry.go` around lines 545 - 547, The failure
message for the assertion using nodeEvents and HaveEventsSequence("Drain") is
inverted; update the message passed to
o.Expect(...).To(HaveEventsSequence("Drain"), ...) to reflect that we expected a
Drain event but none occurred (e.g. "Error, expected a Drain event to be
triggered but it wasn't") so the log correctly describes the failing condition
for the positive assertion in the nodeEvents/HaveEventsSequence call.
Comment on lines
+627
to
+636
| exutil.By(fmt.Sprintf("Check that the file %s has been added to the managed merged trusted image registry configmap", certFile)) | ||
|
|
||
| o.Eventually(GetManagedMergedTrustedImageRegistryCertificates, "20s", "10s").WithArguments(oc.AsAdmin()).Should(o.HaveKey(certFile), | ||
| "The certificate for file %s has not been included in the configmap merged-trusted-image-registry-ca -n openshift-config-managed") | ||
|
|
||
| mmtImageRegistryCert, err := GetManagedMergedTrustedImageRegistryCertificates(oc.AsAdmin()) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting managed merged trusted image registry certificates values") | ||
|
|
||
| o.Expect(mmtImageRegistryCert[certFile] == certValue).To(o.BeTrue(), | ||
| "The certificate in file %s was added to configmap merged-trusted-image-registry-ca -n openshift-config-managed but it has the wrong content") |
There was a problem hiding this comment.
Missing format arguments in assertion messages.
The messages at lines 630 and 636 include %s but pass no arguments, so failures will print %!s(MISSING) tokens instead of the certificate filename. Also note the Eventually at line 629 passes GetManagedMergedTrustedImageRegistryCertificates whose error return is being asserted on a HaveKey matcher — confirm this works as intended (the second return value error is ignored by Eventually's polled-function form, which is acceptable here, but worth a quick sanity check).
🐛 Proposed fix
- o.Eventually(GetManagedMergedTrustedImageRegistryCertificates, "20s", "10s").WithArguments(oc.AsAdmin()).Should(o.HaveKey(certFile),
- "The certificate for file %s has not been included in the configmap merged-trusted-image-registry-ca -n openshift-config-managed")
+ o.Eventually(GetManagedMergedTrustedImageRegistryCertificates, "20s", "10s").WithArguments(oc.AsAdmin()).Should(o.HaveKey(certFile),
+ "The certificate for file %s has not been included in the configmap merged-trusted-image-registry-ca -n openshift-config-managed", certFile)
@@
- o.Expect(mmtImageRegistryCert[certFile] == certValue).To(o.BeTrue(),
- "The certificate in file %s was added to configmap merged-trusted-image-registry-ca -n openshift-config-managed but it has the wrong content")
+ o.Expect(mmtImageRegistryCert[certFile] == certValue).To(o.BeTrue(),
+ "The certificate in file %s was added to configmap merged-trusted-image-registry-ca -n openshift-config-managed but it has the wrong content", certFile)🤖 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 `@test/extended-priv/mco_registry.go` around lines 627 - 636, The assertion
messages for the checks using GetManagedMergedTrustedImageRegistryCertificates
reference "%s" but don't supply certFile, causing missing-format tokens; update
the o.Eventually(...).Should(...) and the final o.Expect(...).To(...) calls to
include the certFile in their failure messages (e.g., pass certFile as the
format arg or pre-format the message with fmt.Sprintf), ensure the message still
names merged-trusted-image-registry-ca and the certFile/ certValue variables are
used to construct the assertion text, and keep using
GetManagedMergedTrustedImageRegistryCertificates(oc.AsAdmin()) with the separate
explicit call that checks err as shown.
Comment on lines
+86
to
+95
| // skipTestIfClusterVersion skips the test case if the provided version matches the constraints. | ||
| func skipTestIfClusterVersion(oc *exutil.CLI, operator, constraintVersion string) { | ||
| clusterVersion, _, err := exutil.GetClusterVersion(oc) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| if CompareVersions(clusterVersion, operator, constraintVersion) { | ||
| g.Skip(fmt.Sprintf("Test case skipped because current cluster version %s %s %s", | ||
| clusterVersion, operator, constraintVersion)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Use ExpectWithOffset(1, ...) for clearer failure context.
This helper function uses o.Expect without an offset, which means test failures will report the line inside this helper rather than the caller's location. For consistency with other helpers in this file (e.g., skipTestIfRHELVersion on line 79), use o.ExpectWithOffset(1, err) instead.
📍 Proposed fix
func skipTestIfClusterVersion(oc *exutil.CLI, operator, constraintVersion string) {
clusterVersion, _, err := exutil.GetClusterVersion(oc)
- o.Expect(err).NotTo(o.HaveOccurred())
+ o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred())
if CompareVersions(clusterVersion, operator, constraintVersion) {
g.Skip(fmt.Sprintf("Test case skipped because current cluster version %s %s %s",
clusterVersion, operator, constraintVersion))
}
}📝 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
| // skipTestIfClusterVersion skips the test case if the provided version matches the constraints. | |
| func skipTestIfClusterVersion(oc *exutil.CLI, operator, constraintVersion string) { | |
| clusterVersion, _, err := exutil.GetClusterVersion(oc) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| if CompareVersions(clusterVersion, operator, constraintVersion) { | |
| g.Skip(fmt.Sprintf("Test case skipped because current cluster version %s %s %s", | |
| clusterVersion, operator, constraintVersion)) | |
| } | |
| } | |
| // skipTestIfClusterVersion skips the test case if the provided version matches the constraints. | |
| func skipTestIfClusterVersion(oc *exutil.CLI, operator, constraintVersion string) { | |
| clusterVersion, _, err := exutil.GetClusterVersion(oc) | |
| o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred()) | |
| if CompareVersions(clusterVersion, operator, constraintVersion) { | |
| g.Skip(fmt.Sprintf("Test case skipped because current cluster version %s %s %s", | |
| clusterVersion, operator, constraintVersion)) | |
| } | |
| } |
🤖 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 `@test/extended-priv/mco.go` around lines 86 - 95, The helper
skipTestIfClusterVersion currently calls o.Expect(err).NotTo(...), which reports
failures at this helper; change it to use o.ExpectWithOffset(1,
err).NotTo(o.HaveOccurred()) so failures point to the caller; keep the rest of
the logic (calls to exutil.GetClusterVersion, CompareVersions and g.Skip with
clusterVersion, operator, constraintVersion) unchanged.
Comment on lines
+19
to
+22
| infraName, _ := oc.WithoutNamespace().AsAdmin().Run("get").Args("infrastructure", "cluster", "-o=jsonpath={.status.infrastructureName}").Output() | ||
| bootstrapName := infraName + "-bootstrap" | ||
| logger.Infof("Azure bootstrap discovery is not yet implemented, skipping bootstrap machine: %s", bootstrapName) | ||
| return nil, &InstanceNotFound{bootstrapName} |
There was a problem hiding this comment.
Discarded error can produce a misleading bootstrap name in logs.
The error from the infrastructure get is silently dropped on Line 19. On failure, infraName will be empty and the log on Line 21 will say "skipping bootstrap machine: -bootstrap", and the returned InstanceNotFound.name will likewise be -bootstrap. Since this stub always returns InstanceNotFound, it's not a correctness bug today, but a small fix would make the log/diagnostic accurate.
Proposed fix
- infraName, _ := oc.WithoutNamespace().AsAdmin().Run("get").Args("infrastructure", "cluster", "-o=jsonpath={.status.infrastructureName}").Output()
+ infraName, err := oc.WithoutNamespace().AsAdmin().Run("get").Args("infrastructure", "cluster", "-o=jsonpath={.status.infrastructureName}").Output()
+ if err != nil {
+ return nil, err
+ }
bootstrapName := infraName + "-bootstrap"📝 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
| infraName, _ := oc.WithoutNamespace().AsAdmin().Run("get").Args("infrastructure", "cluster", "-o=jsonpath={.status.infrastructureName}").Output() | |
| bootstrapName := infraName + "-bootstrap" | |
| logger.Infof("Azure bootstrap discovery is not yet implemented, skipping bootstrap machine: %s", bootstrapName) | |
| return nil, &InstanceNotFound{bootstrapName} | |
| infraName, err := oc.WithoutNamespace().AsAdmin().Run("get").Args("infrastructure", "cluster", "-o=jsonpath={.status.infrastructureName}").Output() | |
| if err != nil { | |
| return nil, err | |
| } | |
| bootstrapName := infraName + "-bootstrap" | |
| logger.Infof("Azure bootstrap discovery is not yet implemented, skipping bootstrap machine: %s", bootstrapName) | |
| return nil, &InstanceNotFound{bootstrapName} |
🤖 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 `@test/extended-priv/util/bootstrap/azure.go` around lines 19 - 22, The code
discards the error returned by
oc.WithoutNamespace().AsAdmin().Run("get").Args(...).Output(), which can leave
infraName empty and produce a misleading bootstrapName and InstanceNotFound
name; change the call that assigns infraName to capture the error (e.g.,
infraName, err := ...Output()), check err and if non-nil log it with context via
logger.Errorf or return a more accurate error, and only construct bootstrapName
and return &InstanceNotFound{bootstrapName} when you have a reliable infraName
(or fall back to a clear placeholder like "<unknown-infra>"); update references
to infraName, bootstrapName, and the InstanceNotFound return accordingly.
Comment on lines
+23
to
+32
| // GetClusterVersion returns the cluster version as string value (Ex: 4.8) and cluster build (Ex: 4.8.0-0.nightly-2021-09-28-165247) | ||
| func GetClusterVersion(oc *CLI) (string, string, error) { | ||
| clusterBuild, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("clusterversion", "-o", "jsonpath={..desired.version}").Output() | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| splitValues := strings.Split(clusterBuild, ".") | ||
| clusterVersion := splitValues[0] + "." + splitValues[1] | ||
| return clusterVersion, clusterBuild, err | ||
| } |
There was a problem hiding this comment.
Add bounds checking to prevent index out of range panic.
Lines 29-30 split the cluster version string and access splitValues[0] and splitValues[1] without validating the slice length. If the cluster version string has fewer than two dots (e.g., malformed version), this will panic with an index out of range error.
🛡️ Proposed fix
func GetClusterVersion(oc *CLI) (string, string, error) {
clusterBuild, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("clusterversion", "-o", "jsonpath={..desired.version}").Output()
if err != nil {
return "", "", err
}
splitValues := strings.Split(clusterBuild, ".")
+ if len(splitValues) < 2 {
+ return "", "", fmt.Errorf("invalid cluster version format: %s", clusterBuild)
+ }
clusterVersion := splitValues[0] + "." + splitValues[1]
return clusterVersion, clusterBuild, err
}📝 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
| // GetClusterVersion returns the cluster version as string value (Ex: 4.8) and cluster build (Ex: 4.8.0-0.nightly-2021-09-28-165247) | |
| func GetClusterVersion(oc *CLI) (string, string, error) { | |
| clusterBuild, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("clusterversion", "-o", "jsonpath={..desired.version}").Output() | |
| if err != nil { | |
| return "", "", err | |
| } | |
| splitValues := strings.Split(clusterBuild, ".") | |
| clusterVersion := splitValues[0] + "." + splitValues[1] | |
| return clusterVersion, clusterBuild, err | |
| } | |
| // GetClusterVersion returns the cluster version as string value (Ex: 4.8) and cluster build (Ex: 4.8.0-0.nightly-2021-09-28-165247) | |
| func GetClusterVersion(oc *CLI) (string, string, error) { | |
| clusterBuild, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("clusterversion", "-o", "jsonpath={..desired.version}").Output() | |
| if err != nil { | |
| return "", "", err | |
| } | |
| splitValues := strings.Split(clusterBuild, ".") | |
| if len(splitValues) < 2 { | |
| return "", "", fmt.Errorf("invalid cluster version format: %s", clusterBuild) | |
| } | |
| clusterVersion := splitValues[0] + "." + splitValues[1] | |
| return clusterVersion, clusterBuild, err | |
| } |
🤖 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 `@test/extended-priv/util/clusters.go` around lines 23 - 32, GetClusterVersion
currently splits clusterBuild into splitValues and directly indexes [0] and [1],
which can panic if the version is malformed; add bounds checking after
strings.Split to verify len(splitValues) >= 2 and return a descriptive error
(including clusterBuild) if not, otherwise construct clusterVersion from
splitValues[0] + "." + splitValues[1] and return as before; ensure you still
return the original err (or nil) on success.
Comment on lines
+60
to
+86
| func (sshClient *SshClient) RunOutput(cmd string) (string, error) { | ||
| config, err := sshClient.getConfig() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get SSH config: %v", err) | ||
| } | ||
|
|
||
| connection, err := ssh.Dial("tcp", fmt.Sprintf("%v:%v", sshClient.Host, sshClient.Port), config) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to dial %s:%d: %v", sshClient.Host, sshClient.Port, err) | ||
| } | ||
| defer connection.Close() | ||
|
|
||
| session, err := connection.NewSession() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create session: %v", err) | ||
| } | ||
| defer session.Close() | ||
|
|
||
| buf := newSynchronizedBuffer() | ||
| session.Stdout = buf | ||
| session.Stderr = buf | ||
|
|
||
| if err := session.Run(cmd); err != nil { | ||
| return "", fmt.Errorf("failed to run cmd '%s': %v\n%s", cmd, err, buf.String()) | ||
| } | ||
| return buf.String(), nil | ||
| } |
There was a problem hiding this comment.
Use net.JoinHostPort for IPv6 compatibility.
Line 66 constructs the SSH address using fmt.Sprintf("%v:%v", sshClient.Host, sshClient.Port), which does not properly handle IPv6 addresses. IPv6 addresses must be wrapped in brackets (e.g., [2001:db8::1]:22). Use net.JoinHostPort instead to ensure both IPv4 and IPv6 addresses work correctly.
🌐 Proposed fix for IPv6 support
+import (
+ "net"
+ "strconv"
+)
+
func (sshClient *SshClient) RunOutput(cmd string) (string, error) {
config, err := sshClient.getConfig()
if err != nil {
return "", fmt.Errorf("failed to get SSH config: %v", err)
}
- connection, err := ssh.Dial("tcp", fmt.Sprintf("%v:%v", sshClient.Host, sshClient.Port), config)
+ address := net.JoinHostPort(sshClient.Host, strconv.Itoa(sshClient.Port))
+ connection, err := ssh.Dial("tcp", address, config)
if err != nil {
return "", fmt.Errorf("failed to dial %s:%d: %v", sshClient.Host, sshClient.Port, err)
}As per coding guidelines: "Use net.JoinHostPort() for URL construction instead" when building addresses that may contain IPv6 hosts.
🤖 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 `@test/extended-priv/util/ssh_client.go` around lines 60 - 86, The SSH dial
address in SshClient.RunOutput is built with fmt.Sprintf("%v:%v",
sshClient.Host, sshClient.Port) which breaks for IPv6; change the address
construction to use net.JoinHostPort(sshClient.Host,
strconv.Itoa(sshClient.Port)) (importing net and strconv) and pass that result
to ssh.Dial so IPv4 and IPv6 hosts are handled correctly.
Contributor
|
@ptalgulk01: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrate 10 test cases from openshift-tests-private to machine-config-operator repo:
mco_registry.gomco_units.goThis PR continues the migration effort started in #6021 and #5902.
Tests Migrated
Registry Tests (mco_registry.go - 9 tests)
Units Test (mco_units.go - 1 test)
Infrastructure Added
Bootstrap Package (from PR #6027)
test/extended-priv/util/bootstrap/bootstrap.go- Core bootstrap SSH infrastructuretest/extended-priv/util/bootstrap/aws.go- AWS instance discoverytest/extended-priv/util/bootstrap/azure.go- Azure instance discoverytest/extended-priv/util/ssh_client.go- SSH client utilitiesHelper Functions & Methods Added
test/extended-priv/util/clusters.go
GetClusterVersion()- Returns cluster version and buildtest/extended-priv/util.go
NewImageTagMirrorSet()- Create ImageTagMirrorSet resourceskipTestIfExtensionsAreUsed()- Skip tests when extensions are presentGetImageRegistryCertificates()- Retrieve image registry certificatesGetManagedMergedTrustedImageRegistryCertificates()- Get merged trusted certificatesGetDataFromConfigMap()- Extract data from ConfigMapstest/extended-priv/mco.go
skipTestIfClusterVersion()- Skip tests based on cluster versiontest/extended-priv/machineconfig.go
GetExtensions()- Get extensions configured in MachineConfigConstants Added
test/extended-priv/const.go
MCDCrioReloadedRegexp- Regex for detecting CRI-O reloadsNodeDisruptionPolicyFiles- "files"NodeDisruptionPolicyUnits- "units"NodeDisruptionPolicySshkey- "sshkey"Build Verification