Skip to content

Update VxLan.php - Add input validations to model#6899

Merged
AdSchellevis merged 2 commits intoopnsense:masterfrom
Monviech:vxlan-input-validation
Oct 2, 2023
Merged

Update VxLan.php - Add input validations to model#6899
AdSchellevis merged 2 commits intoopnsense:masterfrom
Monviech:vxlan-input-validation

Conversation

@Monviech
Copy link
Copy Markdown
Member

#6893

That was rather challenging for me. But I think I got it working the way it should.

Here's some screenshots with the results:

Requirement for either Remote address or Multicast group NOT to be empty:
grafik

Requirement for both Remote address or Multicast group NOT to be populated at the same time:
grafik

Requirement for Device being set when Multicast group is populated:
grafik

Requirement for Device being NONE when Remote address is populated:
grafik

@AdSchellevis AdSchellevis self-assigned this Sep 30, 2023
@AdSchellevis
Copy link
Copy Markdown
Member

hehe, it's certainly a good starting point, in general I think there are a couple of small things missing. The messages should be passed though gettext() and you want to make sure the record inspected is actually changed (to prevent old invalid data not presented locking your save operation)

I can take a look later and offer you the commit, the weekend is a bit busy, but we're probably not in a hurry :)

@Monviech
Copy link
Copy Markdown
Member Author

Thank you very much for your feedback. This is by far the most challenging thing I ever changed in the OPNsense. And I'm not really in a hurry here. I really wanted to see if I could get a result. Maybe you've seen the debugging I did in the forum that resulted in this PR, I'm more firm as network or linux admin. Trying to actually change something in code is pretty new to me. xD

@AdSchellevis
Copy link
Copy Markdown
Member

I missed the forum part, but it has been extremely busy the last week, I just like it when people are making an effort with small and focussed changes.

If you're interested, I can complete this one and annotate the changes in a separate commit so the rational between choices might be easier to understand.

Out of curiosity, how are you using OPNsense? seen quite some useful efforts from your end, thanks for that.

@Monviech
Copy link
Copy Markdown
Member Author

Monviech commented Oct 1, 2023

I will try and implement the gettext() - I've read that its for translation between languages, so I'm interested in seeing how that works. Also I want to try to "prevent old invalid data not presented locking your save operation" - But I would need a hint here how to actually test if something like this works. I have seen in the VIP.php file that at the beginning it loads all changed data. I might be able to do the same, so I wanne try it at least before passing the baton :)

Also I'm using OPNsense DEC Hardware with the business edition for customers and the company I work at. I also like using the community edition as virtual machine on ESXi Servers for small enviroments where hosting and ipsec customer login happens. And of course the community edition for home use and tinkering.

Hope you'll have a nice weekend still. ^^

@AdSchellevis
Copy link
Copy Markdown
Member

Let me add some review notes, I can understand that you want to give it a shot, with some pointers it should be easier 🙂

I'm certainly enjoying my weekend, hope you are enjoying yours too.

@AdSchellevis
Copy link
Copy Markdown
Member

ok, the relevant hints should be in there now, I'm going offline now, but if you do run into issues, just make a note and I'll take a look later.

Enjoy the rest of your weekend!

@Monviech
Copy link
Copy Markdown
Member Author

Monviech commented Oct 2, 2023

I have tested this, and now I'm able to save old broken records because no fields have been changed. And I'm also able to create new records that are validated because fields have been changed.

@AdSchellevis AdSchellevis merged commit ef9c2b4 into opnsense:master Oct 2, 2023
@AdSchellevis
Copy link
Copy Markdown
Member

@Monviech nice! should work like a charm indeed. Thanks!

@AdSchellevis AdSchellevis added the feature Adding new functionality label Oct 2, 2023
@Monviech Monviech deleted the vxlan-input-validation branch October 2, 2023 16:51
fichtner pushed a commit that referenced this pull request Oct 18, 2023
(cherry picked from commit ef9c2b4)
(cherry picked from commit a10cf1c)
(cherry picked from commit 9c15cf7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Adding new functionality

Development

Successfully merging this pull request may close these issues.

2 participants