-
Notifications
You must be signed in to change notification settings - Fork 1
create new cli flags [RHOSRFE-61] #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dsneddon
wants to merge
3
commits into
os-net-config:main
Choose a base branch
from
dsneddon:RHOSRFE-61_new_cli_flags
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| # os-net-config new feature(s) specification | ||
|
|
||
| ### Problem Descriptions | ||
|
|
||
| This specification defines several new CLI flags and modifies the behavior of the --noop flag | ||
| to improve the os-net-config workflow. These new flags will allow rollback of changes, allow | ||
| os-net-config to continue making modifications even if one interface fails to be configured | ||
| as requested, and will add functionality to look for IP address conflicts in the inteface | ||
| configurations which have static IP addresses. | ||
|
|
||
| ### Background Context (Optional) | ||
|
|
||
| Currently os-net-config does not have the ability to rollback if an inteface configuration | ||
| leads to an error, instead the execution will halt and typically the network configuration will | ||
| be left in a state with broken or limited connectivity. This situation is worse with the ifcfg | ||
| provider than with the nmstate provider, because nmstate supports rollback natively. Furthermore, | ||
| IP address conflict detection only happens after the interface configurations are applied. Since | ||
| the interface configurations are applied serially, when an error occurs further interfaces may | ||
| not be brought up, leading to inconsistent or broken state. | ||
|
|
||
| ### Proposed Changes | ||
|
|
||
| Several new CLI flags would be added in order to support rollbacks, continuing to apply interface | ||
| configurations after an error occurs, and new funcionality to check for IP address conflicts when | ||
| the --noop flag is used would be added. | ||
|
|
||
| #### List of Proposed Changes | ||
|
|
||
| 1. Add the --current-config flag which would be used to pass a copy of the config template which | ||
| would be used to rollback to if the --rollback flag is used, or possibly to modify the behavior | ||
| of the --cleanup flag to limit cleanup to only removing interface configurations which were | ||
| present in the --current-config configuration provided but are not present in the new config | ||
| template which is being applied. | ||
| 2. Add the --rollback flag which would either support rolling back to the original configuration | ||
| or to the --current-config template which is specified on the command-line. | ||
| 3. Add the --no-abort flag which would modify the workflow to not abort when a single interface | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the current design as well. The errors in applying the configurations does not break the flow usually. |
||
| error is encountered and continue to apply the configuration on other interfaces, but then | ||
| return a non-zero result code when the execution completes. | ||
| 4. Modify the behavior of the --noop flag to allow for duplicate IP detection of the static IP | ||
| addresses contained in the proposed configuration. | ||
|
|
||
| #### Overview | ||
|
|
||
| The new flags proposed would allow for a rollback option to use a specified configuration to | ||
| rollback to. The --rollback flag would specify that os-net-config would attempt to apply an | ||
| alternate configuration if the proposed configuration fails. The --current-config (or possibly | ||
| a different name would be more appropriate) would speficy what configuration should be applied | ||
| if the proposed configuration fails to apply cleanly. The --no-abort flag would modify the | ||
| workflow such that if an error occurs when configuring an interface, the other interface | ||
| configurations would be applied rather than exiting immediately with an error and a non-zero | ||
| return code. A special return code would indicate that an error had occurred but the remaining | ||
| interfaces were configured and os-net-config attempted to bring the interfaces up or restarted | ||
| them. Finally the --noop flag would add a subrouting to attempt to detect duplicate IPs on the | ||
| network to alert to whether the static IPs proposed would result in a duplicate IP detected error | ||
| when the interface configuration was applied and the interface was brought up or restarted. | ||
|
|
||
| ### Alternatives | ||
|
|
||
| Instead of applying these changes in os-net-config there are other approaches that could be | ||
| taken instead. | ||
|
|
||
| 1. Rather than adding the --rollback, --current-config, and --no-abort option, the workflow could | ||
| be applied by a wrapper script which would run os-net-config several times with different | ||
| config templates applied each time. This would result in the same or similar outcome to the | ||
| proposed CLI flags, but the logs would not be as clear as to what was occurring and it is | ||
| possible that with less state information available to the wrapper script that certain corner | ||
| cases could not be handled. | ||
| 2. Instead of running duplicate IP detection inside the --noop workflow, the duplicate IP | ||
| detection could be performed by extracting the static IP information out of the config | ||
| template and running the duplicate IP detection algorithm using a shell script or Ansible | ||
| playbook, etc. This has the disadvantage that the workflow would need to be implemented in | ||
| the external tool being used (shell script, Ansible playbook, etc.) and would need to be | ||
| implemented differently by each tool which is calling os-net-config. This would increase | ||
| complexity and could result in more errors if the workflow needs to be performed differently | ||
| using different scripting methods (such as shell scripts in one implementation and Ansible | ||
| playbooks in another implementation. By writing this logic once in the os-net-config codebase | ||
| the logic could be thoroughly tested and would potentially be more trustworthy and have less | ||
| chance of bugs occurring in one implementation. | ||
|
|
||
|
|
||
| #### Security Impacts | ||
|
|
||
| On possible security impact could be that a malicious attacker could pass a desired configuration | ||
| via the --current-config flag, along with a config template which is known to create errors, | ||
| knowing that the requested configuration would fail and that the current configuration would be | ||
| applied. This is a low-level impact because a malicious configuration could even more easily be | ||
| passed in the requested config template, and if an attacker already has root access to run | ||
| os-net-config than the attacker has all the access they would need to apply any malicious config | ||
| they desire. | ||
|
|
||
| Another potential impact is the risk of using --noop along with a custom-crafted configuration | ||
| with static IP addresses in order to surreptitiously run a network scanning attack. However the | ||
| attacker would already have access to run the tools needed to detect the presense of IP addresses | ||
| in use on the network, so this is another low-level risk. | ||
|
|
||
| #### Other end-user impact | ||
|
|
||
| The changes proposed here should improve the flexibility for end-users and make it easier to | ||
| handle potential errors and recover from them more gracefully. | ||
|
|
||
| #### Developer Impact | ||
|
|
||
| These changes will require careful analysis of the workflow logic, and will need to be tested | ||
| to ensure that the workflow is executing as designed in all cases. | ||
|
|
||
| ### Implementation | ||
|
|
||
| The implementation proposed here should be implemented in the cli.py code and also in the ifcfg | ||
| provider. A follow-on patchseries could replicate the behavior in the nmstate provider. | ||
|
|
||
| #### Assignee(s) | ||
|
|
||
| This work will be a collaboration between former Nokia engineers who are now working within | ||
| Red Hat to improve os-net-config and the current maintainers of os-net-config, Dan Sneddon | ||
| <[email protected]>, Karthik Sundaravel <[email protected]>, and Viji Candappa | ||
| <[email protected]>, and possibly other contrubutors. | ||
|
|
||
| #### Approver(s) | ||
|
|
||
| This specification work will be approved through the normal change review process, as well as | ||
| the changes to os-net-config. Red Hat QE engineers working alongside developers will review | ||
| the behavior of the changes and validate that they work as designed before the work which is | ||
| done upstream in the Github repository will be brought into the Red Hat product repositories | ||
| for Red Hat OpenStack Services on OpenShift and possibly Red Hat OpenStack RPM repositories | ||
| which are provided for Red Hat Cloud subscribers. | ||
|
|
||
| #### Work Items | ||
|
|
||
| 1. Approve specification using the change review process. | ||
| 2. Schedule the work on the os-net-config repositories on GitHub. | ||
| 3. Test the changes. | ||
| 4. Bring the changes into the product RPM repositories for inclusion into the official RPM | ||
| packages used by customers and included in a future released version. | ||
|
|
||
| #### Implementation Details | ||
|
|
||
| To be determined. | ||
|
|
||
| ### Dependencies | ||
|
|
||
| There will be several command-line utilities which will be required in order to perform | ||
| duplicate IP detection on a host which is running os-net-config. It should be possible to | ||
| check for the presence of these utilities before attempting to run the utilities, or to handle | ||
| failures gracefully and log an appropriate message in the logs. | ||
|
|
||
| ### Testing | ||
|
|
||
| Testing will involve setting up several workflows which are expected to fail and check for | ||
| graceful handling of the failures, as well as testing duplicate IP detection on networks where | ||
| the IP addresses requested are already in use by other hosts on the network. | ||
|
|
||
| ### Documentation Impact | ||
|
|
||
| The official os-net-config doucmentation will need to be updated, as well as the examples | ||
| in the /etc/os-net-config/ subdirectory of the source code. See [^1] for details. Also see the | ||
| Jira issue [^2] which describes these changes in more general terms. | ||
|
|
||
| ### References | ||
|
|
||
| [^1]: Official GitHub os-net-config repository: | ||
| <a href="https://github.com/os-net-config/os-net-config">os-net-config repository</a> | ||
| [^2]: Jira issue RHOSRFE-61 | ||
| <a href="https://issues.redhat.com/browse/RHOSRFE-61">Jira issue RHOSRFE-61</a> | ||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a complete rollback, can we consider configuring the minimal networks needed to enable ssh and other bare minimum tasks ?