Skip to content

Commit

Permalink
Add lint workflow (#23)
Browse files Browse the repository at this point in the history 
* Create lint.yml - lint with golangci-lint and staticcheck
* Fix: correct all lint issues and tidy
  • Loading branch information
@samirtahir91
samirtahir91 authored Apr 1, 2024
1 parent e6cc0ca commit ebcce98
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 62 deletions.
29 changes: 29 additions & 0 deletions .github/workflows/lint.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: Lint

# Trigger the workflow on pull requests and direct pushes to any branch
on:
push:
pull_request:

jobs:
lint:
name: golangci-lint and staticcheck
runs-on: ubuntu-latest
# Pull requests from the same repository won't trigger this checks as they were already triggered by the push
if: (github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository)
steps:
- name: Setup Go
uses: actions/setup-go@v5
with:
go-version: '~1.22'
- name: Clone the code
uses: actions/checkout@v4
- name: Run linter
uses: golangci/golangci-lint-action@v4
with:
version: v1.54
args: --timeout=2m --fix
- name: Run staticcheck
uses: dominikh/staticcheck-action@v1
with:
version: "latest"
120 changes: 64 additions & 56 deletions internal/controller/githubapp_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func (r *GithubAppReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}
return ctrl.Result{}, nil
}
l.Error(err, "Failed to get GithubApp")
l.Error(err, "failed to get GithubApp")
return ctrl.Result{}, err
}

