OSASINFRA-4400: Add cluster-network-config ConfigMap to support max_allowed_address_pairs in OpenStack#3058
OSASINFRA-4400: Add cluster-network-config ConfigMap to support max_allowed_address_pairs in OpenStack#3058danchild wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@danchild: This pull request references OSASINFRA-4400 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe change reads ChangesOpenStack address-pair configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Infrastructure
participant Bootstrap
participant ConfigMap
participant Renderer
participant ControllerDeployment
Infrastructure->>Bootstrap: report OpenStack platform
Bootstrap->>ConfigMap: fetch cloud-network-config
ConfigMap-->>Bootstrap: return raw address-pair value
Bootstrap->>Renderer: provide parsed value and set state
Renderer->>Renderer: validate positive integer
Renderer->>ControllerDeployment: render conditional controller flag
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (19 passed)
✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danchild 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 |
c0144b5 to
666ef2c
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@bindata/cloud-network-config-controller/self-hosted/controller.yaml`:
- Around line 50-54: Convert the container args flow sequence around the
existing "-secret-name" and OSMaxAllowedAddressPairs template entries into a
YAML block sequence using list-item syntax. Preserve the current argument values
and conditional inclusion controlled by OSMaxAllowedAddressPairsIsSet and
OSMaxAllowedAddressPairs.
In `@pkg/network/bootstrap.go`:
- Around line 76-107: Update cloudNetworkConfigBootstrap to return the API error
alongside CloudNetworkConfigBootstrapResult, propagating transient errors from
cl.Get instead of logging and returning an empty result. Adjust Bootstrap and
its caller handling to receive the error and return it so reconciliation
requeues with backoff; preserve the existing not-found and successful
configuration behavior.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 10ffe051-5f03-4806-86dd-a122e8f1e1f3
📒 Files selected for processing (7)
bindata/cloud-network-config-controller/managed/controller.yamlbindata/cloud-network-config-controller/self-hosted/controller.yamlpkg/bootstrap/types.gopkg/network/bootstrap.gopkg/network/bootstrap_test.gopkg/network/cloud_network.gopkg/network/cloud_network_test.go
| "-secret-name", "cloud-credentials" | ||
| {{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }} | ||
| ,"-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}" | ||
| {{- end }} | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Convert args to a YAML block sequence.
Using Go templates within a YAML flow sequence ([...]) causes syntax errors during static analysis (as indicated by the Yamllint error). Converting the args array into a block sequence (a bulleted list) resolves this parser issue and perfectly aligns with standard Kubernetes manifest conventions.
🛠️ Proposed fix to use block sequence
- args: [ "-platform-type", "{{.PlatformType}}",
- "-platform-region={{.PlatformRegion}}",
- "-platform-api-url={{.PlatformAPIURL}}",
- "-platform-aws-ca-override={{.PlatformAWSCAPath}}",
- "-platform-azure-environment={{.PlatformAzureEnvironment}}",
- "-secret-name", "cloud-credentials"
-{{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }}
- ,"-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}"
-{{- end }}
- ]
+ args:
+ - "-platform-type"
+ - "{{.PlatformType}}"
+ - "-platform-region={{.PlatformRegion}}"
+ - "-platform-api-url={{.PlatformAPIURL}}"
+ - "-platform-aws-ca-override={{.PlatformAWSCAPath}}"
+ - "-platform-azure-environment={{.PlatformAzureEnvironment}}"
+ - "-secret-name"
+ - "cloud-credentials"
+{{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }}
+ - "-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}"
+{{- end }}📝 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.
| "-secret-name", "cloud-credentials" | |
| {{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }} | |
| ,"-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}" | |
| {{- end }} | |
| ] | |
| args: | |
| - "-platform-type" | |
| - "{{.PlatformType}}" | |
| - "-platform-region={{.PlatformRegion}}" | |
| - "-platform-api-url={{.PlatformAPIURL}}" | |
| - "-platform-aws-ca-override={{.PlatformAWSCAPath}}" | |
| - "-platform-azure-environment={{.PlatformAzureEnvironment}}" | |
| - "-secret-name" | |
| - "cloud-credentials" | |
| {{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }} | |
| - "-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}" | |
| {{- end }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 51-51: syntax error: expected ',' or ']', but got '{'
(syntax)
🤖 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 `@bindata/cloud-network-config-controller/self-hosted/controller.yaml` around
lines 50 - 54, Convert the container args flow sequence around the existing
"-secret-name" and OSMaxAllowedAddressPairs template entries into a YAML block
sequence using list-item syntax. Preserve the current argument values and
conditional inclusion controlled by OSMaxAllowedAddressPairsIsSet and
OSMaxAllowedAddressPairs.
Source: Linters/SAST tools
|
|
||
| func cloudNetworkConfigBootstrap(cl crclient.Reader) bootstrap.CloudNetworkConfigBootstrapResult { | ||
| result := bootstrap.CloudNetworkConfigBootstrapResult{} | ||
|
|
||
| cm := &corev1.ConfigMap{} | ||
| if err := cl.Get(context.TODO(), types.NamespacedName{ | ||
| Namespace: "openshift-network-operator", | ||
| Name: "cloud-network-config", | ||
| }, cm); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| klog.Warningf("Error fetching cloud-network-config configmap: %v", err) | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| raw, ok := cm.Data["platform-os-max-allowed-address-pairs"] | ||
| if !ok { | ||
| return result | ||
| } | ||
|
|
||
| result.OSMaxAllowedAddressPairs.IsSet = true | ||
| result.OSMaxAllowedAddressPairs.RawValue = raw | ||
|
|
||
| val, err := strconv.Atoi(raw) | ||
| if err != nil { | ||
| klog.Warningf("Error parsing cloud-network-config platform-os-max-allowed-address-pairs=%q: %v", raw, err) | ||
| return result | ||
| } | ||
|
|
||
| result.OSMaxAllowedAddressPairs.Value = val | ||
| return result | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bubble up transient API errors instead of swallowing them.
If fetching the cloud-network-config ConfigMap fails due to a transient API error (e.g., API server unavailable), the current implementation logs a warning and returns an empty result, acting as if the configuration does not exist. This can cause the controller to silently roll back to default settings during a temporary outage.
As per path instructions, "Never ignore error returns". Return the API error so the reconciliation loop can correctly requeue and retry via exponential backoff.
🛡️ Proposed fix to propagate transient API errors
Apply this fix to the cloudNetworkConfigBootstrap signature and its caller in Bootstrap (around line 40):
@@ -38,5 +38,8 @@
if infraStatus.PlatformType == configv1.OpenStackPlatformType {
- out.CloudNetworkConfig = cloudNetworkConfigBootstrap(client.ClientFor("").CRClient())
+ cnc, err := cloudNetworkConfigBootstrap(client.ClientFor("").CRClient())
+ if err != nil {
+ return nil, err
+ }
+ out.CloudNetworkConfig = cnc
}
@@ -76,14 +79,14 @@
-func cloudNetworkConfigBootstrap(cl crclient.Reader) bootstrap.CloudNetworkConfigBootstrapResult {
+func cloudNetworkConfigBootstrap(cl crclient.Reader) (bootstrap.CloudNetworkConfigBootstrapResult, error) {
result := bootstrap.CloudNetworkConfigBootstrapResult{}
cm := &corev1.ConfigMap{}
if err := cl.Get(context.TODO(), types.NamespacedName{
Namespace: "openshift-network-operator",
Name: "cloud-network-config",
}, cm); err != nil {
if !apierrors.IsNotFound(err) {
- klog.Warningf("Error fetching cloud-network-config configmap: %v", err)
+ return result, fmt.Errorf("error fetching cloud-network-config configmap: %w", err)
}
- return result
+ return result, nil
}
raw, ok := cm.Data["platform-os-max-allowed-address-pairs"]
if !ok {
- return result
+ return result, nil
}
result.OSMaxAllowedAddressPairs.IsSet = true
result.OSMaxAllowedAddressPairs.RawValue = raw
val, err := strconv.Atoi(raw)
if err != nil {
klog.Warningf("Error parsing cloud-network-config platform-os-max-allowed-address-pairs=%q: %v", raw, err)
- return result
+ return result, nil
}
result.OSMaxAllowedAddressPairs.Value = val
- return result
+ return result, nil
}📝 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.
| func cloudNetworkConfigBootstrap(cl crclient.Reader) bootstrap.CloudNetworkConfigBootstrapResult { | |
| result := bootstrap.CloudNetworkConfigBootstrapResult{} | |
| cm := &corev1.ConfigMap{} | |
| if err := cl.Get(context.TODO(), types.NamespacedName{ | |
| Namespace: "openshift-network-operator", | |
| Name: "cloud-network-config", | |
| }, cm); err != nil { | |
| if !apierrors.IsNotFound(err) { | |
| klog.Warningf("Error fetching cloud-network-config configmap: %v", err) | |
| } | |
| return result | |
| } | |
| raw, ok := cm.Data["platform-os-max-allowed-address-pairs"] | |
| if !ok { | |
| return result | |
| } | |
| result.OSMaxAllowedAddressPairs.IsSet = true | |
| result.OSMaxAllowedAddressPairs.RawValue = raw | |
| val, err := strconv.Atoi(raw) | |
| if err != nil { | |
| klog.Warningf("Error parsing cloud-network-config platform-os-max-allowed-address-pairs=%q: %v", raw, err) | |
| return result | |
| } | |
| result.OSMaxAllowedAddressPairs.Value = val | |
| return result | |
| } | |
| func cloudNetworkConfigBootstrap(cl crclient.Reader) (bootstrap.CloudNetworkConfigBootstrapResult, error) { | |
| result := bootstrap.CloudNetworkConfigBootstrapResult{} | |
| cm := &corev1.ConfigMap{} | |
| if err := cl.Get(context.TODO(), types.NamespacedName{ | |
| Namespace: "openshift-network-operator", | |
| Name: "cloud-network-config", | |
| }, cm); err != nil { | |
| if !apierrors.IsNotFound(err) { | |
| return result, fmt.Errorf("error fetching cloud-network-config configmap: %w", err) | |
| } | |
| return result, nil | |
| } | |
| raw, ok := cm.Data["platform-os-max-allowed-address-pairs"] | |
| if !ok { | |
| return result, nil | |
| } | |
| result.OSMaxAllowedAddressPairs.IsSet = true | |
| result.OSMaxAllowedAddressPairs.RawValue = raw | |
| val, err := strconv.Atoi(raw) | |
| if err != nil { | |
| klog.Warningf("Error parsing cloud-network-config platform-os-max-allowed-address-pairs=%q: %v", raw, err) | |
| return result, nil | |
| } | |
| result.OSMaxAllowedAddressPairs.Value = val | |
| return result, nil | |
| } |
🤖 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 `@pkg/network/bootstrap.go` around lines 76 - 107, Update
cloudNetworkConfigBootstrap to return the API error alongside
CloudNetworkConfigBootstrapResult, propagating transient errors from cl.Get
instead of logging and returning an empty result. Adjust Bootstrap and its
caller handling to receive the error and return it so reconciliation requeues
with backoff; preserve the existing not-found and successful configuration
behavior.
Source: Path instructions
666ef2c to
62b0134
Compare
Following `openshift/cloud-network-config-controller` openshift#230, a mechanism is required to pass the configured `max_allowed_address_pairs` as a command line argument to the controller. New feature includes: - Add a bootstrap result type for a new `cloud-network-config` ConfigMap, reading it if the platform type is OpenStack - Validate `platform-os-max-allowed-address-pairs`, returning an error if it is negative, explicitly set to 0, or an invalid number (e.g. alphabetic string) - Pass platform-os-max-allowed-address-pairs to the `cloud-network-config-controller` as a command line argument - Check for changes to `cloud-network-config`, restarting the cloud-network-config-controller if there's a diff Signed-off-by: Dan Childers <dchilder@redhat.com>
Exercise managed and self-hosted template rendering for the new max_allowed_address_pairs flag, including zero, negative, non-integer, valid, and unset cases. Signed-off-by: Dan Childers <dchilder@redhat.com>
62b0134 to
1a3f781
Compare
|
@danchild: 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. |
|
/verified by @danchild |
openshift-ci-robot
added
the
verified
|
@danchild: 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. |
2 participants
Following work in openshift/cloud-network-config-controller/pull/230, a mechanism is required to pass the configured
max_allowed_address_pairsas a command line argument to the controller.New feature includes:
cloud-network-configConfigMap,reading it if the platform type is OpenStack
platform-os-max-allowed-address-pairs, returning an errorif it is negative, explicitly set to 0, or an invalid number (e.g.
alphabetic string)
cloud-network-config-controlleras a command line argumentcloud-network-config, restarting thecloud-network-config-controller if there's a diff
Please note that openshift/cloud-network-config-controller/pull/230 must be accepted and merged before accepting these changes.