Skip to content

[azure lfo] Detect existing LFO installs#4

Merged
agulen merged 6 commits into
mainfrom
altan/2509/existinglfo
Sep 15, 2025
Merged

[azure lfo] Detect existing LFO installs#4
agulen merged 6 commits into
mainfrom
altan/2509/existinglfo

Conversation

@agulen
Copy link
Copy Markdown
Member

@agulen agulen commented Sep 11, 2025

Overview

Jira

These changes will detect and report existing LFOs in the LFO onboarding script based on the scopes chosen at onboarding time. The logic goes as follows:

  • For each subscription being monitored for log forwarding
    • Search for a function app prefixed with resources-task. An LFO control plane will have this resource. The suffix of this function app is the control plane ID.
    • If found, note the function app's resource group, subscription ID, control plane ID, and the value of its MONITORED_SUBSCRIPTIONS environment variable. This env var contains the sub IDs that are being monitored for log forwarding. In the future, if the user decides to expand/contract their scopes, this env var will get updated.
  • If these artifacts are found, build a dictionary that maps control plane ID to the metadata stated above. This information will get communicated back to the front-end in a future change.
  • If these artifacts are not found, continue with the installation as normal.

Testing

  • Unit tests added
  • Manual test looks good:
image

Edge Case Gap

There is a gap in this current setup where the LFO detection is limited by the user's permissions who is executing the script. If there is an existing LFO that lives in a place that the user doesn't have access to in Azure, this script won't be able to detect it. The team has discussed the front end doing a logs query on the Datadog side for LFO logs to mitigate this edge case.

@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Sep 11, 2025

⚠️ Code Quality    ✅ Code Vulnerabilities    ✅ Library Vulnerabilities

⚠️ Warnings

🛠️ 1 Code quality issue detected

Low: python-best-practices/comment-fixme-todo-ownership TODO and FIXME comments must have ownership View rule
azure/logging_install/src/azure_logging_install/main.py:149

ℹ️ Info

🛡️ No new code vulnerabilities
📚 No new vulnerable libraries detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: a9aca84 | Docs | Was this helpful? Give us feedback!


set_subscription(config.control_plane_sub_id)
if existing_lfos:
# TODO AZINTS-3894: Report state of azure env to front end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Low: Code Quality Violation

Suggested change
# TODO AZINTS-3894: Report state of azure env to front end
# TODO(<owner>) AZINTS-3894: Report state of azure env to front end
comments must have ownership (...read more)

When using TODO or FIXME, specify who write the annotation. It's a best practice to remind you who created the annotation and have potential context and information about this issue.

View in Datadog  Leave us feedback  Documentation

Comment thread azure/logging_install/src/azure_logging_install/main.py Outdated
@agulen agulen requested review from benjjs and gpalmz September 11, 2025 20:23

set_subscription(config.control_plane_sub_id)
if existing_lfos:
# TODO AZINTS-3894: Report state of azure env to front end
Copy link
Copy Markdown

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Low: Code Quality Violation

Suggested change
# TODO AZINTS-3894: Report state of azure env to front end
# TODO(<owner>) AZINTS-3894: Report state of azure env to front end
comments must have ownership (...read more)

When using TODO or FIXME, specify who write the annotation. It's a best practice to remind you who created the annotation and have potential context and information about this issue.

View in Datadog  Leave us feedback  Documentation

@gpalmz
Copy link
Copy Markdown
Collaborator

gpalmz commented Sep 12, 2025

What's the deal with the corner case where the user running the script doesn't have "read" permission for an existing lfo? Mind adding a callout to the desc if that's still applicable?

@agulen
Copy link
Copy Markdown
Member Author

agulen commented Sep 12, 2025

What's the deal with the corner case where the user running the script doesn't have "read" permission for an existing lfo? Mind adding a callout to the desc if that's still applicable?
@gpalmz

We discussed doing a DD logs query for any log with the tag forwarder:lfo. If we see those logs, then we know the org has LFO set up somewhere already. Will call out though, thx.

@gpalmz
Copy link
Copy Markdown
Collaborator

gpalmz commented Sep 12, 2025

What's the deal with the corner case where the user running the script doesn't have "read" permission for an existing lfo? Mind adding a callout to the desc if that's still applicable?
@gpalmz

We discussed doing a DD logs query for any log with the tag forwarder:lfo. If we see those logs, then we know the org has LFO set up somewhere already. Will call out though, thx.

Super super edge case... What if an LFO is set up but it is failing to forward logs/there are no logs to forward/logs have not yet been forwarded?

@agulen
Copy link
Copy Markdown
Member Author

agulen commented Sep 12, 2025

@gpalmz
That possibility is still open, yes. That would have to happen on top of the first edge case where we have multiple admins doing a log collection setup who don't have permissions into the same azure subscriptions.

@gpalmz
Copy link
Copy Markdown
Collaborator

gpalmz commented Sep 12, 2025

@gpalmz That possibility is still open, yes. That would have to happen on top of the first edge case where we have multiple admins doing a log collection setup who don't have permissions into the same azure subscriptions.

Ok sounds good I think we can live with that. Hopefully we never have a support case with this scenario cause golly gee

@agulen
Copy link
Copy Markdown
Member Author

agulen commented Sep 12, 2025

Yup, agreed. This gap exists in LFO today. The uninstall script can at least detect multiple LFO installs and let the user choose which one to remove. That would be our recommended action to the user. It still has the same limitation that it's constrained by the user's azure permissions, but we could at least get them out of the bad state by having one user run it.

Copy link
Copy Markdown
Collaborator

@benjjs benjjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you were already planning this in a follow up PR, but we are interested in the filtering settings of existing LFOs as well, so that we can populate the interface when there is exactly one existing LFO and we are extending it.

@agulen
Copy link
Copy Markdown
Member Author

agulen commented Sep 15, 2025

@benjjs Good point, I can return the PII and tag filtering settings. Will add that in a follow-up PR.

@agulen agulen merged commit c3be359 into main Sep 15, 2025
2 checks passed
@agulen agulen deleted the altan/2509/existinglfo branch September 15, 2025 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants