AGENT-1461: Do not discard NMStateConfig when agent-config is on disk for unconfigured-ignition#10393
Conversation
|
@pawanpinjarkar: Jira Issue OCPBUGS-78420 is in a security level that is not in the allowed security levels for this repo.
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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts file-backed assets AgentConfigFile and NMStateConfigFile, moves loading/validation into them, updates UnconfiguredIgnition to validate agent-config (rendezvousIP-only), and adds multiple unconfigured-ignition testdata and tests; updates callers and test fixtures to the new asset shapes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cmd/openshift-install/testdata/agent/unconfigured-ignition/configurations/with-agent-config-rendezvousip.txt`:
- Around line 53-58: Replace the real-looking base64 auth string in the test
fixture under the ".dockerconfigjson" object (the "auth" value currently
"c3VwZXItc2VjcmV0Cg==") with a clearly fake placeholder (e.g.
"PLACEHOLDER_AUTH_TOKEN") and update any other similar pull-secret fixtures to
use the same placeholder to avoid triggering the secret scanner; edit the
fixture in the testdata file and mirror the change across other new fixtures
referencing the same "auth" field so CI/gitleaks no longer flags it.
In
`@cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-boot-artifacts-url.txt`:
- Around line 42-47: The fixture contains a hardcoded base64 auth value ("auth":
"c3VwZXItc2VjcmV0Cg==") under the .dockerconfigjson object which tripped the
secret scanner; replace that value with a clear non-secret placeholder (e.g. a
base64 encode of "PLACEHOLDER" or a string like "BASE64_PLACEHOLDER") so it
remains syntactically valid JSON but contains no real credentials, updating the
"auth" field in the test fixture accordingly.
In `@pkg/asset/agent/agentconfig/agent_config_file.go`:
- Around line 57-63: PersistToFile currently writes only a.Template which can be
empty after Load, truncating the file; change PersistToFile in AgentConfigFile
so it writes a non-empty source: prefer a.Template if set, otherwise fall back
to a.File (the original file contents) and if that's empty marshal a.Config to
YAML and write that; update references to AgentConfigFile.PersistToFile,
AgentConfigFile.Template, AgentConfigFile.File and AgentConfigFile.Config to
implement this fallback before calling os.WriteFile for agentConfigFilename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de1dbbc4-b883-440a-9e4a-13571fbd3dc6
📒 Files selected for processing (15)
cmd/openshift-install/testdata/agent/unconfigured-ignition/configurations/with-agent-config-rendezvousip.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-boot-artifacts-url.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-hosts.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-minimal-iso.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txtpkg/asset/agent/agentconfig/agent_config.gopkg/asset/agent/agentconfig/agent_config_file.gopkg/asset/agent/agentconfig/agenthosts_test.gopkg/asset/agent/image/ignition_test.gopkg/asset/agent/image/unconfigured_ignition.gopkg/asset/agent/image/unconfigured_ignition_test.gopkg/asset/agent/manifests/agent_test.gopkg/asset/agent/manifests/nmstateconfig.gopkg/asset/agent/manifests/nmstateconfigfile.gopkg/asset/agent/manifests/util_test.go
...l/testdata/agent/unconfigured-ignition/configurations/ove/with-agent-config-rendezvousip.txt
Show resolved
Hide resolved
...estdata/agent/unconfigured-ignition/validations/ove/agent-config-with-boot-artifacts-url.txt
Show resolved
Hide resolved
pawanpinjarkar
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pawanpinjarkar: This pull request references AGENT-1461 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 "4.22.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. |
pawanpinjarkar
force-pushed
the
do-not-discard-nmstateconfig-with-agentconfig-4-unconfigured-ignition
branch
from
dce4f85 to
033360d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/asset/agent/agentconfig/agent_config_file.go (1)
56-63:⚠️ Potential issue | 🟠 MajorPersisting can silently truncate
agent-config.yamlAt Line 58,
PersistToFilewrites onlya.Template. AfterLoad,a.Templateis typically empty whilea.File/a.Configare populated, so this can overwrite a valid file with empty content.Proposed fix
func (a *AgentConfigFile) PersistToFile(directory string) error { templatePath := filepath.Join(directory, agentConfigFilename) - templateByte := []byte(a.Template) - - err := os.WriteFile(templatePath, templateByte, 0600) + data := []byte(a.Template) + if len(data) == 0 && a.File != nil && len(a.File.Data) > 0 { + data = a.File.Data + } + if len(data) == 0 && a.Config != nil { + rendered, err := yaml.Marshal(a.Config) + if err != nil { + return errors.Wrapf(err, "failed to marshal %s", agentConfigFilename) + } + data = rendered + } + + err := os.WriteFile(templatePath, data, 0600) if err != nil { return err } return nil }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/agentconfig/agent_config_file.go` around lines 56 - 63, The PersistToFile method writes only a.Template to the file, but after loading, a.Template may be empty while a.File or a.Config hold the actual valid content. To fix this, modify PersistToFile to serialize and write the authoritative data source (likely a.Config or a.File) instead of a.Template to prevent overwriting with empty content.
🧹 Nitpick comments (2)
pkg/asset/agent/manifests/nmstateconfigfile.go (2)
120-132: Potential empty name in error message.When
nmStateConfig.Nameis empty (common for unnamed resources), the error message will be unclear:" does not have any label set".♻️ Suggested improvement
for _, nmStateConfig := range n.Config { if len(nmStateConfig.ObjectMeta.Labels) == 0 { - allErrs = append(allErrs, field.Required(fieldPath, fmt.Sprintf("%s does not have any label set", nmStateConfig.Name))) + name := nmStateConfig.Name + if name == "" { + name = "NMStateConfig" + } + allErrs = append(allErrs, field.Required(fieldPath, fmt.Sprintf("%s does not have any label set", name))) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/manifests/nmstateconfigfile.go` around lines 120 - 132, The error message in validateNMStateLabels can produce an unclear string when nmStateConfig.Name is empty; update validateNMStateLabels to derive a safe identifier (e.g., use nmStateConfig.Name or nmStateConfig.ObjectMeta.Name and fall back to a sentinel like "<unnamed>" or use the resource's namespace/name via GetName/GetNamespace if available) and include that identifier in the field.Required message so it never emits an empty name; reference the validateNMStateLabels function, the nmStateConfig loop, and the fieldPath/ nmStateConfig.ObjectMeta.Labels when making the change.
97-108: Log level manipulation is not thread-safe.The
logrus.SetLevel/logrus.GetLevelpattern modifies global state. If multiple goroutines load NMStateConfigFile concurrently, log messages from other components could be unexpectedly suppressed or the level could be restored incorrectly.Consider using a logger instance with a specific level instead of modifying the global logger, or accept that this validation runs in a single-threaded installer context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/manifests/nmstateconfigfile.go` around lines 97 - 108, The current validateWithNMStateCtl uses global logrus.SetLevel/GetLevel which is not thread-safe; replace that pattern by creating a dedicated logger instance (e.g., l := logrus.New()), set its level to logrus.WarnLevel on that instance, and pass instance.WithField("pkg","manifests") into staticnetworkconfig.New instead of logrus.WithField; remove the global GetLevel/SetLevel and defer restore, and keep the call to staticNetworkConfigGenerator.ValidateStaticConfigParams (referenced by staticNetworkConfigGenerator.ValidateStaticConfigParams and NMStateConfigFile.validateWithNMStateCtl) otherwise unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txt`:
- Around line 44-49: The fixture contains a sensitive-looking base64 string
under the .dockerconfigjson auth field ("c3VwZXItc2VjcmV0Cg=="); replace that
value with a clear non-secret placeholder (e.g. a descriptive dummy like
"BASE64_DUMMY_AUTH" or "REPLACE_WITH_DUMMY_AUTH") in this file and apply the
same replacement to any sibling validation fixtures that reuse the same literal
so secret scanners stop flagging the test data; ensure only the literal is
changed and the JSON structure (the "auths" -> "quay.io" -> "auth" hierarchy)
remains unchanged.
---
Duplicate comments:
In `@pkg/asset/agent/agentconfig/agent_config_file.go`:
- Around line 56-63: The PersistToFile method writes only a.Template to the
file, but after loading, a.Template may be empty while a.File or a.Config hold
the actual valid content. To fix this, modify PersistToFile to serialize and
write the authoritative data source (likely a.Config or a.File) instead of
a.Template to prevent overwriting with empty content.
---
Nitpick comments:
In `@pkg/asset/agent/manifests/nmstateconfigfile.go`:
- Around line 120-132: The error message in validateNMStateLabels can produce an
unclear string when nmStateConfig.Name is empty; update validateNMStateLabels to
derive a safe identifier (e.g., use nmStateConfig.Name or
nmStateConfig.ObjectMeta.Name and fall back to a sentinel like "<unnamed>" or
use the resource's namespace/name via GetName/GetNamespace if available) and
include that identifier in the field.Required message so it never emits an empty
name; reference the validateNMStateLabels function, the nmStateConfig loop, and
the fieldPath/ nmStateConfig.ObjectMeta.Labels when making the change.
- Around line 97-108: The current validateWithNMStateCtl uses global
logrus.SetLevel/GetLevel which is not thread-safe; replace that pattern by
creating a dedicated logger instance (e.g., l := logrus.New()), set its level to
logrus.WarnLevel on that instance, and pass
instance.WithField("pkg","manifests") into staticnetworkconfig.New instead of
logrus.WithField; remove the global GetLevel/SetLevel and defer restore, and
keep the call to staticNetworkConfigGenerator.ValidateStaticConfigParams
(referenced by staticNetworkConfigGenerator.ValidateStaticConfigParams and
NMStateConfigFile.validateWithNMStateCtl) otherwise unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a86d23f7-cc22-4fa7-8d21-44f057e10264
📒 Files selected for processing (15)
cmd/openshift-install/testdata/agent/unconfigured-ignition/configurations/with-agent-config-rendezvousip.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-boot-artifacts-url.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-hosts.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-minimal-iso.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txtpkg/asset/agent/agentconfig/agent_config.gopkg/asset/agent/agentconfig/agent_config_file.gopkg/asset/agent/agentconfig/agenthosts_test.gopkg/asset/agent/image/ignition_test.gopkg/asset/agent/image/unconfigured_ignition.gopkg/asset/agent/image/unconfigured_ignition_test.gopkg/asset/agent/manifests/agent_test.gopkg/asset/agent/manifests/nmstateconfig.gopkg/asset/agent/manifests/nmstateconfigfile.gopkg/asset/agent/manifests/util_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/asset/agent/manifests/util_test.go
- cmd/openshift-install/testdata/agent/unconfigured-ignition/configurations/with-agent-config-rendezvousip.txt
- cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-hosts.txt
| .dockerconfigjson: |- | ||
| { | ||
| "auths": { | ||
| "quay.io": { | ||
| "auth": "c3VwZXItc2VjcmV0Cg==" | ||
| } |
There was a problem hiding this comment.
Use a non-secret placeholder in pull-secret fixture to avoid scanner hits
Line 48 uses a value that is repeatedly flagged by secret scanners. Please replace it with a clear dummy placeholder (and apply the same replacement to sibling new validation fixtures using the same literal).
Proposed fixture update
- "quay.io": {
- "auth": "c3VwZXItc2VjcmV0Cg=="
- }
+ "quay.io": {
+ "auth": "test-auth-placeholder"
+ }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| .dockerconfigjson: |- | |
| { | |
| "auths": { | |
| "quay.io": { | |
| "auth": "c3VwZXItc2VjcmV0Cg==" | |
| } | |
| .dockerconfigjson: |- | |
| { | |
| "auths": { | |
| "quay.io": { | |
| "auth": "test-auth-placeholder" | |
| } |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 48-48: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txt`
around lines 44 - 49, The fixture contains a sensitive-looking base64 string
under the .dockerconfigjson auth field ("c3VwZXItc2VjcmV0Cg=="); replace that
value with a clear non-secret placeholder (e.g. a descriptive dummy like
"BASE64_DUMMY_AUTH" or "REPLACE_WITH_DUMMY_AUTH") in this file and apply the
same replacement to any sibling validation fixtures that reuse the same literal
so secret scanners stop flagging the test data; ensure only the literal is
changed and the JSON structure (the "auths" -> "quay.io" -> "auth" hierarchy)
remains unchanged.
|
/retest |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
pawanpinjarkar
force-pushed
the
do-not-discard-nmstateconfig-with-agentconfig-4-unconfigured-ignition
branch
2 times, most recently
from
c01a7df to
9957efb
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txt (1)
44-49:⚠️ Potential issue | 🟠 MajorReplace secret-like auth fixture value with a clear dummy placeholder.
Line 48 uses a credential-looking base64 string that will keep triggering secret scanners in testdata.
Proposed minimal fix
"quay.io": { - "auth": "c3VwZXItc2VjcmV0Cg==" + "auth": "BASE64_DUMMY_AUTH" }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txt` around lines 44 - 49, Replace the secret-like base64 value under the .dockerconfigjson "auth" field (currently "c3VwZXItc2VjcmV0Cg==") with a clear non-secret dummy placeholder (e.g., "dummy-auth" or "REDACTED") so testdata doesn't trigger secret scanners; update the value in the same .dockerconfigjson JSON block where the "auths" -> "quay.io" -> "auth" key appears.
🧹 Nitpick comments (1)
pkg/asset/agent/agentconfig/agent_config_file.go (1)
157-160: Unused functionunmarshalJSON.This function is defined but never called within the file. Consider removing it if it's not needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/agentconfig/agent_config_file.go` around lines 157 - 160, The function unmarshalJSON is defined but never used; either remove the unused function unmarshalJSON entirely from agent_config_file.go, or if intended for YAML-to-JSON conversion reuse, wire it into the code paths that decode config (e.g., replace direct yaml.JSONToYAML calls with unmarshalJSON or call unmarshalJSON from the relevant unmarshalling function); ensure you update imports if removal eliminates yaml usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txt`:
- Around line 44-49: Replace the secret-like base64 value under the
.dockerconfigjson "auth" field (currently "c3VwZXItc2VjcmV0Cg==") with a clear
non-secret dummy placeholder (e.g., "dummy-auth" or "REDACTED") so testdata
doesn't trigger secret scanners; update the value in the same .dockerconfigjson
JSON block where the "auths" -> "quay.io" -> "auth" key appears.
---
Nitpick comments:
In `@pkg/asset/agent/agentconfig/agent_config_file.go`:
- Around line 157-160: The function unmarshalJSON is defined but never used;
either remove the unused function unmarshalJSON entirely from
agent_config_file.go, or if intended for YAML-to-JSON conversion reuse, wire it
into the code paths that decode config (e.g., replace direct yaml.JSONToYAML
calls with unmarshalJSON or call unmarshalJSON from the relevant unmarshalling
function); ensure you update imports if removal eliminates yaml usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c510f177-4595-444b-abbb-f605a557d013
📒 Files selected for processing (15)
cmd/openshift-install/testdata/agent/unconfigured-ignition/configurations/with-agent-config-rendezvousip.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-boot-artifacts-url.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-hosts.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-minimal-iso.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txtpkg/asset/agent/agentconfig/agent_config.gopkg/asset/agent/agentconfig/agent_config_file.gopkg/asset/agent/agentconfig/agenthosts_test.gopkg/asset/agent/image/ignition_test.gopkg/asset/agent/image/unconfigured_ignition.gopkg/asset/agent/image/unconfigured_ignition_test.gopkg/asset/agent/manifests/agent_test.gopkg/asset/agent/manifests/nmstateconfig.gopkg/asset/agent/manifests/nmstateconfigfile.gopkg/asset/agent/manifests/util_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-boot-artifacts-url.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/asset/agent/agentconfig/agenthosts_test.go
- pkg/asset/agent/manifests/agent_test.go
- pkg/asset/agent/manifests/util_test.go
pawanpinjarkar
force-pushed
the
do-not-discard-nmstateconfig-with-agentconfig-4-unconfigured-ignition
branch
from
9957efb to
1527eb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/asset/agent/agentconfig/agent_config_file.go (1)
57-63:⚠️ Potential issue | 🟠 Major
PersistToFilecan still write an emptyagent-config.yamlin Config-only state.If
Templateis empty andFileis nil/empty, this silently writes zero bytes even whenConfigis populated.Proposed fix
func (a *AgentConfigFile) PersistToFile(directory string) error { templatePath := filepath.Join(directory, agentConfigFilename) data := []byte(a.Template) - if a.File != nil && len(a.File.Data) > 0 { + if len(data) == 0 && a.File != nil && len(a.File.Data) > 0 { data = a.File.Data } + if len(data) == 0 && a.Config != nil { + marshaled, err := yaml.Marshal(a.Config) + if err != nil { + return errors.Wrap(err, "failed to marshal agent config") + } + data = marshaled + } + if len(data) == 0 { + return errors.New("no agent-config content available to persist") + } err := os.WriteFile(templatePath, data, 0600) if err != nil { return err } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/agentconfig/agent_config_file.go` around lines 57 - 63, PersistToFile currently chooses data from a.Template or a.File.Data but will write an empty file if both are empty; update PersistToFile to check the resulting data, and if it's empty and a.Config is populated, marshal a.Config into YAML (e.g., using yaml.Marshal) and use that as data; if data is still empty (or marshaling fails) return a clear non-nil error instead of writing an empty file; keep the final os.WriteFile call (with mode 0600) using the validated/generated data and propagate any write errors.
🧹 Nitpick comments (1)
pkg/asset/agent/image/unconfigured_ignition_test.go (1)
57-135: Add a direct regression case foragent-config + nmstateconfigtogether.The table currently covers “nmstate only” and “agent-config only”, but not the exact combined path this PR is fixing. Add one case that injects both and asserts NMState network artifacts are still emitted.
Suggested test case addition
{ name: "with-nmstateconfigs", overrideDeps: []asset.Asset{ &nmStateConfig, }, expectedFiles: generatedFilesUnconfiguredIgnition("/etc/assisted/network/host0/eth0.nmconnection", "/etc/assisted/network/host0/mac_interface.ini", "/usr/local/bin/pre-network-manager-config.sh", "/usr/local/bin/oci-eval-user-data.sh"), serviceEnabledMap: map[string]bool{ "pre-network-manager-config.service": true, "oci-eval-user-data.service": true, "agent-check-config-image.service": true}, }, + { + name: "with-nmstateconfigs-and-rendezvousIP-agentconfig", + overrideDeps: []asset.Asset{ + &nmStateConfig, + &agentconfig.AgentConfig{ + AgentConfigFile: agentconfig.AgentConfigFile{ + Config: &agenttypes.Config{ + RendezvousIP: "192.168.111.20", + }, + }, + }, + }, + expectedFiles: generatedFilesUnconfiguredIgnition( + "/etc/assisted/network/host0/eth0.nmconnection", + "/etc/assisted/network/host0/mac_interface.ini", + "/etc/assisted/rendezvous-host.env", + "/usr/local/bin/pre-network-manager-config.sh", + "/usr/local/bin/oci-eval-user-data.sh", + ), + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/image/unconfigured_ignition_test.go` around lines 57 - 135, Add a new table test case that injects both an agentconfig.AgentConfig and the nmstate asset (e.g. nmstate.NMStateConfig) into overrideDeps to cover the combined path; set agentconfig.AgentConfig.Config.RendezvousIP and a simple nmstate config in nmstate.NMStateConfig, then assert expectedFiles includes the usual generatedFilesUnconfiguredIgnition(...) plus the NMState network artifact(s) (the nmstate output file(s) your code emits) and that serviceEnabledMap remains correct; place this new case alongside the existing cases so the test exercises the combined agent-config + nmstateconfig path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/agent/manifests/nmstateconfigfile.go`:
- Around line 103-106: The code mutates the global Logrus level; instead create
a dedicated logger instance for staticnetworkconfig.New by constructing a new
logger (e.g., logger := logrus.New()), set its level to WarnLevel on that
instance (logger.SetLevel(logrus.WarnLevel)), create a fielded entry
(logrus.NewEntry(logger).WithField("pkg","manifests")) and pass that entry to
staticnetworkconfig.New along with the Config; remove the calls to
logrus.GetLevel and logrus.SetLevel and the defer so no global logger state is
changed.
---
Duplicate comments:
In `@pkg/asset/agent/agentconfig/agent_config_file.go`:
- Around line 57-63: PersistToFile currently chooses data from a.Template or
a.File.Data but will write an empty file if both are empty; update PersistToFile
to check the resulting data, and if it's empty and a.Config is populated,
marshal a.Config into YAML (e.g., using yaml.Marshal) and use that as data; if
data is still empty (or marshaling fails) return a clear non-nil error instead
of writing an empty file; keep the final os.WriteFile call (with mode 0600)
using the validated/generated data and propagate any write errors.
---
Nitpick comments:
In `@pkg/asset/agent/image/unconfigured_ignition_test.go`:
- Around line 57-135: Add a new table test case that injects both an
agentconfig.AgentConfig and the nmstate asset (e.g. nmstate.NMStateConfig) into
overrideDeps to cover the combined path; set
agentconfig.AgentConfig.Config.RendezvousIP and a simple nmstate config in
nmstate.NMStateConfig, then assert expectedFiles includes the usual
generatedFilesUnconfiguredIgnition(...) plus the NMState network artifact(s)
(the nmstate output file(s) your code emits) and that serviceEnabledMap remains
correct; place this new case alongside the existing cases so the test exercises
the combined agent-config + nmstateconfig path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9607246-90c0-4bdf-8d89-caa8b322f0ff
📒 Files selected for processing (15)
cmd/openshift-install/testdata/agent/unconfigured-ignition/configurations/with-agent-config-rendezvousip.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-boot-artifacts-url.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-hosts.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-minimal-iso.txtcmd/openshift-install/testdata/agent/unconfigured-ignition/validations/agent-config-with-ntp-sources.txtpkg/asset/agent/agentconfig/agent_config.gopkg/asset/agent/agentconfig/agent_config_file.gopkg/asset/agent/agentconfig/agenthosts_test.gopkg/asset/agent/image/ignition_test.gopkg/asset/agent/image/unconfigured_ignition.gopkg/asset/agent/image/unconfigured_ignition_test.gopkg/asset/agent/manifests/agent_test.gopkg/asset/agent/manifests/nmstateconfig.gopkg/asset/agent/manifests/nmstateconfigfile.gopkg/asset/agent/manifests/util_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/asset/agent/manifests/util_test.go
- pkg/asset/agent/manifests/agent_test.go
- pkg/asset/agent/image/ignition_test.go
- pkg/asset/agent/agentconfig/agenthosts_test.go
|
/test e2e-agent-compact-ipv4-iso-no-registry |
1 similar comment
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@pawanpinjarkar: This pull request references AGENT-1461 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 "4.22.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. |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
...l/testdata/agent/unconfigured-ignition/configurations/ove/with-agent-config-rendezvousip.txt
Show resolved
Hide resolved
...estdata/agent/unconfigured-ignition/validations/ove/agent-config-with-boot-artifacts-url.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
I don't think this split is really required?
There was a problem hiding this comment.
We need to also split NMStateConfig into NMStateConfig NMStateConfigFile because as the NMStateConfig has dependencies, when it's loaded from disk, the asset system marks it as "dirty" and tries to regenerate it. Since Generate() returns early when there are no agent hosts (line 76), the loaded network config is lost.
level=warning msg= Discarding the NMState Config that was provided in the target directory because its dependencies are dirty and it needs to be regenerated
|
/test e2e-agent-compact-ipv4-iso-no-registry |
pawanpinjarkar
force-pushed
the
do-not-discard-nmstateconfig-with-agentconfig-4-unconfigured-ignition
branch
from
310a5c4 to
306b11d
Compare
… for unconfigured-ignition This use case is particularly for the OVE use case when rendezvousIP and Static networking via NMStateconfig are provided via SaaS UI.
pawanpinjarkar
force-pushed
the
do-not-discard-nmstateconfig-with-agentconfig-4-unconfigured-ignition
branch
from
306b11d to
5889656
Compare
|
/verified by ci/prow/integration-tests |
openshift-ci-robot
added
the
verified
|
@pawanpinjarkar: This PR has been marked as verified by 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. |
|
/lgtm Let's see if we could get also a gree iso-no-registry run /test e2e-agent-compact-ipv4-iso-no-registry |
|
/retest |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/cherrypick release-4.21 |
|
@pawanpinjarkar: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
@pawanpinjarkar: The following tests 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. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
added
needs-rebase
|
PR needs rebase. 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. |
5 participants
Do not discard NMStateConfig when agent-config is on disk for unconfigured-ignition. This use case is particularly for the OVE use case when rendezvousIP and Static networking via NMStateconfig are provided via SaaS UI.
This PR follows a similar approach for already existing
InfraEnvandInfraEnvFileassets.