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>
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
WalkthroughAdds bootstrap discovery and SSH utilities, cluster-version and extension-aware test helpers, new constants and regexes, a thread-safe SSH client, AWS/Azure bootstrap providers, a comprehensive MCO registry Ginkgo test suite, and supporting testdata/templates. ChangesMCO Registry and Bootstrap Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Contributor
Author
ptalgulk01
force-pushed
the
migrate-mco-registry-units
branch
from
853ccf5 to
e8a4eca
Compare
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>
ptalgulk01
force-pushed
the
migrate-mco-registry-units
branch
from
e8a4eca to
64d737a
Compare
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