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

Refactor service agent management #2423

Merged
merged 20 commits into from
Jul 23, 2024
Merged

Refactor service agent management #2423

merged 20 commits into from
Jul 23, 2024

Conversation

juliocc
Copy link
Collaborator

@juliocc juliocc commented Jul 11, 2024

This PR significantly improves how Fabric handles service agents. This is a very large PR but IMO it will simplify things moving forward.

The main drivers for this PR are

  1. There many more service agents today than when we first designed service agents support in Fabric
  2. There are now APIs with more than one service agent
  3. New service agents are being added constantly

This PR introduces the following changes:

  1. Automated Service Agent List: A new script generates the list of service agents by parsing the official GCP documentation. This replaces the manual maintenance of service-agents.yaml. The idea is to run the script manually from time to time
  2. Separate Service Account Outputs: The project.service_accounts output is split into project.service_agents and project.default_service_accounts. The Cloud Service Service Agent is handled like any other service agent within service-agents.yaml.
  3. Standardized Terminology: The codebase now consistently uses the term "service agent", removing outdated terms like "robots" and "service identity"..
  4. Simplified CMEK Dependency Handling: Legacy CMEK dependency handling within the project module is removed. This logic is now the responsibility of users who require it.
  5. New Variable new top-level variable (service_agents_config) to control the default behaviour in regards to service agent handling. By default we enable:
    • Creation of primary service agents
    • Granting default permissions to service agents for any enabled API.
  6. Renamed Variable: The project.service_encryption_key_ids variable is renamed to project.service_encryption_key_ids for clarity. (Suggestions for a better name are welcome!)

There's a lot of noise in this PR as a lot of modules, blueprints and tests changed. For reviewers, please focus on the changes within the modules/project directory. While many files have been modified, the core logic changes reside there.

@juliocc juliocc requested review from wiktorn, ludoo and sruffilli July 11, 2024 14:37
@juliocc juliocc added the incompatible change Pull request that breaks compatibility with previous version label Jul 11, 2024
Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

This is really great. Dropped a couple of comments.

One more poitn we should maybe tackle is inter-service dependency: one example I'm currently facing in real life is Dataproc requiring KMS grants on the GCE, GCS, and its own service agent.

modules/project/variables.tf Outdated Show resolved Hide resolved
modules/project/variables.tf Show resolved Hide resolved
modules/project/service-agents.tf Show resolved Hide resolved
@juliocc juliocc force-pushed the jccb/service-agents branch from 73e6973 to bada713 Compare July 23, 2024 17:02
@juliocc
Copy link
Collaborator Author

juliocc commented Jul 23, 2024

Waiting for e2e to finish before merging

@juliocc juliocc merged commit c0bf32e into master Jul 23, 2024
17 checks passed
@juliocc juliocc deleted the jccb/service-agents branch July 23, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible change Pull request that breaks compatibility with previous version on:blueprints on:FAST on:modules on:tools New or changed tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants