MCO-2209 MCO-2213 MCO-2233: Migrate security, daemon, and kernel TCs from mco.go#6021
Open
ptalgulk01 wants to merge 1 commit into
Open
MCO-2209 MCO-2213 MCO-2233: Migrate security, daemon, and kernel TCs from mco.go#6021ptalgulk01 wants to merge 1 commit into
ptalgulk01 wants to merge 1 commit into
Conversation
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-2209 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. This pull request references MCO-2213 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. This pull request references MCO-2233 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 (13)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughAdds helpers, constructors, and parsing utilities and appends ~10+ new disruptive/long-duration extended-priv Ginkgo tests and YAML test templates for Machine Config Operator scenarios (security hardening, FIPS, cert rotation, daemon resilience, kernel args, CoreDNS/osImage flows). ChangesExtended MCO extended-priv test flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 6❌ Failed checks (6 warnings)
✅ Passed checks (6 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: 11
🤖 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/machineconfigpool.go`:
- Around line 1086-1092: GetCertsExpiry currently calls json.Unmarshal on
expiryString which can be empty (mcp.Get returns ""), causing "unexpected end of
JSON input"; before calling json.Unmarshal in GetCertsExpiry, check if
strings.TrimSpace(expiryString) == "" (or expiryString == "") and return an
empty slice (or nil) and nil error immediately; then proceed to json.Unmarshal
into certsExp as before. Ensure you reference the certsExp variable and the
GetCertsExpiry function when making this change and add a strings import if
needed.
In `@test/extended-priv/mco_daemon.go`:
- Around line 414-417: The test currently checks mcc logs with
o.Eventually(mcc.GetLogs...).ShouldNot(o.And(resourceNotFoundErrorMsg,
listFailureErrorMsg)) which only fails when both messages are present; change
the assertion to use Or so it fails if either resourceNotFoundErrorMsg or
listFailureErrorMsg appears (i.e., replace the And(...) matcher with Or(...) in
the ShouldNot call referencing mcc.GetLogs, resourceNotFoundErrorMsg and
listFailureErrorMsg).
- Around line 350-353: Defer cleanup() is called before verifying err from
GetCompactCompatibleOrCustomPool which can cause a nil-func panic; move the
defer until after validating the call (i.e., after the
o.Expect(err).NotTo(o.HaveOccurred()) check) and/or assert cleanup is non-nil
before deferring it so that cleanup is only deferred when
GetCompactCompatibleOrCustomPool successfully returned a valid cleanup function.
In `@test/extended-priv/mco_kernel.go`:
- Around line 387-401: The Patch calls (mMc.Patch, wMc.Patch) and possibly other
operations are ignoring returned errors; update the FIPS-disable test path to
capture and check each Patch call's error return and fail the test immediately
on error (e.g., use t.Fatalf/require.NoError/ExpectNoError) so the test does not
continue asserting degraded state when a patch failed; do this for
mMc.Patch(...) (both fips=true revert and fips=false), wMc.Patch(...) (both
places), and ensure any call to
wMcp.RecoverFromDegraded()/mMcp.RecoverFromDegraded() also surfaces errors if
they return any.
In `@test/extended-priv/mco_security.go`:
- Around line 1119-1121: The Eventually call using
o.Eventually(certSecret.GetDataValueOrFail).WithArguments("tls.crt").ShouldNot(o.Equal(initialCert))
needs an explicit timeout and polling interval to avoid flakes; update the chain
on certSecret.GetDataValueOrFail / initialCert to include e.g.
.WithTimeout(2*time.Minute).WithPolling(5*time.Second) (or other suitable
duration values) before ShouldNot so the test waits long enough for certificate
rotation.
- Around line 992-1007: The test's comment and intent state ">= 1.15" but the
assertion uses BeNumerically(">", 1.15), excluding 1.15; update the assertion on
goVersion (returned by getGoVersion) to
o.Expect(goVersion).Should(o.BeNumerically(">=", 1.15)) and adjust the nearby
logger message (the logger.Infof call that prints goVersion) or the preceding
comment to consistently state ">= 1.15" so the intent, log output, and the
o.Expect check (in the test block that calls getCommitID and getGoVersion for
machine-config-operator) all match.
- Around line 1228-1240: The test is order-dependent because it compares
certsExpiry[i] to ccKCertsInfo[i]; change the loop to match entries by a stable
key (e.g., Bundle or Subject) instead of by index: build an index map from
certsExpiry (map[bundle]expiry or map[subject]expiry) and then iterate
ccKCertsInfo, lookup the corresponding cert expiry from that map and assert
Bundle, Expiry, and Subject equality for the matched entry; reference
ccKCertsInfo, certsExpiry, and certExpry when locating the code to modify.
In `@test/extended-priv/mco.go`:
- Around line 188-196: The loop over pods can panic because nodes[i] is accessed
without ensuring i < len(nodes); add a bounds check similar to the timestamps
guard (e.g., ensure i < len(nodes) before using nodes[i]) or skip/continue when
the pod's corresponding nodeName is missing/empty so you never index past nodes;
update the block that checks strings.HasPrefix(pod, "coredns-") &&
strings.Contains(nodes[i], "worker") to first verify i < len(nodes) (or validate
nodeName per-pod) before inspecting nodes[i], leaving the existing timestamps
check intact.
In `@test/extended-priv/util.go`:
- Around line 1329-1342: The getGoVersion function currently shells out
(exec.Command with bash -c) and slices the curl output unsafely; replace that
with a safe HTTP GET using net/http (e.g., build the URL with fmt.Sprintf and
use http.Get), check for non-200 responses and read the body into a string, then
scan lines to find the first line starting with "go" (use strings.HasPrefix) and
validate its length before slicing; split that line by whitespace or "." to
extract the X and Y components (ensure at least two components), trim
newline/space, and use strconv.ParseFloat on the "X.Y" string, returning clear
errors on HTTP failures, missing go line, or bad version format—do all parsing
in getGoVersion (no shelling out) to avoid shell injection and
index-out-of-range panics.
- Around line 1316-1327: The getCommitID function builds a shell command with
exec.Command("bash", "-c", ...) using outFilePath and component which permits
shell injection; replace the shell pipeline with native Go: use the outFilePath
returned by OutputToFile to open and read the file, iterate its lines, find the
line that contains the exact component string (use strings.Contains or fields
parsing to match the component field), split the matching line into fields
(e.g., strings.Fields) and extract the third field as the commit ID, trim
newline with strings.TrimSpace and return it; keep error returns from
getPullSecret and oc.AsAdmin().WithoutNamespace().Run(...).OutputToFile and
propagate file I/O or parsing errors from getCommitID.
In `@test/extended-priv/util/clusters.go`:
- Around line 99-101: The code assumes clusterBuild and splitValues have at
least two dot-separated segments and that the jsonpath returns a single token;
add defensive checks in the function that computes clusterVersion: after
retrieving clusterBuild (from the oc get clusterversion jsonpath call) trim
whitespace, ensure it is non-empty and does not contain multiple space-separated
tokens (return an explicit error if it does), then call strings.Split and verify
len(splitValues) >= 2 before accessing splitValues[0] and splitValues[1]; if the
format is unexpected return a clear error instead of panicking. Also tighten the
jsonpath used to fetch desired.version (avoid recursive `..desired.version`) so
it returns a single precise value, and update any callers that expect
(clusterVersion, clusterBuild, err) accordingly.
🪄 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: acaa2d9f-bcba-4746-9d9e-327dcfbc9367
📒 Files selected for processing (13)
test/extended-priv/const.gotest/extended-priv/controller.gotest/extended-priv/machineconfig.gotest/extended-priv/machineconfigpool.gotest/extended-priv/mco.gotest/extended-priv/mco_daemon.gotest/extended-priv/mco_kernel.gotest/extended-priv/mco_security.gotest/extended-priv/testdata/files/add-gpg-pub-key.yamltest/extended-priv/testdata/files/change-fips.yamltest/extended-priv/testdata/files/change-policy-json.yamltest/extended-priv/util.gotest/extended-priv/util/clusters.go
Comment on lines
+1086
to
+1092
| var certsExp []CertExpiry | ||
|
|
||
| jsonerr := json.Unmarshal([]byte(expiryString), &certsExp) | ||
|
|
||
| if jsonerr != nil { | ||
| return nil, jsonerr | ||
| } |
There was a problem hiding this comment.
GetCertsExpiry returns a spurious JSON error when the MCP has no certExpirys
When status.certExpirys is absent, mcp.Get(...) returns an empty string with no error. json.Unmarshal([]byte(""), ...) then returns unexpected end of JSON input, making callers believe the MCP is broken rather than simply having no certificate data yet.
🛡️ Proposed fix
var certsExp []CertExpiry
+if expiryString == "" {
+ return certsExp, nil
+}
jsonerr := json.Unmarshal([]byte(expiryString), &certsExp)📝 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
| var certsExp []CertExpiry | |
| jsonerr := json.Unmarshal([]byte(expiryString), &certsExp) | |
| if jsonerr != nil { | |
| return nil, jsonerr | |
| } | |
| var certsExp []CertExpiry | |
| if expiryString == "" { | |
| return certsExp, nil | |
| } | |
| jsonerr := json.Unmarshal([]byte(expiryString), &certsExp) | |
| if jsonerr != nil { | |
| return nil, jsonerr | |
| } |
🤖 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/machineconfigpool.go` around lines 1086 - 1092,
GetCertsExpiry currently calls json.Unmarshal on expiryString which can be empty
(mcp.Get returns ""), causing "unexpected end of JSON input"; before calling
json.Unmarshal in GetCertsExpiry, check if strings.TrimSpace(expiryString) == ""
(or expiryString == "") and return an empty slice (or nil) and nil error
immediately; then proceed to json.Unmarshal into certsExp as before. Ensure you
reference the certsExp variable and the GetCertsExpiry function when making this
change and add a strings import if needed.
Comment on lines
+414
to
+417
| o.Eventually(mcc.GetLogs, "1m", "20s").ShouldNot(o.And( | ||
| o.ContainSubstring(resourceNotFoundErrorMsg), | ||
| o.ContainSubstring(listFailureErrorMsg)), | ||
| "MCC is reporting that some resources cannot be found or listed") |
There was a problem hiding this comment.
Use Or for independent MCC log error conditions
Line 414 currently uses ShouldNot(o.And(...)), which still passes when only one failure signal is present. The intent here is to fail if either error appears.
Suggested fix
- o.Eventually(mcc.GetLogs, "1m", "20s").ShouldNot(o.And(
+ o.Eventually(mcc.GetLogs, "1m", "20s").ShouldNot(o.Or(
o.ContainSubstring(resourceNotFoundErrorMsg),
o.ContainSubstring(listFailureErrorMsg)),
"MCC is reporting that some resources cannot be found or listed")🤖 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_daemon.go` around lines 414 - 417, The test currently
checks mcc logs with
o.Eventually(mcc.GetLogs...).ShouldNot(o.And(resourceNotFoundErrorMsg,
listFailureErrorMsg)) which only fails when both messages are present; change
the assertion to use Or so it fails if either resourceNotFoundErrorMsg or
listFailureErrorMsg appears (i.e., replace the And(...) matcher with Or(...) in
the ShouldNot call referencing mcc.GetLogs, resourceNotFoundErrorMsg and
listFailureErrorMsg).
Contributor
Author
There was a problem hiding this comment.
We need to check both the errors here
There was a problem hiding this comment.
@ptalgulk01, understood! The ShouldNot(o.And(...)) is intentional here — the test should only flag a problem when both "the server could not find the requested resource" and "failed to list" appear together in the MCC logs, as their co-occurrence is what indicates the specific resource-listing failure. Having either message alone may be acceptable. Thanks for the clarification!
ptalgulk01
force-pushed
the
migrate-mco-security-daemon-kernel
branch
from
aacf393 to
95df15b
Compare
…from otp3 mco.go Migrates 13 test cases from openshift-tests-private/test/extended/mco/mco.go into existing destination test suite files: - mco_security.go: 43278, 46965, 47062, 62084, 65208, 66436, 67395 - mco_daemon.go: 68684, 82299, 83134, 83943 - mco_kernel.go: 72132, 72135 New helpers/methods added to support these TCs: - getCommitID, getGoVersion (util.go) - getCoreDNSWorkerPodCreationTime (mco.go) - Controller.RemovePod (controller.go) - MachineConfigPool.GetCertsExpiry (machineconfigpool.go) - NewMachineConfigList (machineconfig.go) - exutil.GetClusterVersion (util/clusters.go) - MCDCrioReloadedRegexp constant (const.go) Template files added: add-gpg-pub-key.yaml, change-policy-json.yaml, change-fips.yaml Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ptalgulk01
force-pushed
the
migrate-mco-security-daemon-kernel
branch
from
95df15b to
ffce562
Compare
Contributor
|
@ptalgulk01: all tests passed! 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.
Migrates 13 test cases from openshift-tests-private/test/extended/mco/mco.go into existing destination test suite files:
New helpers/methods added to support these TCs:
Template files added: add-gpg-pub-key.yaml, change-policy-json.yaml, change-fips.yaml
Summary by CodeRabbit
Tests
New Features