Add sia go lib to support use default service identity from pod service account annotation #3126
Conversation
Summary of ChangesHello @MartinTrojans, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Athenz Service Identity Agent (SIA) for Google Cloud Platform (GCP) by integrating Kubernetes-native mechanisms for service identity attestation. It introduces the ability for applications deployed on Google Kubernetes Engine (GKE) to declare their default service identity through annotations on their Kubernetes service accounts. This provides a more flexible and Kubernetes-idiomatic way to manage service identities, complementing the existing metadata server-based approach. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 support for GKE to the Athenz SIA GCP attestation library, allowing pods to specify their default service identity via Kubernetes service account annotations. The changes are well-structured and include new functions for interacting with the Kubernetes API to fetch annotations. The tests cover the new functionality, although one test name is a bit misleading. My review includes suggestions for improving robustness, adhering to best practices for context handling, and ensuring proper error handling. Overall, this is a good addition to the library.
| defaultIdentity := "" | ||
| if isRunningInGKE() { | ||
| // Get the default identity from the service account annotation | ||
| defaultIdentity, _ = getServiceAccountAnnotation("athenz.io/default-service-identity") |
There was a problem hiding this comment.
The error returned by getServiceAccountAnnotation is ignored. If the function fails (e.g., due to missing RBAC permissions), the error is suppressed, and defaultIdentity will be an empty string. This can lead to silent failures and unexpected behavior. The error should be logged to provide visibility into potential issues.
var err error
defaultIdentity, err = getServiceAccountAnnotation("athenz.io/default-service-identity")
if err != nil {
log.Printf("unable to get default service identity from pod annotation: %v", err)
}| return "", fmt.Errorf("failed to get current service account name: %v", err) | ||
| } | ||
|
|
||
| serviceAccount, err := clientset.CoreV1().ServiceAccounts(namespace).Get(context.Background(), serviceAccountName, metav1.GetOptions{}) |
There was a problem hiding this comment.
The Kubernetes API call uses context.Background(). It's better to accept a context.Context as an argument in getServiceAccountAnnotation and pass it down to the API calls. This allows callers to manage timeouts and cancellations. This would likely require propagating the context up to the New function. A similar issue exists in getCurrentServiceAccountName on line 94.
|
|
||
| // getCurrentServiceAccountName gets the service account name of the current pod | ||
| func getCurrentServiceAccountName(clientset *kubernetes.Clientset, namespace string) (string, error) { | ||
| podName := os.Getenv("HOSTNAME") |
There was a problem hiding this comment.
Using os.Getenv("HOSTNAME") to get the pod name is not always reliable. If a pod's spec.hostname is set, the HOSTNAME environment variable will be set to that value, which is not the pod's name. A more robust approach is to use the Downward API to expose the pod name as an environment variable. For example, you can add this to your pod spec:
env:
- name: POD_NAME
valueFrom:
fieldRef:
fieldPath: metadata.nameThen in your code, you can use os.Getenv("POD_NAME"). This ensures you always get the correct pod name.
| // TestNewWithGKEFallback tests that when running in GKE but annotation fails, | ||
| // it falls back to metadata | ||
| func TestNewWithGKEFallback(t *testing.T) { |
There was a problem hiding this comment.
The test name TestNewWithGKEFallback and its comment suggest that it tests a fallback from GKE annotation to instance metadata. However, the implementation in attestation.go does not have this fallback logic; it's a strict if/else. Also, as noted in the test, it actually only tests the non-GKE path. This is confusing. Please consider renaming the test to reflect what it actually tests (e.g., TestNewWithMetadata) and clarify if a fallback from GKE annotation to metadata is desired. If so, the implementation in New() needs to be updated.
…ce account annotation Signed-off-by: dma <[email protected]>
MartinTrojans
force-pushed
the
addGkeSiaSupport
branch
from
3a71e48 to
db1e29e
Compare
2 participants
Description
Contribution Checklist:
Attach Screenshots (Optional)