Skip to content

Commit

Permalink
chore: improve error logs in server/cluster/cluser.go (#20711)
Browse files Browse the repository at this point in the history
  • Loading branch information
@SuminSSon
SuminSSon authored Nov 13, 2024
1 parent 993d79c commit 0f872f5
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 37 deletions.
69 changes: 35 additions & 34 deletions server/cluster/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cluster

import (
"context"
"fmt"
"net/url"
"time"

Expand Down Expand Up @@ -53,14 +54,14 @@ func CreateClusterRBACObject(project string, server string) string {
func (s *Server) List(ctx context.Context, q *cluster.ClusterQuery) (*appv1.ClusterList, error) {
clusterList, err := s.db.ListClusters(ctx)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to list clusters: %w", err)
}

filteredItems := clusterList.Items

// Filter clusters by id
if filteredItems, err = filterClustersById(filteredItems, q.Id); err != nil {
return nil, err
return nil, fmt.Errorf("error filtering clusters by id: %w", err)
}

// Filter clusters by name
Expand All @@ -80,7 +81,7 @@ func (s *Server) List(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Clus
return nil
})
if err != nil {
return nil, err
return nil, fmt.Errorf("error running async cluster responses: %w", err)
}

cl := *clusterList
Expand All @@ -102,7 +103,7 @@ func filterClustersById(clusters []appv1.Cluster, id *cluster.ClusterID) ([]appv
case "name_escaped":
nameUnescaped, err := url.QueryUnescape(id.Value)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to unescape cluster name: %w", err)
}
items = filterClustersByName(clusters, nameUnescaped)
default:
Expand Down Expand Up @@ -143,17 +144,17 @@ func filterClustersByServer(clusters []appv1.Cluster, server string) []appv1.Clu
// Create creates a cluster
func (s *Server) Create(ctx context.Context, q *cluster.ClusterCreateRequest) (*appv1.Cluster, error) {
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionCreate, CreateClusterRBACObject(q.Cluster.Project, q.Cluster.Server)); err != nil {
return nil, err
return nil, fmt.Errorf("permission denied while creating cluster: %w", err)
}
c := q.Cluster
clusterRESTConfig, err := c.RESTConfig()
if err != nil {
return nil, err
return nil, fmt.Errorf("error getting REST config: %w", err)
}

serverVersion, err := s.kubectl.GetServerVersion(clusterRESTConfig)
if err != nil {
return nil, err
return nil, fmt.Errorf("error getting server version: %w", err)
}

clust, err := s.db.CreateCluster(ctx, c)
Expand All @@ -173,7 +174,7 @@ func (s *Server) Create(ctx context.Context, q *cluster.ClusterCreateRequest) (*
return nil, status.Error(codes.InvalidArgument, argo.GenerateSpecIsDifferentErrorMessage("cluster", existing, c))
}
} else {
return nil, err
return nil, fmt.Errorf("error creating cluster: %w", err)
}
}

