Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default Google API Alerts #2747

Closed

Conversation

joshw123
Copy link
Contributor

@joshw123 joshw123 commented Dec 5, 2024


Checklist

I applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

@joshw123 joshw123 changed the title Default api alerts Default Google API Alerts Dec 5, 2024
@joshw123 joshw123 mentioned this pull request Dec 5, 2024
4 tasks
@joshw123
Copy link
Contributor Author

joshw123 commented Dec 5, 2024

@ludoo @juliocc

Just bringing this over to the new PR, The reason I originally put it within the project module, is because for Compliance to pass, every project created needs to have these API Alerts configured, including the projects created in Bootrap, Resman etc... by putting this into Project Factory would mean projects created outside of stage2 project factory wouldn't have the alerts, and would need to be put in place by the user? Just wondering on thoughts :) Thank You!

@joshw123
Copy link
Contributor Author

joshw123 commented Dec 5, 2024

@ludoo @juliocc

Just bringing this over to the new PR, The reason I originally put it within the project module, is because for Compliance to pass, every project created needs to have these API Alerts configured, including the projects created in Bootrap, Resman etc... by putting this into Project Factory would mean projects created outside of stage2 project factory wouldn't have the alerts, and would need to be put in place by the user? Just wondering on thoughts :) Thank You!

As mentioned in the README.md, its generally best to default to true on the module itself to ensure that every project receives.

@joshw123
Copy link
Contributor Author

joshw123 commented Dec 5, 2024

@juliocc @ludoo Let me know your thoughts :) I can move if you think it will be best placed there


Although you may not use the services listed above, such as SQL, it is still important to monitor these events for compliance purposes

To enable these alerts by default on all projects created, it is recommended to default the variable `api_slerts` within `variables.tf` to true, along with adding a default email address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo api_slerts

@juliocc
Copy link
Collaborator

juliocc commented Dec 6, 2024

@juliocc @ludoo Let me know your thoughts :) I can move if you think it will be best placed there

I agree with @ludoo that the project module might not be the most suitable place for these metrics. If we go with the metrics in this PR, where do we draw the line? Why would we want these metrics but not, for example, GCE, GKE, or BigQuery metrics?

Beyond that, this PR is introducing log-based metrics, custom metrics, and alert policies. What if the user wants to customize or exclude some of these resources? I'm also concerned about potential permission issues around the creation of these new resources.

In another message you said that your goal is to enable these metrics within FAST projects. However, this approach forces all projects to have all these metrics, even if they just need some of them. I'd suggest implementing a more targeted approach by implementing these metrics within the specific projects where they are directly relevant—for instance, incorporating VPC-related metrics solely into networking projects.

An alternative approach could involve creating an 'observability factory' to manage these resources. Within FAST, you could have data folders to store the observability configuration specific to each project. @ludoo @sruffilli wdyt?

@ludoo
Copy link
Collaborator

ludoo commented Dec 6, 2024

@juliocc you probably dropped off the call before we got to this point yesterday, but the original requirement slightly change how I see this. What I got from the call (rough recap): customers who need ISO compliance / use SCC need to have specific API alerts at the project level for every project, even for disabled APIs. It sounds dumb, but yep that's how compliance works in my experience.

So while I agree the actual metric+alert recipes should not be in the project module, I might see a benefit of having an additional factory in the module that allows configuring metric+alert pairs from YAML files in a folder. This means that in FAST, you can point every stage creating projects to the same folder and get the same API alert policy applied everywhere.

A couple alternatives:

  • use the logging module for the same factory, then add optional support for it in FAST; this is slightly more complex as we would end up adding an extra module to every stage managing project creation (botostrap, tenants, net, sec, pf, etc)
  • use an extra module for the factory, which I kind of dislike as we'd end up promoting "meta-modules" which are really blueprints-in-disguise, and would probably end up with the same supportability issues we're having there

So all in all, I'm supportive of adding factory support in the project module, and then keeping the configurations outside of it. WDYT?

@juliocc
Copy link
Collaborator

juliocc commented Dec 6, 2024

@juliocc you probably dropped off the call before we got to this point yesterday, but the original requirement slightly change how I see this. What I got from the call (rough recap): customers who need ISO compliance / use SCC need to have specific API alerts at the project level for every project, even for disabled APIs. It sounds dumb, but yep that's how compliance works in my experience.

Indeed. I missed that part of the discussion. I'll reserve my opinion on the value of checkbox compliance for myself ;)

A couple alternatives:

  • use the logging module for the same factory, then add optional support for it in FAST; this is slightly more complex as we would end up adding an extra module to every stage managing project creation (botostrap, tenants, net, sec, pf, etc)
  • use an extra module for the factory, which I kind of dislike as we'd end up promoting "meta-modules" which are really blueprints-in-disguise, and would probably end up with the same supportability issues we're having there

What do you mean with "logging module"? Currently logging is embedded within the CRM modules (project, folder, organization).

So all in all, I'm supportive of adding factory support in the project module, and then keeping the configurations outside of it. WDYT?

Agreed. This sounds like a more sensible approach. We could even start curating a nice set of observability recipes like the ones proposed in this PR.

@ludoo
Copy link
Collaborator

ludoo commented Dec 6, 2024

Indeed. I missed that part of the discussion. I'll reserve my opinion on the value of checkbox compliance for myself ;)

😁

What do you mean with "logging module"? Currently logging is embedded within the CRM modules (project, folder, organization).

This, refactoring it to encompass more logging-related functionality.

So all in all, I'm supportive of adding factory support in the project module, and then keeping the configurations outside of it. WDYT?

Agreed. This sounds like a more sensible approach. We could even start curating a nice set of observability recipes like the ones proposed in this PR.

Sounds like we have a plan. :)

@ludoo
Copy link
Collaborator

ludoo commented Dec 8, 2024

Josh I'm closing this, we should rework this as described above. I can give it a shot this week, or let me know if you prefer to try your hand at this.

@ludoo ludoo closed this Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants