Fix constance being accessed before database is ready and other fixes #341

gsnider2195 · 2024-11-13T21:04:34Z

Closes #340

What's Changed

Remove conditional logic for urlpatterns based on constance settings
Update existing API views that were affected by the above change to return a 404 if the integration is not enabled
Remove conditional logic for adding grafana navigation menu items
- We can't do this at init because it needs to access the database
- We can't modify the nav menu during runtime because core doesn't support removing nav menu entries dynamically
Add a "grafana disabled" view in case a user clicks on a grafana nav menu item when the grafana integration is disabled
Fix multiple bugs in the grafana views
- Incorrect view names
- Incorrectly using a reference to the model instead of the model itself
- Delete all grafana API files since there we no API views provided by grafana integration
- Fixed Nautobot v2.3 incompatibility caused by saved views not being able to determine the models' table classes
Fix dev environment nautobot_config.py to fall back to constance if environment variable is not used
Fix mattermost, msteams, slack and webex integration API views that were not requiring user to be logged in
Add exception handling for cases when diffsync is not installed, since it's marked as optional

Screenshots

glennmatthews · 2024-11-13T21:40:41Z

I'm curious about:

Fix mattermost, msteams, slack and webex integration API views that were not requiring user to be logged in

These are the endpoints for receiving messages from the chat platform, aren't they? I wouldn't expect them to require Nautobot user authentication.

gsnider2195 · 2024-11-13T21:42:17Z

nautobot_chatops/integrations/grafana/views.py

+            return TemplateResponse(
+                request=self.request,
+                template="nautobot_chatops_grafana/grafana_disabled.html",
+                context={},


This should probably be a redirect to another view but this works.

glennmatthews · 2024-11-13T21:41:32Z

nautobot_chatops/integrations/grafana/diffsync/sync.py

@@ -22,6 +28,9 @@ def run_dashboard_sync(overwrite: bool = False) -> Union[str, None]:
    Args:
        overwrite (bool): Overwrite Nautobot data and delete records that are no longer in Grafana.
    """
+    if not DIFFSYNC_INSTALLED:
+        raise ImportError("DiffSync is not installed. Please install DiffSync to use the grafana integration.")


ImportError for these calls feels weird to me - shouldn't it be something like RuntimeError?

I was going to use RuntimeError but then I decided to replicate the exception you would get if you tried to import diffsync in this function. It seemed more appropriate to use ImportError here but I'm happy to change it to RuntimeError if you think that's better here.

Would be my preference, but I don't feel strongly about it.

Might be good either way to add a Raises: section to the docstring documenting this behavior?

smk4664 · 2024-11-13T21:46:02Z

I'm curious about:

Fix mattermost, msteams, slack and webex integration API views that were not requiring user to be logged in

These are the endpoints for receiving messages from the chat platform, aren't they? I wouldn't expect them to require Nautobot user authentication.

Yeah, these need to be reverted. The API for these views is for the chat platform to communicate with Nautobot. Each of these views have their own way to authenticate the system. For instance, Slack signs their requests.

gsnider2195 · 2024-11-13T21:51:29Z

I'm curious about:

Fix mattermost, msteams, slack and webex integration API views that were not requiring user to be logged in

These are the endpoints for receiving messages from the chat platform, aren't they? I wouldn't expect them to require Nautobot user authentication.

Yeah, these need to be reverted. The API for these views is for the chat platform to communicate with Nautobot. Each of these views have their own way to authenticate the system. For instance, Slack signs their requests.

That's simple enough to do. I didn't have the context for why these were left wide open. These should probably also be changed to rest_framework views at some point instead of django views?

changes/341.housekeeping

glennmatthews · 2024-11-13T22:02:04Z

nautobot_chatops/urls.py

-    except Exception:
-        grafana_urlpatterns = []
-        logger = logging.getLogger(__name__)
-        logger.warning("Grafana ChatOps integration is not available.", exc_info=True)


Wondering - were there cases where importing the grafana URLs might fail here (missing optional dependency, etc.) that we need to handle still?

Probably. Our dockerfile installs with --extras all. I'll test without that.

Looks like I need to add more import guardrails 😞

If I just put this exception handling back in we get a broken nav menu.

This is fixed now. Discussed the implementation in DSU and decided not to spend too much time on it since ideally nobody will ever encounter this.

glennmatthews · 2024-11-13T22:03:13Z

I'm curious about:

Fix mattermost, msteams, slack and webex integration API views that were not requiring user to be logged in

These are the endpoints for receiving messages from the chat platform, aren't they? I wouldn't expect them to require Nautobot user authentication.

Yeah, these need to be reverted. The API for these views is for the chat platform to communicate with Nautobot. Each of these views have their own way to authenticate the system. For instance, Slack signs their requests.

That's simple enough to do. I didn't have the context for why these were left wide open. These should probably also be changed to rest_framework views at some point instead of django views?

They don't use DRF serializers AFAIK so I'm not sure why they would be changed over?

gsnider2195 · 2024-11-13T22:08:46Z

I'm curious about:

Fix mattermost, msteams, slack and webex integration API views that were not requiring user to be logged in

These are the endpoints for receiving messages from the chat platform, aren't they? I wouldn't expect them to require Nautobot user authentication.

Yeah, these need to be reverted. The API for these views is for the chat platform to communicate with Nautobot. Each of these views have their own way to authenticate the system. For instance, Slack signs their requests.

That's simple enough to do. I didn't have the context for why these were left wide open. These should probably also be changed to rest_framework views at some point instead of django views?

They don't use DRF serializers AFAIK so I'm not sure why they would be changed over?

Just so they don't have to manually disable csrf checks. This is the first place I've seen @method_decorator(csrf_exempt, name="dispatch") on an API view.

smk4664 · 2024-11-14T20:59:38Z

nautobot_chatops/models.py

-from .integrations.grafana.models import Dashboard as GrafanaDashboard
-from .integrations.grafana.models import Panel as GrafanaPanel
-from .integrations.grafana.models import PanelVariable as GrafanaPanelVariable
+from .integrations.grafana.models import GrafanaDashboard, GrafanaPanel, GrafanaPanelVariable


Oh good, I am glad that we can get rid of the rename. I am not sure why this wasn't done when originally implemented.

I had to leave it in for the diffsync code because the diffsync models are called GrafanaDashboard and GrafanaPanel and I didn't want to go down another rabbit hole trying to fix those...

smk4664

Thank you for the fixes!

gsnider2195 added 2 commits November 13, 2024 12:44

multiple fixes

b2c460b

require login on views

03e5df3

gsnider2195 requested review from glennmatthews, smk4664 and romanukes as code owners November 13, 2024 21:04

gsnider2195 added 2 commits November 13, 2024 13:28

add import guardrails

5d488fe

move more database access into functions instead of at import

807c624

gsnider2195 commented Nov 13, 2024

View reviewed changes

glennmatthews reviewed Nov 13, 2024

View reviewed changes

changelogs

67a5b63

gsnider2195 added 2 commits November 13, 2024 13:53

remove authentication from API endpoints

b051bfa

update docstrings

7eab0e8

glennmatthews reviewed Nov 13, 2024

View reviewed changes

add development environment changes

7087907

handle missing dependencies in grafana views

578535a

smk4664 reviewed Nov 14, 2024

View reviewed changes

smk4664 approved these changes Nov 14, 2024

View reviewed changes

gsnider2195 merged commit 4761790 into develop Nov 15, 2024
14 checks passed

gsnider2195 deleted the u/gas-340-constance-fix branch November 15, 2024 16:15

gsnider2195 mentioned this pull request Nov 18, 2024

Release/v3.1.1 #344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix constance being accessed before database is ready and other fixes #341

Fix constance being accessed before database is ready and other fixes #341

gsnider2195 commented Nov 13, 2024 •

edited

Loading

glennmatthews commented Nov 13, 2024

gsnider2195 Nov 13, 2024

glennmatthews Nov 13, 2024

gsnider2195 Nov 13, 2024

glennmatthews Nov 13, 2024

smk4664 commented Nov 13, 2024

gsnider2195 commented Nov 13, 2024

glennmatthews Nov 13, 2024

gsnider2195 Nov 13, 2024

gsnider2195 Nov 13, 2024

gsnider2195 Nov 14, 2024

glennmatthews commented Nov 13, 2024

gsnider2195 commented Nov 13, 2024

smk4664 Nov 14, 2024

gsnider2195 Nov 14, 2024

smk4664 left a comment

Fix constance being accessed before database is ready and other fixes #341

Fix constance being accessed before database is ready and other fixes #341

Conversation

gsnider2195 commented Nov 13, 2024 • edited Loading

Closes #340

What's Changed

Screenshots

glennmatthews commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smk4664 commented Nov 13, 2024

gsnider2195 commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennmatthews commented Nov 13, 2024

gsnider2195 commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smk4664 left a comment

Choose a reason for hiding this comment

gsnider2195 commented Nov 13, 2024 •

edited

Loading