epacke / BigIPReport

Overview of your loadbalancer configuration
https://loadbalancing.se
8 stars 2 forks source link

Tim's updates, see description (Closes #74 #95 #124 #139 #146 #157 #158) #145

Closed timriker closed 4 years ago

timriker commented 5 years ago

regex searching "Move the content of this folder to the configured report root" -> underlay new settings in xml! added preferences.json, removed defaultpreferences.json

new tab/button styles and icons expand/collapse/normal toggle on tables pool, irule and datagroup tables use "details" html5 markup stats to loggederrors.json hide main desc, asm, ssl, compression, persistence columns by default hide certs country, state by default show snat pool in virtual server details add description column to pool table url and pool links in datagroups new status search syntax Fix curl headers add monitor links monitor names in pooltable remove hidden textstatus markup merge code to resetFilters() and toggleExpandCollapseRestore()

epacke commented 5 years ago

Looks good. Just one change left.

This section:

"BigIPReport is written and maintained by Patrik Jonsson"

should contain your name.

epacke commented 5 years ago

I ran into a bunch of exceptions. Need to troubleshoot a bit.

epacke commented 5 years ago

Created a patch for it and made a PR to your branch. It uses the REST API but I think that's OK now. We can't support v11 forever anyway. If people need that they can always use an old version of the report.

What do you think?

timriker commented 5 years ago

What exceptions are you seeing? We're running this in production now without issues. Are there specific datagroups it's failing on? I can create anything I like on our sandbox if you can send me more details.

epacke commented 5 years ago

I've tried to find the common denominator, but haven't been able to. Either way, I think it's about time that we started to migrate the script over to REST because it's likely faster, supported by F5 and we can start running the script on linux/containers. :)

timriker commented 5 years ago

I fixed an issue dealing with datagroups with name entries without values. Can you test if that is your issue? I do think we should move to REST. People can use admin accounts if they still have < v12 servers. I have a few that we have not yet turned off, but we are really close. Testing 14 now. Still not doing any active-active, but I hope to test that in our sandbox soon. Test out the fix? it's this: if (!$TempPool) {continue}

epacke commented 5 years ago

Regarding active-active, what changes would you propose? Will test the fix today.

On Fri, Oct 18, 2019, 7:38 AM Tim Riker notifications@github.com wrote:

I fixed an issue dealing with datagroups with name entries without values. Can you test if that is your issue? I do think we should move to REST. People can use admin accounts if they still have < v12 servers. I have a few that we have not yet turned off, but we are really close. Testing 14 now. Still not doing any active-active, but I hope to test that in our sandbox soon. Test out the fix? it's this: if (!$TempPool) {continue}

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/epacke/BigIPReport/pull/145?email_source=notifications&email_token=AEDD6O3NIFMKMUS4XTUAERLQPFDU3A5CNFSM4IY2WK32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBSW7SA#issuecomment-543518664, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEDD6OZJVVPGMLHPWEDX2JTQPFDU3ANCNFSM4IY2WK3Q .

epacke commented 5 years ago

The fix did not work:

Cannot index into a null array. At C:\Scripts\BigipReport\new\bigipreport.ps1:1183 char:13

  • $Value = [string]$AddressClassValues[$i][$x]
  • 
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : NullArray

It seems to be thrown globally across some units for all data group lists and not thrown per data group. I tried rebooting but no luck.

timriker commented 5 years ago

That line is with your patch, right? Mine still has: [array]$AddressClassValues = $F5.LocalLBClass.get_address_class_member_data_value($AddressClassKeys) Many of my address datagroups have no comments in the value side. I'll test with a mix and see if I can cause any issues.

epacke commented 5 years ago

That line is with your patch, right? Mine still has:

No, with the latest commit in this PR.

timriker commented 5 years ago

Oh! I read it backwards. Your patch would have removed this line: https://github.com/epacke/BigIPReport/pull/153/commits/f01867f8442b9e0801294e53cb6a59061a9676b7#diff-621cd034392d21b7a2726b46f96c59e8L1181 I'll see if I can duplicate it.

timriker commented 5 years ago

Do you have F5s with no address datagroups? or just one? If one, does it have values? So far, I've not been able to duplicate the error.

timriker commented 5 years ago

I wrapped the line in a try catch, which should avoid the issue. Test?

epacke commented 4 years ago

Do you have F5s with no address datagroups? or just one? If one, does it have values? So far, I've not been able to duplicate the error.

Actually, the bug concerns the whole LB no matter if a data group is empty or not. Catch won't work as we won't get any data at all in that case.

epacke commented 4 years ago

Let's not add any more functionality to this PR if that's OK? Since there's no unit/integration testing it takes a long time to go through everything every time. :)

timriker commented 4 years ago

The REST datagroup access is failing on a couple of my boxes. Still testing. Will comment as I learn more.

timriker commented 4 years ago

One one of our largest boxes, I'm was getting a few errors on the irule saving virtualservers. This was because it had some zero byte iRules. I added minimum support to bypass the error. In order to pull ASM, a Guest or Auditor account is not enough. :( I've upped accounts to Administrator for testing. I'm fine with this pull request getting approved. I have a few small changes coming, adding regex to site defaults, etc. but it can go before if you like.

epacke commented 4 years ago

Hi Tim Now the code runs without errors, nice features and well done!

A few questions/feedback:

new status search syntax

How does it work?

do we need textstatus html anymore?

I don't understand this one.

regex searching

How is it enabled?

expand/collapse/normal toggle

Could you please print out what the button does instead of using a single letter? I think it would be better from a UX perspective?

monitor names in pooltable

I don't see the difference here.

The colors of the table seems to have changed from Gray to Blueish purple. Was this change intentional?

timriker commented 4 years ago

new status search syntax - How does it work? - see the help page. I changed the status text. I added hover titles so that the statustext equivalent for an icon is visible.

do we need textstatus html anymore? - I don't understand this one. - there is a span.textstatus that is not displayed. The new text status is rendered to datatables for the filter view. I think we can remove it from the display and print views. We can already search on it without it being in the html.

regex searching - How is it enabled? - in my pull request it's now enabled by default, and can be turned off under the preferences tab.

expand/collapse/normal toggle - Could you please print out what the button does instead of using a single letter? I think it would be better from a UX perspective? - I was looking for some chevron type indications, but I'll revert to full words for now until we find some other UI way of showing it. I'd like to add the same button to the other table views. Did you see my pull request with the "details/summary" implementation? nice that the browsers now have that built in, not so nice that the "summary" has to stay visible when expanded.

monitor names in pooltable - I don't see the difference here. - new searchable column, disabled by default. Toggle it on to see. :)

The colors of the table seems to have changed from Gray to Blueish purple. Was this change intentional? - I'm trying to not have the first row the same color as the header. I added a little blue to differentiate them. Any color difference will do, feel free to change. Did you notice the row hover highlight now? or the sort column highlight? Those are included in the datatables "display" class.

epacke commented 4 years ago

new status search syntax - How does it work? - see the help page. I changed the status text. I added hover titles so that the statustext equivalent for an icon is visible.

I see it now. Nice!

do we need textstatus html anymore? - I don't understand this one. - there is a span.textstatus that is not displayed. The new text status is rendered to datatables for the filter view. I think we can remove it from the display and print views. We can already search on it without it being in the html.

If it works without the span, let's remove it. :)

do we need textstatus html anymore? - I don't understand this one. - there is a span.textstatus that is not displayed. The new text status is rendered to datatables for the filter view. I think we can remove it from the display and print views. We can already search on it without it being in the html.

I see it now. This is great!

I was looking for some chevron type indications, but I'll revert to full words for now until we find some other UI way of showing it. I'd like to add the same button to the other table views. Did you see my pull request with the "details/summary" implementation? nice that the browsers now have that built in, not so nice that the "summary" has to stay visible when expanded.

Good stuff. I will have a look at the other PR soon. :)

monitor names in pooltable - I don't see the difference here. - new searchable column, disabled by default. Toggle it on to see. :)

I can see this too now. Must have been some cache that expired over the night. Or maybe I need glasses. Nice!

The colors of the table seems to have changed from Gray to Blueish purple. Was this change intentional? - I'm trying to not have the first row the same color as the header. I added a little blue to differentiate them. Any color difference will do, feel free to change. Did you notice the row hover highlight now? or the sort column highlight? Those are included in the datatables "display" class.

I really like the hover highlights and I agree that the old style looked a bit "dull". However the blue tint looks out of place for me. Any chance this part can be restored together with the borders? I can look into making PR additions in the weekend if you don't have time right now.

timriker commented 4 years ago

I reverted headerbackgroundcolor to the previous. Not sure what border look you are going for. Adding class "display" changed the defaults. Feel free to alter styling. It might be worth removing the collapse entries and testing that with the defaults. datatables has an additional "cell-border" style which I didn't add. I think the current borders and collapse setting are still in place. I actually like the "compact" style. Considering adding a preferences toggle for that one. :) https://datatables.net/manual/styling/classes

epacke commented 4 years ago

Love the new icons, where did they come from?

It's very important that they're license and free of attribution etc so we don't get into legal trouble.

You mentioned this in the mail and I know this was a problem with some of the buttons before too, but the readability is quite bad when hovering over buttons: image

I will add this to the back log.

The Close Pool details button seems to have disappeared (same with Virtual server) image

This should be changed to authors (plural) and maybe point towards the Devcentral thread: image

You mentioned details section in your mail, what did you mean by that?

If you don't have time/energy to fix any of the above I am OK to merge now. Your call.

timriker commented 4 years ago

Love the new icons, where did they come from?

It's very important that they're license and free of attribution etc so we don't get into legal trouble.

You mentioned this in the mail and I know this was a problem with some of the buttons before too, but the readability is quite bad when hovering over buttons: image

I will add this to the back log.

The Close Pool details button seems to have disappeared (same with Virtual server) image

This should be changed to authors (plural) and maybe point towards the Devcentral thread: image

You mentioned details section in your mail, what did you mean by that?

If you don't have time/energy to fix any of the above I am OK to merge now. Your call.

My co-worker Jon built the icons. They are built from scratch and under no restrictions. The gear was a royalty free, free use pic originally before he modified it. The F5 logo came from the F5 media kit. I think it's valid use. If someone objects, we remove it.

Pushing fixes for the other issues you raised. Good catch!

timriker commented 4 years ago

Alright. ChangeLog updated, mentioned issues fixed. zip distro created. This is ready to merge. Wow, that's a lot of issues it closes. :)

epacke commented 4 years ago

Well done Tim!

epacke commented 4 years ago

I will update the instructions page and will ping you later if you're willing to give feedback?

timriker commented 4 years ago

Great! The help page could use more regex search examples. I added a few. The Folder rename is probably the biggest issue for the instructions. Now called just "underlay" instead of "Move...". The new config xml has more settings in it, so people upgrading will need to add those new settings.

epacke commented 4 years ago

I've uploaded the new version as beta on DC (figured we can let the brave people try it for a while before changing to stable), but I feel it would be more suitable for you to present the changes as you've done almost all of the work this release.

timriker commented 4 years ago

I commented on the thread:

https://devcentral.f5.com/s/feed/0D51T00006j4YiC

I wish comments were handled better on DC. I didn't see a way to link to my comment, for example.