Firewall attribute "Shared" is set to None by default instead of 'False'

[vishwanath jayaraman]

In the current implementation, when a firewall is created, the default value of the attribute 'Shared' is set to 'None' instead of 'False'. When Firewall attributes are seen from Horizon, the display value is shown as 'Maybe' instead of 'No' due to value being 'None'.

[Akihiro Motoki (amotoki)]

After a quick look around the code, I noticed two problems.

- 'shared' attribute in firewall resource is not passed to DB entry. [1]
- 'shared' attribute is marked as 'visible=False' in the attribute map, so 'shared' attribute does not exist in the API response. [2]

If we read the code and FWaaS API literally, it seems that changing the above two points fixes the bug.

[1] https://github.com/openstack/neutron-fwaas/blob/master/neutron_fwaas/db/firewall/firewall_db.py#L309
[2] https://github.com/openstack/neutron-fwaas/blob/master/neutron_fwaas/extensions/firewall.py#L323

[Akihiro Motoki (amotoki)]

However, I have a question on how a 'shared' firewall works, especially with router-insertion.

I am not sure why the 'shared' attribute is marked as 'invisible' in the attribute map (i.e. API response).
I wonder if there is some decision during the initial implementation of FWaaS API.
Sumit or others who was involved in the initial effort may be able to answer it.
It seems better to reach them to clarify the background.

If 'shared' firewall does not work, we should not expose 'shared' attribute
rather than changing two points I mentioned in the previous comment.

[vishwanath jayaraman (vishwanathj)]

Akhiro,
 Thanks for your detailed analysis.
 I have subscribed Sumit and Sridar to this bug for their comments. Seems to me that tenants MAY NOT want to share firewall with other tenants.
We can always fix the Horizon code to not display the Shared attribute field for firewall as last resort. However, I will wait to hear Sumit and Sridar's comments.

[Sridar Kandaswamy (skandasw)]

I too don't have all the history on this, will wait on Sumit.

Akihiro - possibly this is just bug on [1] that u mention above. [2] i am not so sure. Vish, from a usage perspective, the shared is really to set this attribute for an admin owned resource but there is no way to discriminate this to a specific or subset of tenants.

[Sumit Naiksatam (snaiksat)]

Hi, Sorry for chiming in late. The original idea behind using the "shared" attribute was to cover the use case where an operator owned firewall resource could be shared between tenants. However, this has data-path implications beyond just the sharing of the resource in the DB, and was difficult to achieve within Neutron. Hence we decided to keep the attribute for future use (and mark it as "invisible").

If this translates to a "maybe" status in the UI, I definitely agree that its confusing. I would recommend not exposing it to the user until its actually used. Of course, one could argue why we should have this attribute at all in the first place if its usage is not anticipated in the immediate future.

[vishwanath jayaraman (vishwanathj)]

@Sumit, Thanks for your comments.
Looks like removing the display from UI seems like the optimal approach at this time.
I will also wait for Akhiro to weigh in with his comments before we decide to remove the display from Horizon UI.

[Akihiro Motoki (amotoki)]

In Neutron side, 'shared' attribute for firewall has no effect, so I don't think we need any change in DB operation. We can keep the current implementation as-is.

In Horizon side, we should drop 'shared' attribute from FirewallsTable in dashboards/project/firewalls/tables.py
because it has no effect and 'shared' attribute is invisible to end users.

In neutronclient, it is better to drop --shared flag because 'shared' attribute is not supported by the design decision.

From the above reason, I will add neutronclient and horizon to the affected projects.

[Akihiro Motoki (amotoki)]

We decided not to change neutron-fwaas code, so I mark it Won't fix in the neutron project.

[Akihiro Motoki (amotoki)]

@vishwanath
If you would like to make a change in neutronclient or horizon, feel free to take the bug from me.

[Akihiro Motoki (amotoki)]

Not sure why gerrit reviews are not linked.

neutronclient https://review.openstack.org/194652
horizon https://review.openstack.org/#/c/194621/

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/198089

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/194652
Committed: https://git.openstack.org/cgit/openstack/python-neutronclient/commit/?id=52718908bf0ef189c25e2db74905913df8034a79
Submitter: Jenkins
Branch: master

commit 52718908bf0ef189c25e2db74905913df8034a79
Author: Akihiro Motoki <email address hidden>
Date: Tue Jun 23 21:17:35 2015 +0900

    Remove --shared option from firewall-create

    Neutron FWaaS firewall object does not support 'shared'
    attribute at now and 'shared' attribute has no meaning.
    It is confusing to have --shared option in CLI side.

    Change-Id: Ia737b80d7b8a397f06fdf6d6ea8a5cd64d9036a4
    Closes-Bug: #1465440

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/198089
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=fea681e20fd0fbb3d24079475cca650f30720486
Submitter: Jenkins
Branch: stable/kilo

commit fea681e20fd0fbb3d24079475cca650f30720486
Author: Akihiro Motoki <email address hidden>
Date: Tue Jun 23 21:09:11 2015 +0900

    Remove 'shared' from Neutron Firewall table

    Neutron FWaaS firewall object does not support 'shared' attribute
    at now and 'shared' attribute is not included in API response.
    This commmit removes 'shared' attribute for firewall object.
    For more detail of the background, please see the bug report.

    Note that 'shared' attributes for firewall rule and policy
    are supported. This commit only affect the firewall object.

    (Pulled from gate, failing unit tests)

    Change-Id: I5787529395e0005ce1efee48beb6f0f688c1c736
    Closes-Bug: #1465440
    (cherry picked from commit 991d8ac86c12fa45e1275851711d6feb2d9891a2)