fix: set SSL_CERT_FILE for git-sync for custom CA during GitHub App Auth#2141
fix: set SSL_CERT_FILE for git-sync for custom CA during GitHub App Auth#2141samirtahir91 wants to merge 3 commits intoGoogleContainerTools:mainfrom
Conversation
|
Hi @samirtahir91. Thanks for your PR. I'm waiting for a GoogleContainerTools member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the SSL_CERT_FILE environment variable to the git-sync container when a CA certificate is provided, ensuring that GitHub App token exchanges using the Go net/http client can validate certificates. The review feedback points out a typo in the added comment and suggests using a constant for the environment variable name to align with the project's coding patterns.
| // SSL_CERT_FILE is needed for Github App token exchange | ||
| // as it uses net/http client and not the git CLI. | ||
| result = append(result, corev1.EnvVar{ | ||
| Name: "SSL_CERT_FILE", | ||
| Value: caCertFilePath, | ||
| }) |
There was a problem hiding this comment.
The comment contains a typo ("Github" should be "GitHub") and could be more precise. Additionally, using a hardcoded string for the environment variable name "SSL_CERT_FILE" is inconsistent with the existing pattern in this file, where environment variable names are defined as constants (e.g., GitSSLCAInfo on line 54). While I cannot suggest adding a constant to the const block as it is outside the diff range, consider defining one for SSL_CERT_FILE to maintain consistency with the rest of the file.
| // SSL_CERT_FILE is needed for Github App token exchange | |
| // as it uses net/http client and not the git CLI. | |
| result = append(result, corev1.EnvVar{ | |
| Name: "SSL_CERT_FILE", | |
| Value: caCertFilePath, | |
| }) | |
| // SSL_CERT_FILE is needed for GitHub App token exchange, | |
| // as it uses the Go net/http client and not the git CLI. | |
| result = append(result, corev1.EnvVar{ | |
| Name: "SSL_CERT_FILE", | |
| Value: caCertFilePath, | |
| }) |
|
@tiffanny29631 - Are you happy to review please? |
tiffanny29631
left a comment
tiffanny29631
left a comment
There was a problem hiding this comment.
Nice fix! Since we have an existing test suite in gitsync_env_test.go, could we add a test case to TestGitSyncEnvVars to verify this change?
samirtahir91
force-pushed
the
fix/git-sync-ca-cert-github-app
branch
from
a97a9b0 to
9bfbace
Compare
Added test, can you review please |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tiffanny29631 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 |
2 participants
This PR updates the git-sync reconciler to have the
SSL_CERT_FILEenv variable set to custom CA certificate when the root/repo sync has as custom ca cert defined.Without is Github App Auth fails due to SSL errors.
Issue - #2140