Expand All @@ -185,7 +186,7 @@ func (s *Server) Create(ctx context.Context, q *cluster.ClusterCreateRequest) (*
},
})
if err != nil {
return nil, err
return nil, fmt.Errorf("error setting cluster info in cache: %w", err)
}
return s.toAPIResponse(clust), err
}
Expand All @@ -194,7 +195,7 @@ func (s *Server) Create(ctx context.Context, q *cluster.ClusterCreateRequest) (*
func (s *Server) Get(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Cluster, error) {
c, err := s.getClusterAndVerifyAccess(ctx, q, rbacpolicy.ActionGet)
if err != nil {
return nil, err
return nil, fmt.Errorf("error verifying access to update cluster: %w", err)
}

return s.toAPIResponse(c), nil
Expand All @@ -211,7 +212,7 @@ func (s *Server) getClusterWith403IfNotExist(ctx context.Context, q *cluster.Clu
func (s *Server) getClusterAndVerifyAccess(ctx context.Context, q *cluster.ClusterQuery, action string) (*appv1.Cluster, error) {
c, err := s.getClusterWith403IfNotExist(ctx, q)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get cluster with permissions check: %w", err)
}

// verify that user can do the specified action inside project where cluster is located
Expand All @@ -232,7 +233,7 @@ func (s *Server) getCluster(ctx context.Context, q *cluster.ClusterQuery) (*appv
} else if q.Id.Type == "name_escaped" {
nameUnescaped, err := url.QueryUnescape(q.Id.Value)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to unescape cluster name: %w", err)
}
q.Name = nameUnescaped
} else {
Expand All @@ -243,7 +244,7 @@ func (s *Server) getCluster(ctx context.Context, q *cluster.ClusterQuery) (*appv
if q.Server != "" {
c, err := s.db.GetCluster(ctx, q.Server)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get cluster by server: %w", err)
}
return c, nil
}
Expand All @@ -253,7 +254,7 @@ func (s *Server) getCluster(ctx context.Context, q *cluster.ClusterQuery) (*appv
if q.Name != "" {
clusterList, err := s.db.ListClusters(ctx)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to list clusters: %w", err)
}
for _, c := range clusterList.Items {
if c.Name == q.Name {
Expand Down Expand Up @@ -300,7 +301,7 @@ func (s *Server) Update(ctx context.Context, q *cluster.ClusterUpdateRequest) (*
Id: q.Id,
}, rbacpolicy.ActionUpdate)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to verify access for updating cluster: %w", err)
}

if len(q.UpdatedFields) == 0 || sets.NewString(q.UpdatedFields...).Has("project") {
Expand All @@ -320,18 +321,18 @@ func (s *Server) Update(ctx context.Context, q *cluster.ClusterUpdateRequest) (*
}
clusterRESTConfig, err := q.Cluster.RESTConfig()
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get REST config for cluster: %w", err)
}

// Test the token we just created before persisting it
serverVersion, err := s.kubectl.GetServerVersion(clusterRESTConfig)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get server version: %w", err)
}

clust, err := s.db.UpdateCluster(ctx, q.Cluster)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to update cluster in database: %w", err)
}
err = s.cache.SetClusterInfo(clust.Server, &appv1.ClusterInfo{
ServerVersion: serverVersion,
Expand All @@ -341,7 +342,7 @@ func (s *Server) Update(ctx context.Context, q *cluster.ClusterUpdateRequest) (*
},
})
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to set cluster info in cache: %w", err)
}
return s.toAPIResponse(clust), nil
}
Expand All @@ -350,7 +351,7 @@ func (s *Server) Update(ctx context.Context, q *cluster.ClusterUpdateRequest) (*
func (s *Server) Delete(ctx context.Context, q *cluster.ClusterQuery) (*cluster.ClusterResponse, error) {
c, err := s.getClusterWith403IfNotExist(ctx, q)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get cluster with permissions check: %w", err)
}

if q.Name != "" {
Expand All @@ -361,12 +362,12 @@ func (s *Server) Delete(ctx context.Context, q *cluster.ClusterQuery) (*cluster.
}
for _, server := range servers {
if err := enforceAndDelete(s, ctx, server, c.Project); err != nil {
return nil, err
return nil, fmt.Errorf("failed to enforce and delete cluster server: %w", err)
}
}
} else {
if err := enforceAndDelete(s, ctx, q.Server, c.Project); err != nil {
return nil, err
return nil, fmt.Errorf("failed to enforce and delete cluster server: %w", err)
}
}

Expand All @@ -388,7 +389,7 @@ func enforceAndDelete(s *Server, ctx context.Context, server, project string) er
func (s *Server) RotateAuth(ctx context.Context, q *cluster.ClusterQuery) (*cluster.ClusterResponse, error) {
clust, err := s.getClusterWith403IfNotExist(ctx, q)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get cluster with permissions check: %w", err)
}

var servers []string
Expand Down Expand Up @@ -417,23 +418,23 @@ func (s *Server) RotateAuth(ctx context.Context, q *cluster.ClusterQuery) (*clus
logCtx.Info("Rotating auth")
restCfg, err := clust.RESTConfig()
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get REST config for cluster: %w", err)
}
if restCfg.BearerToken == "" {
return nil, status.Errorf(codes.InvalidArgument, "Cluster '%s' does not use bearer token authentication", server)
}

claims, err := clusterauth.ParseServiceAccountToken(restCfg.BearerToken)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to parse service account token: %w", err)
}
kubeclientset, err := kubernetes.NewForConfig(restCfg)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to create Kubernetes clientset: %w", err)
}
newSecret, err := clusterauth.GenerateNewClusterManagerSecret(kubeclientset, claims)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to generate new cluster manager secret: %w", err)
}
// we are using token auth, make sure we don't store client-cert information
clust.Config.KeyData = nil
Expand All @@ -442,16 +443,16 @@ func (s *Server) RotateAuth(ctx context.Context, q *cluster.ClusterQuery) (*clus

clusterRESTConfig, err := clust.RESTConfig()
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get REST config for cluster: %w", err)
}
// Test the token we just created before persisting it
serverVersion, err := s.kubectl.GetServerVersion(clusterRESTConfig)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get server version: %w", err)
}
_, err = s.db.UpdateCluster(ctx, clust)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to update cluster in database: %w", err)
}
err = s.cache.SetClusterInfo(clust.Server, &appv1.ClusterInfo{
ServerVersion: serverVersion,
Expand All @@ -461,11 +462,11 @@ func (s *Server) RotateAuth(ctx context.Context, q *cluster.ClusterQuery) (*clus
},
})
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to set cluster info in cache: %w", err)
}
err = clusterauth.RotateServiceAccountSecrets(kubeclientset, claims, newSecret)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to rotate service account secrets: %w", err)
}
logCtx.Infof("Rotated auth (old: %s, new: %s)", claims.SecretName, newSecret.Name)
}
Expand Down Expand Up @@ -498,13 +499,13 @@ func (s *Server) toAPIResponse(clust *appv1.Cluster) *appv1.Cluster {
func (s *Server) InvalidateCache(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Cluster, error) {
cls, err := s.getClusterAndVerifyAccess(ctx, q, rbacpolicy.ActionUpdate)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to verify access for cluster: %w", err)
}
now := v1.Now()
cls.RefreshRequestedAt = &now
cls, err = s.db.UpdateCluster(ctx, cls)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to update cluster in database: %w", err)
}
return s.toAPIResponse(cls), nil
}
6 changes: 3 additions & 3 deletions server/cluster/cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ func TestGetCluster_CannotSetCADataAndInsecureTrue(t *testing.T) {
Cluster: cluster,
})

assert.EqualError(t, err, `Unable to apply K8s REST config defaults: specifying a root certificates file with the insecure flag is not allowed`)
assert.EqualError(t, err, `error getting REST config: Unable to apply K8s REST config defaults: specifying a root certificates file with the insecure flag is not allowed`)
})

cluster.Config.TLSClientConfig.CAData = nil
Expand Down Expand Up @@ -485,7 +485,7 @@ func TestDeleteClusterByName(t *testing.T) {
Name: "foo",
})

assert.EqualError(t, err, `rpc error: code = PermissionDenied desc = permission denied`)
assert.EqualError(t, err, `failed to get cluster with permissions check: rpc error: code = PermissionDenied desc = permission denied`)
})

t.Run("Delete Succeeds When Deleting by Name", func(t *testing.T) {
Expand Down Expand Up @@ -562,7 +562,7 @@ func TestRotateAuth(t *testing.T) {
Name: "foo",
})

assert.EqualError(t, err, `rpc error: code = PermissionDenied desc = permission denied`)
assert.EqualError(t, err, `failed to get cluster with permissions check: rpc error: code = PermissionDenied desc = permission denied`)
})

// While the tests results for the next two tests result in an error, they do
Expand Down

0 comments on commit 0f872f5

Please sign in to comment.