Expand All @@ -107,32 +107,24 @@ func (r *GithubAppReconciler) Reconcile(ctx context.Context, req ctrl.Request) (

// Call the function to check if access token required
// Will either create the access token secret or update it
if err := r.checkExpiryAndUpdateAccessToken(ctx, githubApp, req); err != nil {
l.Error(err, "Failed to check expiry and update access token")
if err := r.checkExpiryAndUpdateAccessToken(ctx, githubApp); err != nil {
l.Error(err, "failed to check expiry and update access token")
// Update status field 'Error' with the error message
if updateErr := r.updateStatusWithError(ctx, githubApp, err.Error()); updateErr != nil {
l.Error(updateErr, "Failed to update status field 'Error'")
l.Error(updateErr, "failed to update status field 'Error'")
}
return ctrl.Result{}, err
}

// Call the function to check expiry and renew the access token if required
// Always requeue the githubApp for reconcile as per `reconcileInterval`
requeueResult, err := r.checkExpiryAndRequeue(ctx, githubApp, req)
if err != nil {
l.Error(err, "Failed to check expiry and requeue")
// Update status field 'Error' with the error message
if updateErr := r.updateStatusWithError(ctx, githubApp, err.Error()); updateErr != nil {
l.Error(updateErr, "Failed to update status field 'Error'")
}
return ctrl.Result{}, err
}
requeueResult := r.checkExpiryAndRequeue(ctx, githubApp)

// Clear the error field
// Clear the error field and clear it
if githubApp.Status.Error != "" {
githubApp.Status.Error = ""
if err := r.Status().Update(ctx, githubApp); err != nil {
l.Error(err, "Failed to clear status field 'Error' for GithubApp")
l.Error(err, "failed to clear status field 'Error' for GithubApp")
return ctrl.Result{}, err
}
}
Expand Down Expand Up @@ -177,7 +169,7 @@ func (r *GithubAppReconciler) updateStatusWithError(ctx context.Context, githubA
}

// Function to check expiry and update access token
func (r *GithubAppReconciler) checkExpiryAndUpdateAccessToken(ctx context.Context, githubApp *githubappv1.GithubApp, req ctrl.Request) error {
func (r *GithubAppReconciler) checkExpiryAndUpdateAccessToken(ctx context.Context, githubApp *githubappv1.GithubApp) error {

l := log.FromContext(ctx)

Expand All @@ -186,7 +178,7 @@ func (r *GithubAppReconciler) checkExpiryAndUpdateAccessToken(ctx context.Contex

// If expiresAt status field is not present or expiry time has already passed, generate or renew access token
if expiresAt.IsZero() || expiresAt.Before(time.Now()) {
return r.generateOrUpdateAccessToken(ctx, githubApp, req)
return r.generateOrUpdateAccessToken(ctx, githubApp)
}

// Check if the access token secret exists if not reconcile immediately
Expand All @@ -198,7 +190,7 @@ func (r *GithubAppReconciler) checkExpiryAndUpdateAccessToken(ctx context.Contex
if err := r.Get(ctx, accessTokenSecretKey, accessTokenSecret); err != nil {
if apierrors.IsNotFound(err) {
// Secret doesn't exist, reconcile straight away
return r.generateOrUpdateAccessToken(ctx, githubApp, req)
return r.generateOrUpdateAccessToken(ctx, githubApp)
}
// Error other than NotFound, return error
return err
Expand All @@ -207,7 +199,7 @@ func (r *GithubAppReconciler) checkExpiryAndUpdateAccessToken(ctx context.Contex
for key := range accessTokenSecret.Data {
if key != "token" && key != "username" {
l.Info("Removing invalid key in access token secret", "Key", key)
return r.generateOrUpdateAccessToken(ctx, githubApp, req)
return r.generateOrUpdateAccessToken(ctx, githubApp)
}
}

Expand All @@ -216,28 +208,28 @@ func (r *GithubAppReconciler) checkExpiryAndUpdateAccessToken(ctx context.Contex
username := string(accessTokenSecret.Data["username"])

// Check if the access token is a valid github token via gh api auth
if !isAccessTokenValid(ctx, username, accessToken, req) {
if !isAccessTokenValid(ctx, username, accessToken) {
// If accessToken is invalid, generate or update access token
return r.generateOrUpdateAccessToken(ctx, githubApp, req)
return r.generateOrUpdateAccessToken(ctx, githubApp)
}

// Access token exists, calculate the duration until expiry
durationUntilExpiry := expiresAt.Sub(time.Now())
durationUntilExpiry := time.Until(expiresAt)

// If the expiry threshold met, generate or renew access token
if durationUntilExpiry <= timeBeforeExpiry {
l.Info(
"Expiry threshold reached - renewing",
)
err := r.generateOrUpdateAccessToken(ctx, githubApp, req)
err := r.generateOrUpdateAccessToken(ctx, githubApp)
return err
}

return nil
}

// Function to check if the access token is valid by making a request to GitHub API
func isAccessTokenValid(ctx context.Context, username string, accessToken string, req ctrl.Request) bool {
func isAccessTokenValid(ctx context.Context, username string, accessToken string) bool {
l := log.FromContext(ctx)

// If username has been modified, renew the secret
Expand All @@ -257,7 +249,7 @@ func isAccessTokenValid(ctx context.Context, username string, accessToken string
// Create a new request
ghReq, err := http.NewRequest("GET", url, nil)
if err != nil {
l.Error(err, "Error creating request to GitHub API for rate limit")
l.Error(err, "error creating request to GitHub API for rate limit")
return false
}

Expand All @@ -270,8 +262,6 @@ func isAccessTokenValid(ctx context.Context, username string, accessToken string
l.Error(err, "Error sending request to GitHub API for rate limit")
return false
}
// close connection
defer resp.Body.Close()

// Check if the response status code is 200 (OK)
if resp.StatusCode != http.StatusOK {
Expand Down Expand Up @@ -301,13 +291,21 @@ func isAccessTokenValid(ctx context.Context, username string, accessToken string
return false
}

// Close the response body to prevent resource leaks
defer func() {
if err := resp.Body.Close(); err != nil {
// Handle error if closing the response body fails
l.Error(err, "error closing response body:")
}
}()

// Rate limit is valid
l.Info("Rate limit is valid", "Remaining requests:", remaining)
return true
}

// Fucntion to check expiry and requeue
func (r *GithubAppReconciler) checkExpiryAndRequeue(ctx context.Context, githubApp *githubappv1.GithubApp, req ctrl.Request) (ctrl.Result, error) {
// Function to check expiry and requeue
func (r *GithubAppReconciler) checkExpiryAndRequeue(ctx context.Context, githubApp *githubappv1.GithubApp) (ctrl.Result) {
l := log.FromContext(ctx)

// Get the expiresAt status field
Expand All @@ -319,11 +317,11 @@ func (r *GithubAppReconciler) checkExpiryAndRequeue(ctx context.Context, githubA
// Return result with no error and request reconciliation after x minutes
l.Info("Expiry threshold:", "Time", timeBeforeExpiry)
l.Info("Requeue after:", "Time", reconcileInterval)
return ctrl.Result{RequeueAfter: reconcileInterval}, nil
return ctrl.Result{RequeueAfter: reconcileInterval}
}

// Function to generate or update access token
func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, githubApp *githubappv1.GithubApp, req ctrl.Request) error {
func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, githubApp *githubappv1.GithubApp) error {
l := log.FromContext(ctx)

// Get the private key from the Secret
Expand All @@ -332,7 +330,7 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g
secret := &corev1.Secret{}
err := r.Get(ctx, client.ObjectKey{Namespace: secretNamespace, Name: secretName}, secret)
if err != nil {
l.Error(err, "Failed to get Secret")
l.Error(err, "failed to get Secret")
return err
}

Expand All @@ -344,12 +342,13 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g

// Generate or renew access token
accessToken, expiresAt, err := generateAccessToken(
ctx,
githubApp.Spec.AppId,
githubApp.Spec.InstallId,
privateKey,
)
if err != nil {
return fmt.Errorf("Failed to generate access token: %v", err)
return fmt.Errorf("failed to generate access token: %v", err)

}

Expand All @@ -372,7 +371,7 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g

// Set owner reference to GithubApp object
if err := controllerutil.SetControllerReference(githubApp, newSecret, r.Scheme); err != nil {
l.Error(err, "Failed to set owner reference for access token secret")
l.Error(err, "failed to set owner reference for access token secret")
return err
}

Expand All @@ -382,7 +381,7 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g
if apierrors.IsNotFound(err) {
// Secret doesn't exist, create a new one
if err := r.Create(ctx, newSecret); err != nil {
l.Error(err, "Failed to create Secret for access token")
l.Error(err, "failed to create Secret for access token")
return err
}
l.Info(
Expand All @@ -391,27 +390,27 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g
)
// Update the status with the new expiresAt time
if err := updateGithubAppStatusWithRetry(ctx, r, githubApp, expiresAt, 10); err != nil {
return fmt.Errorf("Failed after creating secret: %v", err)
return fmt.Errorf("failed after creating secret: %v", err)
}
// Restart the pods is required
if err := r.restartPods(ctx, githubApp, req); err != nil {
return fmt.Errorf("Failed to restart pods after after creating secret: %v", err)
if err := r.restartPods(ctx, githubApp); err != nil {
return fmt.Errorf("failed to restart pods after after creating secret: %v", err)
}
return nil
}
l.Error(
err,
"Failed to get access token secret",
"failed to get access token secret",
"Namespace", githubApp.Namespace,
"Secret", accessTokenSecret,
)
return fmt.Errorf("Failed to get access token secret: %v", err)
return fmt.Errorf("failed to get access token secret: %v", err)
}

// Secret exists, update its data
// Set owner reference to GithubApp object
if err := controllerutil.SetControllerReference(githubApp, existingSecret, r.Scheme); err != nil {
l.Error(err, "Failed to set owner reference for access token secret")
l.Error(err, "failed to set owner reference for access token secret")
return err
}
// Clear existing data and set new access token data
Expand All @@ -423,17 +422,17 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g
"username": gitUsername,
}
if err := r.Update(ctx, existingSecret); err != nil {
l.Error(err, "Failed to update existing Secret")
l.Error(err, "failed to update existing Secret")
return err
}

// Update the status with the new expiresAt time
if err := updateGithubAppStatusWithRetry(ctx, r, githubApp, expiresAt, 10); err != nil {
return fmt.Errorf("Failed after updating secret: %v", err)
return fmt.Errorf("failed after updating secret: %v", err)
}
// Restart the pods is required
if err := r.restartPods(ctx, githubApp, req); err != nil {
return fmt.Errorf("Failed to restart pods after updating secret: %v", err)
if err := r.restartPods(ctx, githubApp); err != nil {
return fmt.Errorf("failed to restart pods after updating secret: %v", err)
}

l.Info("Access token updated in the existing Secret successfully")
Expand All @@ -453,19 +452,22 @@ func updateGithubAppStatusWithRetry(ctx context.Context, r *GithubAppReconciler,
if apierrors.IsConflict(err) {
// Conflict error, retry the update
if attempts >= maxAttempts {
return fmt.Errorf("Maximum retry attempts reached, failed to update GitHubApp status")
return fmt.Errorf("maximum retry attempts reached, failed to update GitHubApp status")
}
// Incremental sleep between attempts
time.Sleep(time.Duration(attempts*2) * time.Second)
continue
}
// Other error, return with the error
return fmt.Errorf("Failed to update GitHubApp status: %v", err)
return fmt.Errorf("failed to update GitHubApp status: %v", err)
}
}

// function to generate new access token for gh app
func generateAccessToken(appID int, installationID int, privateKey []byte) (string, metav1.Time, error) {
func generateAccessToken(ctx context.Context, appID int, installationID int, privateKey []byte) (string, metav1.Time, error) {

l := log.FromContext(ctx)

// Parse private key
parsedKey, err := jwt.ParseRSAPrivateKeyFromPEM(privateKey)
if err != nil {
Expand All @@ -474,10 +476,10 @@ func generateAccessToken(appID int, installationID int, privateKey []byte) (stri

// Generate JWT
now := time.Now()
claims := jwt.StandardClaims{
claims := jwt.RegisteredClaims{
Issuer: fmt.Sprintf("%d", appID),
IssuedAt: now.Unix(),
ExpiresAt: now.Add(10 * time.Minute).Unix(), // Expiry time is 10 minutes from now
IssuedAt: jwt.NewNumericDate(now),
ExpiresAt: jwt.NewNumericDate(now.Add(10 * time.Minute)), // Expiry time is 10 minutes from now
}
token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims)
signedToken, err := token.SignedString(parsedKey)
Expand All @@ -500,8 +502,6 @@ func generateAccessToken(appID int, installationID int, privateKey []byte) (stri
if err != nil {
return "", metav1.Time{}, fmt.Errorf("failed to perform HTTP request: %v", err)
}
// Close connection
defer resp.Body.Close()

// Check error in response
if resp.StatusCode != http.StatusCreated {
Expand All @@ -522,11 +522,19 @@ func generateAccessToken(appID int, installationID int, privateKey []byte) (stri
return "", metav1.Time{}, fmt.Errorf("failed to parse expire time: %v", err)
}

// Close the response body to prevent resource leaks
defer func() {
if err := resp.Body.Close(); err != nil {
// Handle error if closing the response body fails
l.Error(err, "error closing response body:")
}
}()

return accessToken, metav1.NewTime(expiresAt), nil
}

// Function to bounce pods as per `spec.restartPods.labels` in GithubApp (in the same namespace)
func (r *GithubAppReconciler) restartPods(ctx context.Context, githubApp *githubappv1.GithubApp, req ctrl.Request) error {
func (r *GithubAppReconciler) restartPods(ctx context.Context, githubApp *githubappv1.GithubApp) error {
l := log.FromContext(ctx)

// Check if restartPods field is defined
Expand Down Expand Up @@ -610,7 +618,7 @@ func (r *GithubAppReconciler) SetupWithManager(mgr ctrl.Manager) error {
reconcileInterval, err = time.ParseDuration(reconcileIntervalStr)
if err != nil {
// Handle case where environment variable is not set or invalid
log.Log.Error(err, "Failed to set reconcileInterval, defaulting")
log.Log.Error(err, "failed to set reconcileInterval, defaulting")
reconcileInterval = defaultRequeueAfter
}

Expand All @@ -619,7 +627,7 @@ func (r *GithubAppReconciler) SetupWithManager(mgr ctrl.Manager) error {
timeBeforeExpiry, err = time.ParseDuration(timeBeforeExpiryStr)
if err != nil {
// Handle case where environment variable is not set or invalid
log.Log.Error(err, "Failed to set timeBeforeExpiry, defaulting")
log.Log.Error(err, "failed to set timeBeforeExpiry, defaulting")
timeBeforeExpiry = defaultTimeBeforeExpiry
}

Expand Down
Loading

0 comments on commit ebcce98

Please sign in to comment.