Skip to content

Commit

Permalink
feat: Incremental improvements on server
Browse files Browse the repository at this point in the history 
* When queried fields are invalid, return error

* Allow listing all users using admin endpoint

* Add e2e tests validating new features

Signed-off-by: Mahendra Paipuri <[email protected]>
  • Loading branch information
@mahendrapaipuri
mahendrapaipuri committed Jul 11, 2024
1 parent e4447b9 commit 78d4e23
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 11 deletions.
7 changes: 4 additions & 3 deletions pkg/api/http/error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ const (

// Custom errors
var (
errNoUser = errors.New("no user identified")
errNoPrivs = errors.New("current user does not have admin privileges")
errInvalidRequest = errors.New("invalid request")
errNoUser = errors.New("no user identified")
errNoPrivs = errors.New("current user does not have admin privileges")
errInvalidRequest = errors.New("invalid request")
errInvalidQueryField = errors.New("invalid query fields")
)

// Return error response for by setting errorString and errorType in response
Expand Down
6 changes: 0 additions & 6 deletions pkg/api/http/querier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,6 @@ func countRows(dbConn *sql.DB, query Query) (int, error) {
}
defer countStmt.Close()

queryStmt, err := dbConn.Prepare(queryString)
if err != nil {
return 0, err
}
defer queryStmt.Close()

// queryParams has to be an inteface. Do casting here
qParams := make([]interface{}, len(queryParams))
for i, v := range queryParams {
Expand Down
28 changes: 26 additions & 2 deletions pkg/api/http/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,11 @@ func (s *CEEMSServer) unitsQuerier(

// Get fields query parameters if any
queriedFields := s.getQueriedFields(r.URL.Query(), base.UnitsDBTableColNames)
if len(queriedFields) == 0 {
level.Error(s.logger).Log("msg", "Invalid query fields", "loggedUser", loggedUser, "err", errInvalidQueryField)
errorResponse[any](w, &apiError{errorBadData, errInvalidQueryField}, s.logger, nil)
return
}

// Initialise query builder
q := Query{}
Expand Down Expand Up @@ -748,8 +753,14 @@ func (s *CEEMSServer) usersQuerier(users []string, w http.ResponseWriter, r *htt
// Make query
q := Query{}
q.query(fmt.Sprintf("SELECT * FROM %s", base.UsersDBTableName))
q.query(" WHERE name IN ")
q.param(users)
// If no user is queried, return all users. This can happen only for admin
// end points
if len(users) == 0 {
q.query(" WHERE name LIKE '%' ")
} else {
q.query(" WHERE name IN ")
q.param(users)
}

// Get cluster_id query parameters if any
if clusterIDs := r.URL.Query()["cluster_id"]; len(clusterIDs) > 0 {
Expand Down Expand Up @@ -1147,6 +1158,11 @@ func (s *CEEMSServer) usage(w http.ResponseWriter, r *http.Request) {

// Get fields query parameters if any
queriedFields := s.getQueriedFields(r.URL.Query(), base.UsageDBTableColNames)
if len(queriedFields) == 0 {
level.Error(s.logger).Log("msg", "Invalid query fields", "loggedUser", dashboardUser)
errorResponse[any](w, &apiError{errorBadData, errInvalidQueryField}, s.logger, nil)
return
}

// handle current usage query
if mode == "current" {
Expand Down Expand Up @@ -1211,6 +1227,9 @@ func (s *CEEMSServer) usageAdmin(w http.ResponseWriter, r *http.Request) {
// Set headers
s.setHeaders(w)

// Get current user from header
_, dashboardUser := s.getUser(r)

// Get path parameter type
var mode string
var exists bool
Expand All @@ -1221,6 +1240,11 @@ func (s *CEEMSServer) usageAdmin(w http.ResponseWriter, r *http.Request) {

// Get fields query parameters if any
queriedFields := s.getQueriedFields(r.URL.Query(), base.UsageDBTableColNames)
if len(queriedFields) == 0 {
level.Error(s.logger).Log("msg", "Invalid query fields", "loggedUser", dashboardUser)
errorResponse[any](w, &apiError{errorBadData, errInvalidQueryField}, s.logger, nil)
return
}

// handle current usage query
if mode == "current" {
Expand Down
1 change: 1 addition & 0 deletions pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"status":"error","data":null,"errorType":"bad_data","error":"invalid query fields"}
1 change: 1 addition & 0 deletions pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"status":"success","data":[{"cluster_id":"slurm-0","resource_manager":"slurm","name":"testusr","projects":["testacc"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr1","projects":["acc1","acc2"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr15","projects":["acc1"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr2","projects":["acc2"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr3","projects":["acc3"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr4","projects":["acc4"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr8","projects":["acc1"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"testusr","projects":["testacc"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr1","projects":["acc1","acc2"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr15","projects":["acc1"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr2","projects":["acc2"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr3","projects":["acc3"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr4","projects":["acc4"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr8","projects":["acc1"]}]}
14 changes: 14 additions & 0 deletions scripts/e2e-test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ then
then
desc="/users/admin end point test"
fixture='pkg/api/testdata/output/e2e-test-api-server-user-admin-query.txt'
elif [ "${scenario}" = "api-user-admin-all-query" ]
then
desc="/users/admin end point test that queries all users"
fixture='pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt'
elif [ "${scenario}" = "api-cluster-admin-query" ]
then
desc="/clusters/admin end point test"
Expand All @@ -112,6 +116,10 @@ then
then
desc="/units end point test with uuid query param"
fixture='pkg/api/testdata/output/e2e-test-api-server-uuid-query.txt'
elif [ "${scenario}" = "api-units-invalid-query" ]
then
desc="/units end point test with invalid field query"
fixture='pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt'
elif [ "${scenario}" = "api-running-query" ]
then
desc="/units end point test with running query param"
Expand Down Expand Up @@ -425,12 +433,18 @@ then
elif [ "${scenario}" = "api-user-admin-query" ]
then
get -H "X-Grafana-User: grafana" "127.0.0.1:${port}/api/${api_version}/users/admin?user=usr1" > "${fixture_output}"
elif [ "${scenario}" = "api-user-admin-all-query" ]
then
get -H "X-Grafana-User: grafana" "127.0.0.1:${port}/api/${api_version}/users/admin" > "${fixture_output}"
elif [ "${scenario}" = "api-cluster-admin-query" ]
then
get -H "X-Ceems-User: usr1" "127.0.0.1:${port}/api/${api_version}/clusters/admin" > "${fixture_output}"
elif [ "${scenario}" = "api-uuid-query" ]
then
get -H "X-Grafana-User: usr2" "127.0.0.1:${port}/api/${api_version}/units?uuid=1481508&project=acc2&cluster_id=slurm-0" > "${fixture_output}"
elif [ "${scenario}" = "api-units-invalid-query" ]
then
get -H "X-Grafana-User: usr2" "127.0.0.1:${port}/api/${api_version}/units?cluster_id=slurm-0&from=1676934000&to=1677538800&field=uuiid" > "${fixture_output}"
elif [ "${scenario}" = "api-running-query" ]
then
get -H "X-Grafana-User: usr3" "127.0.0.1:${port}/api/${api_version}/units?running&cluster_id=slurm-1&from=1676934000&to=1677538800&field=uuid&field=state&field=started_at&field=allocation&field=tags" > "${fixture_output}"
Expand Down

0 comments on commit 78d4e23

Please sign in to comment.