-
Notifications
You must be signed in to change notification settings - Fork 932
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
ios.rb - Restore NVRAM config timestamp lines to config output #1921
Comments
Referring to these lines?
This is pretty cool! |
Information about the user and the time of modification is not reliable. It is not possible to audit changes based on this information. It changes even if you only enter configuration mode. If a person makes a mistake with the switch, finds an error when he enters the configuration mode and immediately exits, this information will change, and you will consider that the change was made by a completely different person at another time (I hope you will not conduct investigations and make charges based on this information , this is a great opportunity to blame another person). In addition, this information changes only after exiting the configuration mode, and if the session is saved and not leaving the configuration mode, then the date and username will remain old forever. Thus, the only normally working way to track changes is AAA accounting of entered commands. But audit of changes based on an unreliable header in the configuration - this is really "bad practices". Please, do not push other users to this. In addition, this problem exists with the equipment of other vendors, but in their models, the change time/username is simply deleted, even without the ability to change this behavior, so do not forget about the need to ensure consistency in the behavior of models. If you add an additional parameter to control whether this information will be deleted, then, in my opinion, the default value should provide deletion. |
802.1x is one scenario where configuration is constantly "updated" every time interface goes up/down. Such changes are visible in show run like this (note that "by username" is missing):
Having "last configuration change" as part of the config creates useless commit every time oxidized runs. |
@seros1521 @tarko - I understand the different use cases and I'm not advocating everyone rely on this method as the only way of tracking who changed what, I certainly don't - but I like layered approaches. In this case I think having the default behavior be less restrictive, and matching the historical behavior, is a better solution than removing it altogether. Custom models are still just as possible now as they've ever been and #456's method would provide an optional flag to strip these lines out. I'd rather have a PR with that functionality built-in. |
Maybe filter when no username? |
@ytti I thought about that too, but that would create a diff anyway, wouldn't it? I think it's all or nothing in this case. Would you be opposed to a PR that adds the new optional flag? I think that's the best solution as it's easy for new users to get their head around, rather than sending them off to create a custom model, and also gives power users that want to remove those lines the same ease of use. That would look like this:
|
sample cfg lines: cfg.each_line.reject { |line| line.match /^! (Last|No) configuration change (at|since).*/ unless line.match /\d+\sby\s\S+$/ }.join With this block we would keep the last configuration change line, if it contains "by username", providing basic accounting information. |
I'd like to have the behavior introduced in commit 3599f90 from #1896 reverted.
That is removing critical details of when the config was actually modified on the device as well as the username that did the modification. That seems like a poor choice for default behavior - particularly since those lines have been included for all ios users up til this commit.
#456 contains a workaround that includes a method of optionally ignoring these lines, which would make for a better solution if others want that behavior.
The text was updated successfully, but these errors were encountered: