Allow nova_api_audit_map.conf in DefaultConfigOverride#1104
Conversation
gibizer
left a comment
gibizer
left a comment
There was a problem hiding this comment.
Could you please providing reasoning why this changes is needed?
|
this might be requried for non default middelware? but we do not support modifying the mideddlware without a supprort exectpion so this probaly needs deicssion about how this will be tested and if it will be supproted in the product at all unless that has alreayd happend in either case any feature work shoudl be assocated with a jira issue in osprh so at a minimum that context need to be capture before this proceeds. |
|
this appares to be realted to https://opendev.org/openstack/pycadf/src/branch/master/doc/source/audit_maps.rst#L43 has never been suprpoted in optream nova with any testing and is not supproted in our downstream today. it might work but there is 0 testing or supprot for that form the nova maintaienr today i dont nessiarlly object to supprotign the config file but customer woudl not be able to use this without violdating the supprot for the nova api. if they have any api perfonce issue or geniss issue we woudl require them to repoduce with the default middelware set before there requrest would be supproted and all issue related to this woudl have to be feilded by the Secrity team or how eve fis the formal maintaienr of that in our downstrema so can you please provide the jira planing docs and work wirht teh relevent product owners to ensure this is planned properly before we proceed. |
|
We have a feature for providing documentation on how to enable and forward audit logs. I added the task to enable the nova_api_audit_map.conf customization in nova-operator in the PR description following an example I saw in some other PR here. @SeanMooney we have a whole feature based around audit logs https://redhat.atlassian.net/browse/RHOSSTRAT-1255 . Plan is to provide documentation on how to enable audit logs and how to configure openshift-logging to distinguish them and to forward them into a separate log storage. As part of the feature we'll also work on providing test coverage. Feel free to leave feedback if you want. |
SeanMooney
left a comment
SeanMooney
left a comment
There was a problem hiding this comment.
I have commented on the Jira, but this is not sufficient to enable this feature.
The CADF middleware that this is for uses RabbitMQ by default and falls back to the notification message bus if its own config options are not set:
https://opendev.org/openstack/keystonemiddleware/src/branch/master/keystonemiddleware/audit/__init__.py#L44-L66
It is not correct to set the transport_url via the CustomServiceConfig or the nova-api-audit-map.conf, as that would be in plain text. This means that today there is no protection from enabling the middleware via api-paste.ini when
you have not passed a notification bus.
This patch is also missing Kuttl tests to show that the feature works end-to-end in that environment. Ideally, this would be enabled in one of the EDPM jobs—either the Ceph job or the local storage job, whichever has notifications enabled.
The fact that middleware can use a RabbitMQ connection running in the Nova API process without our knowledge is also concerning from a performance and concurrency point of view, especially given the Eventlet removal efforts and the change to how that works now.
We do not allow long-running connections in the API process, and if it is killed and recreated on every request, that will also slow down every request by at least the TCP handshake time for every notification it sends.
|
We don't plan to use RabbitMQ or a notification bus, since we don't have anything, that'd listen there for these logs. The documentation will have users set the log driver for the audit middleware like this: Logs will then get collected by the openshift-logging and stored. I'll add kuttl-test coverage 👍 |
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
2 times, most recently
from
ed882bf to
b87609b
Compare
or we could default the value of use_oslo_messaging whey woudl still have to opt into the middlware by modifying the pipeline |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5fdad4322b6d4f1582634d34e221b439 ✔️ openstack-meta-content-provider SUCCESS in 2h 31m 55s |
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
from
b87609b to
652bfcd
Compare
SeanMooney
dismissed
their stale review
ill see if i can find time to re reivew but i think most of my feedback has been adressed
but i still want the compute folks to way in on if they are ok wiht this in general
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/202ae4c0635347cea19e10a20918927f ✔️ openstack-meta-content-provider SUCCESS in 3h 55m 35s |
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
from
652bfcd to
0545a2d
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/d2b200289b52491283b1b69f94c8ab0c ❌ openstack-meta-content-provider FAILURE in 5m 23s |
|
recheck download tools issue |
|
recheck |
|
Build failed (check pipeline). Post ❌ openstack-meta-content-provider NODE_FAILURE Node(set) request 100-0000082651 failed in 0s |
|
recheck |
|
Build failed (check pipeline). Post ✔️ openstack-meta-content-provider SUCCESS in 3h 23m 13s |
|
Build failed (check pipeline). Post ✔️ openstack-meta-content-provider SUCCESS in 2h 55m 44s |
|
recheck no logs |
|
Build failed (check pipeline). Post ✔️ openstack-meta-content-provider SUCCESS in 3h 16m 24s |
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
from
f25b7cf to
197975a
Compare
|
Build failed (check pipeline). Post ✔️ openstack-meta-content-provider SUCCESS in 2h 57m 21s |
|
recheck ceph job failed during make openstack while waiting for metallb operator to start |
|
Build failed (check pipeline). Post ✔️ openstack-meta-content-provider SUCCESS in 2h 52m 59s |
|
recheck timeout in tempest.scenario.test_network_basic_ops.TestNetworkBasicOps test in ceph job and POST_FAILURE without accessible logs in kuttl. |
|
@gibizer I'd say this is ready for reviews now. I think I addressed all of Sean's comments. To sum up why it's needed. We need to have audit logs from some chosen services (nova is among them). For that we need to be able to modify the api-paste.ini and add a nova_api_audit_map.conf file from https://github.com/openstack/pycadf/blob/master/etc/pycadf/nova_api_audit_map.conf . BTW, this isn't a completely new thing in RHOSO, we already have this doc https://docs.redhat.com/en/documentation/red_hat_openstack_services_on_openshift/18.0/html/hardening_a_red_hat_openstack_services_on_openshift_deployment/audit-system-events_hardening , which IMO really needs improvements ASAP. |
mrkisaolamb
left a comment
mrkisaolamb
left a comment
There was a problem hiding this comment.
Can we add a simple functional test to check the newly generated config section?
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
from
197975a to
617bd3d
Compare
|
Rebased on top of current main and implemented mrkisaolamb's suggestions (see the 3rd commit). |
|
Build failed (check pipeline). Post ✔️ openstack-meta-content-provider SUCCESS in 2h 55m 26s |
|
I believe kuttl-tests are just blocked on #1108 . And ceph job failed early on deploying nncp it seems. I'll wait until 1108 merges before trying again. |
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
from
617bd3d to
832887d
Compare
|
just a rebase on top of current main. |
Allow providing a nova_api_audit_map.conf file through the DefaultConfigOverride to NovaAPI. Conditionally configure the audit_middleware_notifications.use_oslo_messaging based on if notification_transport_url is defined or not. This is being tested in functional tests, kuttl tests and the nova-operator-tempest-multinode test job. Closes: https://redhat.atlassian.net/browse/OSPRH-29237
vyzigold
force-pushed
the
allow_nova_api_audit_map_conf
branch
from
bd26bac to
5bc3d42
Compare
|
Resolved mrkisaolamb's comments and squashed the commits. PR description was updated to reflect the added testing and conditional nova.conf config added later. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrkisaolamb, vyzigold The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
Allow providing a nova_api_audit_map.conf file through the
DefaultConfigOverride to NovaAPI.
Conditionally configure the
audit_middleware_notifications.use_oslo_messaging
based on if notification_transport_url is defined or not.
This is being tested in functional tests, kuttl tests and the
nova-operator-tempest-multinode test job.
Jira: OSPRH-29257