feat(apps/prod2/tirelease): prepare deployment for tibuild-v2 migration - #2209
Conversation
- Extend tibuild-v2 product_repo_map with the products tirelease builds in hotfix flows (br/dumpling/tidb-lightning/ticdc-newarch/tici/ enterprise-plugin/ng-monitoring/tidb-binlog/drainer/pump/ tidb-dashboard/tidb-operator/tidb-tools/tiproxy), otherwise create requests fail with 400 'github full repo not found'. - Add the tibuild client config section (baseURL/audience/ ociFileserverURL/createdBy/saKey) to the tirelease configmaps; the saKey file is mounted via the existing tirelease-secret-config. Companion: PingCAP-QE/tirelease#697 (migrate tibuild client to tibuild-v2), PingCAP-QE/ee-apps#590 (binaries platform in build report).
ti-chi-bot
Bot
added
do-not-merge/invalid-title
area/apps
env/GCP
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR prepares deployment configuration for migrating to tibuild-v2 by expanding the product_repo_map in the external secret config to cover additional products used in tirelease hotfix builds, and by adding a new tibuild config section (with baseURL, audience, ociFileserverURL, createdBy, and saKey) to tirelease configmaps. The approach is straightforward and focused on configuration updates with clear documentation about required manual GCP steps. The code changes are minimal and mostly declarative YAML, so quality is good with no obvious errors.
Critical Issues
- None found. The changes are purely configuration and do not introduce logic or runtime code changes.
Code Improvements
-
apps/gcp/tibuild/v2/external-secret-config.yaml (line 44-60)
The mapping for new products uses some duplication of repository names (e.g.,br,dumpling,tidb-lightningall map topingcap/tidb). This is presumably intentional, but consider adding a short comment explaining why these map topingcap/tidbto improve maintainability and avoid confusion for future editors. -
apps/prod2/tirelease/configmaps.yaml (line 18-24 and 51-57)
Thetibuildconfig block is duplicated under two data sections, presumably for different environments or namespaces. To reduce duplication and risk of divergence, consider referencing a shared config or using a Helm value or Kustomize patch if the deployment supports it.
Best Practices
-
Documentation
-
Add a brief comment in
external-secret-config.yamlabove the expandedproduct_repo_mapblock to clarify the purpose of the additional mappings, e.g.:# Additional products required for tirelease hotfix builds to prevent 400 errors from tibuild-v2 create API -
In
configmaps.yaml, clarify what thecreatedByfield represents (e.g., contact person or automation user), and mention thatsaKeycorresponds to the secret key name mounted from GCP Secret Manager.
-
-
Testing Coverage
- Since these are config changes, it’s important to ensure integration tests or deployment validation pipelines verify the correctness of these configs and the ability of tibuild-v2 to create builds with these products and config entries.
- If not done, add or update tests to cover tibuild-v2 integration with these new product mappings and config.
-
Naming Conventions
- The key
tibuild_sa_keyis used as a secret name; confirm that this naming is consistent with existing secret keys and documented in your secret management standards.
- The key
Summary of Suggestions
# apps/gcp/tibuild/v2/external-secret-config.yaml
# Additional products required for tirelease hotfix builds to prevent 400 errors from tibuild-v2 create API
br: pingcap/tidb # br repo mapped to tidb because ...
dumpling: pingcap/tidb
...
# apps/prod2/tirelease/configmaps.yaml
# tibuild config for tibuild-v2 integration
# createdBy: contact person/email for this config
# saKey: name of service account key secret mounted from GCP Secret Manager
tibuild:
baseURL: ...
...- Consider DRY’ing the duplicated tibuild config in
configmaps.yamlby using Helm/Kustomize or a shared config. - Ensure integration tests cover these expanded product mappings and tibuild-v2 config usage.
- Document manual GCP steps in team runbooks or README if not yet done.
Overall, this PR is well scoped and low risk. Attention to documentation and test coverage will help maintainability and smooth migration.
Add a data entry to the tirelease-secret-config ExternalSecret that fetches the GCP Secret Manager secret tibuild-v2-api-client-sa-key (whole payload = SA key JSON) and writes it into the target secret as tibuild_sa_key, so it is mounted at /etc/secret-config/tibuild_sa_key in all tirelease deployments (main/jobs/webhook).
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR prepares deployment configurations for the tibuild-v2 migration by expanding the product repository map for external secrets and adding tibuild-related config sections to tirelease configmaps. It also introduces a new external secret entry for the tibuild service account key. The changes are straightforward YAML config additions and updates with no complex logic, and the PR includes clear instructions for required manual GCP operations. Overall, the changes are well-scoped and documented for this migration step.
Critical Issues
- None found: The PR contains configuration updates without logic changes, so no immediate bugs or security issues are identified.
Code Improvements
-
Clarify product_repo_map comments (file:
apps/gcp/tibuild/v2/external-secret-config.yaml, lines ~40-60)
The comment# extra products required by tirelease hotfix buildsis helpful, but it would improve maintainability to explicitly mention why some products map topingcap/tidb(e.g., shared repo) and others to different repos. This helps future maintainers understand repo-product relationships better.Suggested enhancement:
# Additional products needed for tirelease hotfix builds. # Note: Some products like br/dumpling point to pingcap/tidb repo as they share the same codebase.
-
Avoid hardcoding email in configmaps (file:
apps/prod2/tirelease/configmaps.yaml, lines ~18-26 and 51-59)
ThecreatedBy: xiaozhou.zhu@pingcap.comfield is hardcoded. If this config is used widely or by multiple people, it might be better to make it configurable or document when/how to update it. -
Add version or comment about config compatibility (all updated configmaps)
Consider adding aversionormigrationcomment in configmaps to track the tibuild-v2 migration related changes explicitly. This assists future debugging or rollbacks.
Best Practices
-
Add more descriptive comments for new config keys (multiple files)
For example, inapps/prod2/tirelease/configmaps.yaml, the newtibuildsection keys (baseURL,audience,ociFileserverURL,createdBy,saKey) could have brief inline comments describing their purpose and usage context, e.g.:tibuild: baseURL: https://prow.tidb.net/tibuild # Base URL for tibuild-v2 API audience: prow.tidb.net # OAuth2 audience for token validation ociFileserverURL: https://prow.tidb.net/dl/oci-file # OCI file server URL used by tibuild createdBy: xiaozhou.zhu@pingcap.com # Owner/creator of this config saKey: tibuild_sa_key # Secret key filename for tibuild service account
-
Document mounting path for SA key in external-secret.yaml (file:
apps/prod2/tirelease/external-secret.yaml)
The existing comment mentions the mount path/etc/secret-config/tibuild_sa_keybut it could be more explicit about how the app references this key (e.g., environment variables, file path usage). -
Testing coverage
Since these are config updates tied to deployment and authentication, ensure the associated services (tibuild-v2, tirelease) have end-to-end tests validating that these new configs work as expected, especially authentication flows. Add testing instructions if missing in the PR description. -
Style and formatting consistency
The indentation and formatting are consistent. Just ensure the comments use the same style (e.g., start with uppercase, punctuation) across all YAML files for polish.
Summary of action points:
- Add clarifying comments on product_repo_map entries
- Consider making
createdByfield configurable or document update policy - Add inline comments to tibuild config keys for clarity
- Expand comments on SA key usage/mount path
- Confirm testing coverage or add testing instructions in PR description
These improvements will enhance maintainability and reduce friction during the tibuild-v2 migration.
Rename the Secret Manager secret referenced for the tibuild-v2 SA key from tibuild-v2-api-client-sa-key to tirelease-tibuild-sa-key so the secret id makes clear the key is issued for tirelease.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR prepares deployment configurations necessary for migrating to tibuild-v2, primarily by expanding the product_repo_map with more product entries, adding tibuild-related config sections to tirelease configmaps, and injecting a new service account key secret for tibuild-v2 API authentication. The changes are mostly declarative YAML modifications and are well-scoped for the migration support. Overall, the changes are clear and well-documented, including manual steps for GCP preparation.
Code Improvements
-
apps/prod2/tirelease/external-secret.yaml (lines 15-23)
TheremoteRef.keyspecified istirelease-tibuild-sa-keybut the manual steps indicate the secret name should betibuild-v2-api-client-sa-key. This inconsistency may cause secret fetch failures.Issue:
The secret name in external-secret.yaml should match the one created in GCP Secret Manager.Suggestion:
UpdateremoteRef.keytotibuild-v2-api-client-sa-keyto align with the documented manual steps:remoteRef: key: tibuild-v2-api-client-sa-key
-
apps/gcp/tibuild/v2/external-secret-config.yaml (lines 37-57)
The product_repo_map extension is hardcoded and duplicates some repo mappings (e.g.,br,dumpling,tidb-lightningall mapped topingcap/tidb). This may cause confusion or maintenance overhead.Suggestion:
Consider adding comments or grouping related products to clarify the reason for multiple product names mapping to the same repo, or if possible refactor the mapping to avoid redundant entries. -
apps/prod2/tirelease/configmaps.yaml (lines 12-24 and 39-51)
The tibuild config block is duplicated almost identically for two different configmaps (tirelease-configandtirelease-webhook-config).Suggestion:
To avoid duplication and potential config drift, consider referencing a shared config or templating this block if the deployment system supports it.
Best Practices
-
Documentation / Comments
The manual GCP setup instructions are comprehensive and clear, which is excellent. However, adding a short note in the YAML files directly about the expected secret names and their source would improve maintainability for future operators. -
Naming Consistency
The key nametibuild_sa_keyis used in the configmaps and mount path, but the secret key in external-secret.yaml istibuild_sa_keywhile the remote secret is named differently (tirelease-tibuild-sa-keyvs.tibuild-v2-api-client-sa-key). Aligning these names will reduce confusion.
Summary of Actionable Changes
--- a/apps/prod2/tirelease/external-secret.yaml
+++ b/apps/prod2/tirelease/external-secret.yaml
@@ -20,7 +20,7 @@ spec:
- remoteRef:
- key: tirelease-tibuild-sa-key
+ remoteRef:
+ key: tibuild-v2-api-client-sa-key- Consider consolidating duplicated tibuild config sections in
apps/prod2/tirelease/configmaps.yaml. - Add inline comments in YAML files clarifying secret names and expected sources.
- Add explanation or grouping in
apps/gcp/tibuild/v2/external-secret-config.yamlabout multiple product keys mapping to the same repo.
Addressing these points will improve maintainability, reduce the chance of misconfiguration, and ease future updates.
Rename the tibuild-v2 API client SA to tirelease-tibuild-v2-api-client (<system>-tibuild-v2-api-client convention) so it is clear the SA was issued for the tirelease system; other systems (e.g. tcms) will get their own SA following the same convention.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR prepares deployment configurations for migrating to tibuild-v2 in the tirelease system by extending product mappings, adding tibuild-related config sections, and injecting a GCP Secret Manager secret for the tibuild service account key. The approach is straightforward and well-documented, with proper separation of concerns and no obvious logic errors. The quality is generally good, but there are some opportunities to improve maintainability, security, and clarity.
Critical Issues
- Potential Secret Exposure / Security Risk
- File:
apps/prod2/tirelease/external-secret.yaml(lines 15-23) - Issue: The entire GCP service account key JSON is stored and mounted as a secret (
tibuild_sa_key). If this key has broader IAM roles or permissions than intended, it could pose a security risk. There is no mention of key rotation or least privilege principle enforcement. - Suggestion:
- Verify that the SA only has the minimal required permissions (preferably only token minting as described).
- Add documentation or automation for key rotation.
- Consider using workload identity or token exchange if possible to avoid long-lived keys.
- File:
Code Improvements
-
Hardcoded Email in ConfigMap
- File:
apps/prod2/tirelease/configmaps.yaml(lines 18 and 51) - Issue: The field
createdBy: xiaozhou.zhu@pingcap.comis hardcoded. This may cause confusion or maintenance issues if ownership changes. - Suggestion:
- Replace with a more generic or configurable value, or document clearly why this is hardcoded and who owns the config.
- E.g.,
createdBy: tibuild-v2-team@pingcap.com
- Or leverage environment-specific annotations.
- File:
-
YAML Duplication in ConfigMaps
- File:
apps/prod2/tirelease/configmaps.yaml(lines 12-20 and 39-57) - Issue: The
tibuildconfig section is added twice intirelease-configandtirelease-webhook-configwith identical values, causing duplication and risk of divergence. - Suggestion:
- Extract and reuse common config with YAML anchors or a templating mechanism if supported by your deployment system.
- Alternatively, document the need for duplication explicitly.
- File:
-
Comment Clarity and Consistency
- File:
apps/prod2/tirelease/external-secret.yaml(lines 15-23) - Issue: Comments are helpful but could be clearer and consistently formatted to improve maintainability and onboarding.
- Suggestion:
- Use bullet points or clearer line breaks.
- Example:
# GCP Service Account key for tibuild-v2 API client: # - Used to mint Google ID tokens to authenticate to tibuild-v2 (https://prow.tidb.net/tibuild) # - Mounted at /etc/secret-config/tibuild_sa_key # - Referenced by config key: tibuild.saKey
- File:
Best Practices
-
Testing Coverage
- No indication of added tests or validation for these config changes.
- Suggestion:
- Include or link to automated tests or validation jobs that verify the new product mappings and secret mount behavior.
- At minimum, verify external-secret sync and configmap injection in staging.
-
Documentation for Manual Steps
- The manual GCP steps are well documented in the PR description but are not reflected inside the repository (e.g., README or runbooks).
- Suggestion:
- Add these manual GCP provisioning steps to the relevant infra docs (
infrastructure/prod2/external-secrets/README.md) or create a dedicateddocs/tibuild-v2-migration.mdto ensure they are discoverable and maintainable.
- Add these manual GCP provisioning steps to the relevant infra docs (
-
Naming Consistency
- File:
apps/gcp/tibuild/v2/external-secret-config.yaml(lines 37-52) - Product mapping keys like
ticdc-newarchandticiare introduced along with a comment about distinguishingpingcap-inc/ticdcfrompingcap/ticdclater. This is good but could be confusing now. - Suggestion:
- Add a TODO marker or issue number linking to the follow-up for clarity.
- Consider adding a brief inline comment explaining the difference or why the current mapping is temporary.
- File:
Overall, this PR is a solid step toward tibuild-v2 migration deployment. Addressing the security considerations around the SA key, reducing duplication, and improving documentation will enhance maintainability and security posture.
Rename the GCP Secret Manager secret id from tirelease-tibuild-sa-key to tirelease-tibuild-sa-key_json so the id constrains the payload format (SA key JSON), consistent with the existing tirelease_secret_json naming.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR prepares the deployment configuration for migrating to tibuild-v2 in the tirelease system. It expands the product repo map to cover all relevant products for hotfix builds, adds tibuild-specific config sections to tirelease configmaps, and injects the new tibuild service account key secret from GCP Secret Manager. The approach is straightforward and configuration-focused, with well-documented manual GCP steps. Overall, the changes are clear and appropriately scoped for this deployment preparation.
Critical Issues
- Potential Secret Key Mismatch in
external-secret.yaml- File:
apps/prod2/tirelease/external-secret.yaml(lines 15-26) - Issue: The new secret mapping is:
However, the manual instructions specify the secret id as
- secretKey: tibuild_sa_key remoteRef: key: tirelease-tibuild-sa-key_json
tirelease-tibuild-sa-key(no_jsonsuffix). This mismatch will cause the external secret controller to fail fetching the secret. - Suggested fix: Change
remoteRef.keyto exactlytirelease-tibuild-sa-keyto match the secret name in GCP Secret Manager and the manual steps:- secretKey: tibuild_sa_key remoteRef: key: tirelease-tibuild-sa-key
- File:
Code Improvements
-
Repeated
tibuildblock inconfigmaps.yaml- File:
apps/prod2/tirelease/configmaps.yaml(lines ~12-18 and 39-45) - Issue: The
tibuildconfig section is duplicated verbatim in two configmaps (tirelease-configandtirelease-webhook-config). This risks config drift or inconsistency. - Suggested fix: Extract the shared config into a common ConfigMap or use a single source of truth if possible, or add a comment explaining why duplication is necessary. This reduces maintenance overhead and potential config drift.
- File:
-
Clarify comment for product mappings related to ticdc
- File:
apps/gcp/tibuild/v2/external-secret-config.yaml(lines 37-57) - Issue: The PR notes follow-ups about ticdc product mapping ambiguity but does not address it now. Adding a TODO or FIXME comment in the config near these mappings would help future maintainers.
- Suggested fix: Add a comment like:
# TODO: Confirm and distinguish pingcap-inc/ticdc vs pingcap/ticdc in product_repo_map
- File:
Best Practices
-
Add explicit comments for new tibuild config entries
- File:
apps/prod2/tirelease/configmaps.yaml - Issue: While the PR description explains the tibuild config keys, the configmap itself lacks inline comments explaining each key's purpose.
- Suggested fix: Add brief comments inline for clarity, e.g.:
tibuild: baseURL: https://prow.tidb.net/tibuild # tibuild-v2 API base URL audience: prow.tidb.net # Expected audience for ID tokens ociFileserverURL: https://prow.tidb.net/dl/oci-file # OCI fileserver base URL createdBy: xiaozhou.zhu@pingcap.com # Maintainer contact saKey: tibuild_sa_key # Secret key filename for SA JSON
- File:
-
Testing and Validation
- No evidence of testing or validation steps related to these config changes is mentioned. Consider adding smoke tests or dry-run validations for the external secrets and configmaps to catch config errors early.
-
Consistent naming for SA key secret
- The SA key secret is named
tirelease-tibuild-sa-keyin GCP and referenced astibuild_sa_keyin the mount path/config. This is fine but ensure documentation consistently uses these names to avoid confusion.
- The SA key secret is named
This PR is mostly configuration with minimal logic changes, so the main risks are in key mismatches and config duplication. Addressing the secret key name mismatch is critical to avoid runtime failures. Adding inline comments and clarifying duplication will improve maintainability.
|
/hold |
The SA was created as tirelease-tibuild-client (<system>-tibuild-client per-consumer naming, GCP account id is capped at 30 chars).
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR adds deployment configuration for the tibuild-v2 migration, primarily expanding product repository mappings for tibuild-v2, adding tibuild-related config sections to tirelease configmaps, and introducing a new external secret for a GCP service account key. The approach is straightforward and well-documented, with clear instructions for required manual GCP steps. Overall, the changes are low-risk, mostly configuration updates, and the code quality is good.
Critical Issues
- No critical bugs or security vulnerabilities detected.
The secret is referenced properly and the manual steps specify secure handling of the SA key.
Code Improvements
-
apps/gcp/tibuild/v2/external-secret-config.yaml(lines ~38-57)
The extendedproduct_repo_maphas multiple entries mapping products likebr,dumpling,tidb-lightningtopingcap/tidb. This is potentially confusing because those products are nottidbitself. Consider adding a comment explaining why these products map topingcap/tidb(e.g., shared repo or build reasons). This can reduce confusion for future maintainers. -
In
apps/prod2/tirelease/configmaps.yaml(lines ~12-18 and 39-45)
Thetibuildconfig section is duplicated verbatim in two places (tirelease-configandtirelease-webhook-config). If these configmaps have a common base or template, consider refactoring to avoid duplication and ease future updates. -
In
apps/prod2/tirelease/external-secret.yaml(lines ~15-27)
The new external secret data entry relies on the secret IDtirelease-tibuild-sa-key_json. Since this is a manual external dependency, consider adding a validation step in the CI or deployment pipeline to confirm the secret exists and is accessible, reducing runtime errors.
Best Practices
-
Documentation
- The PR description is excellent and thorough, including manual GCP steps and rationale.
- Add a brief comment in
external-secret.yamlnear thedataentry to explain why the suffix_jsonis required, for maintainers unfamiliar with Secret Manager payload constraints.
-
Testing
- There is no indication of automated tests or validation for these config changes. Since these are mostly config updates, consider adding integration tests or dry-run validations to confirm the external secret syncs correctly and the new config sections do not break runtime behavior.
-
Naming Conventions
- The
createdByfield in configmaps is set to a specific user email. Consider if this should be a more generic or automated identifier to avoid stale ownership info.
- The
Summary of suggested code changes:
# apps/gcp/tibuild/v2/external-secret-config.yaml
# Add comment to explain why some product_repo_map entries point to pingcap/tidb
# e.g.,
# br, dumpling, tidb-lightning share the pingcap/tidb repo for build consistency
# apps/prod2/tirelease/external-secret.yaml
# Add comment before 'data' entry to clarify the _json suffix requirement:
# The secret id must end with '_json' to enforce JSON payload format in GCP Secret ManagerOverall, this PR is clear, well-structured, and low-risk. Addressing the minor documentation and duplication points will improve maintainability and reduce potential confusion.
wuhuizuo
changed the title
wuhuizuo
changed the title
|
/lgtm |
[LGTM Timeline notifier]Timeline:
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: timzxz, wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
2 participants
What
Deployment config for the tibuild-v2 migration (companion to PingCAP-QE/tirelease#697 and PingCAP-QE/ee-apps#590).
Changes
apps/gcp/tibuild/v2/external-secret-config.yaml: extendproduct_repo_mapfrom 6 to 20 entries, covering all products tirelease builds in hotfix flows (br/dumpling/tidb-lightning/ticdc-newarch/tici/enterprise-plugin/ng-monitoring/tidb-binlog/drainer/pump/tidb-dashboard/tidb-operator/tidb-tools/tiproxy); otherwise v2createreturns 400github full repo not found. Existing entries (including the ticdc/tiflow mapping) are left untouched and will be handled separately.apps/prod2/tirelease/configmaps.yaml: add thetibuildconfig section (baseURL/audience/ociFileserverURL/createdBy/saKey) to bothtirelease-configandtirelease-webhook-config.apps/prod2/tirelease/external-secret.yaml: add adataentry that fetches the GCP Secret Manager secrettirelease-tibuild-sa-key_json(a tibuild-v2 SA key issued for tirelease; the_jsonsuffix constrains the payload format — the whole payload is the SA key JSON) and writes it into the target secret astibuild_sa_key, mounted at/etc/secret-config/tibuild_sa_key(the main/jobs/webhook deployments all sharetirelease-secret-config, so no deployment manifest changes are needed).Create the SA (per-consumer naming: -tibuild-client (GCP account id capped at 30 chars)) and the Secret Manager entry (project
pingcap-testing-account, matching theee-gcp-smClusterSecretStore):Follow-ups (not in this PR)