Skip to content

Conversation

@mist714
Copy link
Member

@mist714 mist714 commented Oct 10, 2025

Support for multiple principals separated by commas.

  • Annotation accepts comma‑separated Google service account emails / AWS arn.
  • First secret keeps the original (unchanged) name; additional ones get -1, -2, ...

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for multiple Google Service Account (GSA) emails in the googlecloud-service-account-email annotation by accepting comma-separated values. The controller now creates separate secrets for each GSA email, with the first secret keeping the original name and additional secrets getting numbered suffixes (-1, -2, etc.).

  • Support for parsing comma-separated GSA emails from annotations
  • Creation of multiple indexed secrets for each GSA email
  • Cleanup logic to handle removal of GSA emails and their corresponding secrets

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
internal/controller/serviceaccount_controller.go Main logic for handling multiple GSA emails, creating indexed secrets, and managing cleanup
internal/controller/config.go Added secretNameIndexed function to generate numbered secret names
internal/controller/serviceaccount_controller_test.go Test cases for multiple GSA email scenarios and cleanup behavior
Comments suppressed due to low confidence (1)

internal/controller/serviceaccount_controller.go:1

  • The condition should be idx == 0 instead of idx <= 0. Negative indices don't make sense in this context and would incorrectly return the base secret name.
/*

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mist714 mist714 changed the title Support for multiple GSAs separated by commas Support for multiple identity separated by commas Oct 15, 2025
@mist714 mist714 requested a review from ordovicia October 15, 2025 07:18
@ordovicia
Copy link
Contributor

evictor.go should also be modified to support multiple principals. It identifies and evicts pods that are failing to pull images because provisioned image pull secrets are not attached yet.

return newExp, nil
}

func (r *serviceAccountReconciler) shouldCreateOrRefreshImagePullSecret(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: When principals annotated in ServiceAccount change, we need to recreate image pull secrets for the new principals.
This issue is not directly related to this PR, so I will fix it in a separate PR.

},
}

It("Create and attach multiple Secrets (two emails)", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add some tests for

  • refreshing secrets
  • cleaning up secrets

when SA has multiple principals annotation?

Copy link
Contributor

@ordovicia ordovicia left a comment

Choose a reason for hiding this comment

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

Thank you, good job!

@ordovicia ordovicia merged commit 432384e into pfnet:main Oct 17, 2025
3 checks passed
@github-actions github-actions bot mentioned this pull request Oct 17, 2025
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.

2 participants