firewall: Show any included services on a service #12385

croissanne · 2019-07-22T11:55:58Z

marusak · 2019-07-22T12:39:50Z

Cool stuff :)
I was at first just rather confused :D Have you talked about this with @garrett or @andreasn ?
I know that not always there has to be Details tab. But why the second one has FreeIPA 4 server (itself) but first has Details and not DNS?
Maybe it would be better to have just one tab Included Services and having some listing in there?
What do you think @garrett ?

croissanne · 2019-07-22T13:24:01Z

@marusak yea I wasn't sure about the design at all. More consistency there would be good I agree.

garrett · 2019-07-22T13:51:02Z

This is really cool.

I've always thought it's a little odd to have 1 tab for details.

I wonder if it makes sense to remove the tabs altogether and show all the information below.

Is there a service that pulls in more services than FreeIPA currently?

croissanne · 2019-07-22T15:00:07Z

@garrett I think freeipa-4 includes quite a lot. Most are about 4-5.

marusak · 2019-07-23T08:28:25Z

Needs to rebase to master since #12367 changed tests names

martinpitt

Very nice! Just one fixme in the tests, the rest looks good.

martinpitt · 2019-07-24T05:28:44Z

pkg/networkmanager/firewall-client.js

+                    }
+                    // We can't use Promise.all() here until cockpit is able to dispatch es2015 promises
+                    // https://github.com/cockpit-project/cockpit/issues/10956
+                    // eslint-disable-next-line cockpit/no-cockpit-all


This should actually be obsolete now. But let's keep it here, and we can drop all cockpit.all() in a separate PR.

martinpitt · 2019-07-24T05:34:44Z

test/verify/check-networking-firewall

+            addService('freeipa-4')
+            b.click("tr[data-row-id='freeipa-4']")
+            b.click("tbody.open a:contains(kerberos)")
+            b.wait_present("tbody.open .listing-ct-body:contains(Included service)")


Please do an "else:" here, so that we have a reminder (failing test) once firewalld 0.7 trickles into distros. For example, debian-testing already has 0.7, so please add it here.

I think our debian-testing is still buster?

But i will add the else!

croissanne · 2019-07-24T11:33:24Z

@garrett Would the updated photo's design work better?

garrett · 2019-07-24T13:29:19Z

What if the we ditched the tabs entirely and had the services listed under the details text?

Something like this:

FreeIPA is an LDAP and Kerberos domain controller for Linux systems. Enable this option if you plan to provide a FreeIPA Domain Controller using the LDAP protocol. You can also enable the 'freeipa-ldaps' service if you want to provide the LDAPS protocol. Enable the 'dns' service if this FreeIPA server provides DNS services and 'freeipa-replication' service if this FreeIPA server is part of a multi-master replication setup.

Included Services

WWW (HTTP), port 80: HTTP is the protocol used to serve Web pages. If you plan to make your Web server publicly available, enable this option. This option is not required for viewing pages locally or developing Web pages.

Secure WWW (HTTPS), port 443: HTTPS is a modified HTTP used to serve Web pages when security is important. Examples are sites that require logins like stores or web mail. This option is not required for viewing pages locally or developing Web pages. You need the httpd package installed for this option to be useful.

Kerberos, port 88: Kerberos network authentication protocol server Kpasswd: Kerberos password (Kpasswd) server

LDAP, port 389: Lightweight Directory Access Protocol (LDAP) server

LDAPS, port 636: Lightweight Directory Access Protocol (LDAP) over Secure Sockets Layer (SSL) server

It's just a heading (probably an h4 or h5 — whatever is 1 more than the previous heading), which would be styled to be no larger than the row's text (so we'll probably need to do font-size: inherit on it here), then a <li> with <strong> for each item's title.

I don't think it would be too tall to do it this way. As there's nothing but informative text, I'd like it if we could keep it all together in the details.

If we eventually add more features, we could then we could reconsider tabs. But I'm not sure what features that would be. For example: Editing is about which zone the service is in really... or if it's a custom rule, then the ports selected too. (In other words: editing would be in a dialog, not inline in an expanded row.) So if not editing, then what? This is why I don't think we need tabs here, either in the present or the future.

TL;DR: Bulleted list with a heading. Names of services would be in a <strong> tag. No tabs.

marusak · 2019-07-29T09:21:03Z

Looks great now! :) Both debian stable and testing failed in testFirewallPage exactly in the same way which seems suspicious. I've retried it, lets see

pkg/lib/cockpit-components-listing.jsx

marusak · 2019-07-29T09:39:50Z

pkg/lib/cockpit-components-listing.jsx

-                                </ul>
-                            </div>
-                            {tabs}
+                            { tabs && heading }


this tabs just tests if tabs is defined and if is, then it shows heading? Wouldn't it make sense to just show heading when defined? ({ heading })
Or wouldn something like simpleBody || (heading && tabs) work? (reading like "set simpleBody or heading followed by tabs")

marusak · 2019-07-29T09:46:16Z

I am afraid that tests are broken on debian. I've seen 4 results and all 4 failed the same way on testFirewallPage. Unfortunately needs work

martinpitt

Thanks! This is much more elegant.

pkg/networkmanager/firewall.jsx

martinpitt · 2019-08-02T07:00:40Z

Too many flakes, retrying a bit. Please self-land when all firewall tests pass.

martinpitt · 2019-08-02T08:48:01Z

Still two firewall failures, retrying more.

needs rework for changed firewalld API

Fixes rhbz#1721548 Closes cockpit-project#12385

martinpitt

Thanks!

martinpitt · 2019-08-05T13:50:57Z

pkg/networkmanager/firewall-client.js

+                })
+                .then(path => firewalld_dbus.call(path[0],
+                                                  'org.fedoraproject.FirewallD1.config.service',
+                                                  'getSettings2', []))


Follow-up: Start with this call, and if it fails, fall back to the old one and set includes to empty. Saves one call on new firewalld, and on older versions there's no change (one succeeding and one failing call).

Right, that would make a lot more sense. I'll do it as a followup, this one has been in the pipeline too long :p

Fixes rhbz#1721548 Closes #12385

martinpitt · 2019-08-06T08:06:27Z

@Gundersanne : can you please send a PR against the rhel-8.1 branch with this backported?

croissanne · 2019-08-06T08:37:09Z

@martinpitt sure, will be my first backport :D

martinpitt · 2019-08-08T05:53:53Z

@Gundersanne: Sorry, can you please do the screenshot slightly wider (to avoid the label wrap) and have some more context? Right now it's hard to see from where this actually happens.

croissanne · 2019-08-08T07:45:07Z

@martinpitt

croissanne force-pushed the bz-1721548 branch from fc49b66 to e5d5107 Compare July 22, 2019 11:57

croissanne force-pushed the bz-1721548 branch from e5d5107 to ac79888 Compare July 22, 2019 13:23

marusak added the needs-rebase label Jul 23, 2019

martinpitt requested changes Jul 24, 2019

View reviewed changes

croissanne added needs-rebase and removed needs-rebase labels Jul 24, 2019

croissanne force-pushed the bz-1721548 branch from ac79888 to 7018d64 Compare July 24, 2019 10:11

croissanne force-pushed the bz-1721548 branch from 7018d64 to f8d72d1 Compare July 24, 2019 11:33

croissanne force-pushed the bz-1721548 branch from f8d72d1 to 20b5b95 Compare July 25, 2019 08:16

croissanne added needswork and removed needs-rebase labels Jul 25, 2019

croissanne force-pushed the bz-1721548 branch from 20b5b95 to c24449b Compare July 25, 2019 09:46

croissanne removed the needswork label Jul 29, 2019

marusak reviewed Jul 29, 2019

View reviewed changes

marusak added the needswork label Jul 29, 2019

croissanne force-pushed the bz-1721548 branch from f7acd51 to 49d3e7c Compare August 1, 2019 15:17

martinpitt previously approved these changes Aug 2, 2019

View reviewed changes

pkg/networkmanager/firewall.jsx Show resolved Hide resolved

lib: cockpit-listing node as body instead of tabs

2efa7fc

croissanne added the blocked Don't land until something else happens first (see task list) label Aug 2, 2019

croissanne force-pushed the bz-1721548 branch from 49d3e7c to 17c1085 Compare August 2, 2019 13:29

croissanne removed the blocked Don't land until something else happens first (see task list) label Aug 2, 2019

croissanne requested review from martinpitt and removed request for martinpitt August 5, 2019 11:19

croissanne force-pushed the bz-1721548 branch from 17c1085 to 58f4345 Compare August 5, 2019 11:28

croissanne requested a review from martinpitt August 5, 2019 11:28

firewall: Show any included services on a service

5f99630

Fixes rhbz#1721548 Closes cockpit-project#12385

croissanne force-pushed the bz-1721548 branch from 58f4345 to 5f99630 Compare August 5, 2019 11:29

lib/listing: Remove font px hardcoding

9f0e2df

martinpitt approved these changes Aug 5, 2019

View reviewed changes

martinpitt merged commit 5e5a5e4 into cockpit-project:master Aug 6, 2019

martinpitt pushed a commit that referenced this pull request Aug 6, 2019

firewall: Show any included services on a service

a960703

Fixes rhbz#1721548 Closes #12385

martinpitt added release-note and removed release-note labels Aug 6, 2019

croissanne deleted the bz-1721548 branch September 23, 2019 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

firewall: Show any included services on a service #12385

firewall: Show any included services on a service #12385

croissanne commented Jul 22, 2019 •

edited

Loading

marusak commented Jul 22, 2019

croissanne commented Jul 22, 2019

garrett commented Jul 22, 2019

croissanne commented Jul 22, 2019

marusak commented Jul 23, 2019

martinpitt left a comment

martinpitt Jul 24, 2019

martinpitt Jul 24, 2019

croissanne Jul 24, 2019 •

edited

Loading

croissanne commented Jul 24, 2019

garrett commented Jul 24, 2019

Included Services

marusak commented Jul 29, 2019

marusak Jul 29, 2019

marusak commented Jul 29, 2019

martinpitt left a comment

martinpitt commented Aug 2, 2019

martinpitt commented Aug 2, 2019

martinpitt left a comment

martinpitt Aug 5, 2019

croissanne Aug 5, 2019

martinpitt commented Aug 6, 2019

croissanne commented Aug 6, 2019

martinpitt commented Aug 8, 2019

croissanne commented Aug 8, 2019 •

edited

Loading

firewall: Show any included services on a service #12385

firewall: Show any included services on a service #12385

Conversation

croissanne commented Jul 22, 2019 • edited Loading

marusak commented Jul 22, 2019

croissanne commented Jul 22, 2019

garrett commented Jul 22, 2019

croissanne commented Jul 22, 2019

marusak commented Jul 23, 2019

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Jul 24, 2019

Choose a reason for hiding this comment

martinpitt Jul 24, 2019

Choose a reason for hiding this comment

croissanne Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

croissanne commented Jul 24, 2019

garrett commented Jul 24, 2019

Included Services

marusak commented Jul 29, 2019

marusak Jul 29, 2019

Choose a reason for hiding this comment

marusak commented Jul 29, 2019

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented Aug 2, 2019

martinpitt commented Aug 2, 2019

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Aug 5, 2019

Choose a reason for hiding this comment

croissanne Aug 5, 2019

Choose a reason for hiding this comment

martinpitt commented Aug 6, 2019

croissanne commented Aug 6, 2019

martinpitt commented Aug 8, 2019

croissanne commented Aug 8, 2019 • edited Loading

croissanne commented Jul 22, 2019 •

edited

Loading

croissanne Jul 24, 2019 •

edited

Loading

croissanne commented Aug 8, 2019 •

edited

Loading