Conversation
There was a problem hiding this comment.
Pull request overview
Updates the k6 RBAC load tests and shared utilities to align with updated helper function signatures and to improve user/project setup for RBAC scenarios.
Changes:
- Reorders
createNormanProject()parameters and updates call sites accordingly. - Refactors
create_RTBs.jsto use shared project/user utilities and to add GlobalRoleBinding creation for test users. - Enhances request parameter handling (headers/cookies) across Rancher-related k6 utils and adds a generic
getRandomElements()helper.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| k6/vai/filter_change_configmaps.js | Updates createNormanProject() invocation to match new signature. |
| k6/tests/load_projects_rbac.js | Updates retry helper + call sites for new createNormanProject() signature. |
| k6/rbac/rbac_utils.js | Adjusts user creation + listUsers return shape; updates deletion helpers to match. |
| k6/rbac/create_RTBs.js | Refactors RBAC test setup/execution to use updated utils and add GRBs for login. |
| k6/rancher/rancher_utils.js | Fixes login request params construction and ensures logout returns response. |
| k6/rancher/rancher_users_utils.js | Adds consistent Accept: application/json headers to several calls. |
| k6/projects/project_utils.js | Adjusts project cleanup to query Norman projects and updates createNormanProject() signature. |
| k6/generic/generic_utils.js | Adds getRandomElements() helper used by RBAC test setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…void false negatives on 'http_req_failed' metric
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
k6/rbac/create_RTBs.js:113
- Projects are created with clusterId hardcoded to "local" (line 113), but earlier in the setup function a cluster is randomly selected from available clusters (line 93). This seems inconsistent - either the randomly selected clusterId should be used here, or the random selection logic is unnecessary. The selected clusterId is stored in the returned data object (line 177) and used later in the test (e.g., line 395, 474), so using "local" here may cause inconsistencies if the selected cluster is not "local".
clusterId: "local",
k6/rbac/create_RTBs.js:409
- Both iterationIndex and userIndex are calculated as
__ITER % data.users.length, making them identical. The userIndex variable is redundant and can be removed. Consider using iterationIndex directly at line 409 or removing the duplicate calculation.
const iterationIndex = __ITER % data.users.length
// Use modulo to get user index
const userIndex = __ITER % data.users.length
let user = data.users[userIndex]
k6/rbac/create_RTBs.js:141
- The createGlobalRoleBinding function expects the fourth parameter 'roles' to be an array (default is ["user"]), but here it's being passed as the string "user". This will result in "globalRoleId": "user" instead of "globalRoleId": ["user"] in the request body. The call should either pass ["user"] or omit the fourth parameter to use the default value.
res = createGlobalRoleBinding(baseUrl, { cookies: cookies }, userId, "user")
k6/projects/project_utils.js:25
- The cleanupMatchingProjects function now filters Norman projects by checking r["name"].startsWith(namePrefix). However, the caller passes "rtbs-test" as the prefix, but at line 157 of create_RTBs.js, projects are filtered from management.cattle.io projects by checking p["spec"]["displayName"].startsWith(projectsPrefix) where projectsPrefix is also "rtbs-test". There's a potential inconsistency in how projects are identified for cleanup versus how they're identified after creation. Verify that Norman project "name" and management.cattle.io project "spec.displayName" are consistently synchronized by the Rancher API.
JSON.parse(res.body)["data"].filter(r => r["name"].startsWith(namePrefix)).forEach(r => {
k6/projects/project_utils.js:31
- The function returns an HTTP response object (either from GET or the last DELETE), but at line 221 of create_RTBs.js it's used in a boolean context alongside other cleanup functions that return booleans (e.g., deleteUsersByPrefix returns true/false). This inconsistency means the check
if (!projectsDeleted || ...)may not work as intended. If no projects match and the GET succeeds (status 200), projectsDeleted will be truthy even though no deletions occurred. Consider returning a boolean like the other delete functions do, where true indicates all deletions succeeded and false indicates a failure.
export function cleanupMatchingProjects(baseUrl, cookies, namePrefix) {
let res = http.get(`${baseUrl}/${normanProjectsPath}`, {
headers: {
accept: "application/json",
'content-type': "application/json",
},
cookies: cookies })
check(res, {
'GET /v3/projects returns status 200': (r) => r.status === 200,
})
JSON.parse(res.body)["data"].filter(r => r["name"].startsWith(namePrefix)).forEach(r => {
res = http.del(`${baseUrl}/${normanProjectsPath}/${r["id"]}`, { cookies: cookies })
check(res, {
'DELETE /v3/projects returns status 200': (r) => r.status === 200,
})
})
return res
k6/rbac/rbac_utils.js:335
- The default value for the prefix parameter is "Dartboard " (with a trailing space), but at line 217 of create_RTBs.js, the function is called with "Dartboard" (without a trailing space). This inconsistency could lead to unexpected behavior. Consider either updating the default value to match the common usage pattern, or ensuring all callers pass the correct prefix with the space.
export function deleteUsersByPrefix(baseUrl, cookies, prefix = "Dartboard ") {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rancher-max
left a comment
There was a problem hiding this comment.
At some point it would be nice to only log based on a DEBUG variable (or something similar). Basically, using a logging package instead of console.log for everything. That's a future refactor though and not necessary as part of this PR, I just noticed a lot of console.log statements and it triggered the thought.
| let projectsRes = listProjects(baseUrl, cookies) | ||
| // Wait for GlobalRoleBindings to propagate before continuing | ||
| console.log("Waiting for GlobalRoleBindings to propagate...") | ||
| sleep(5) |
There was a problem hiding this comment.
Ideally we'd avoid the sleeps, but I understand it is probably necessary in this case. Could lead to flakiness though depending on the underlying OS/network/etc.
There was a problem hiding this comment.
Yeah, I'm not sure what the API-equivalent way to wait for the bindings to be in place besides just polling attempts to login with a user? But then that kind of pollutes the metrics with a bunch of failed requests.
create_RTBs.jstest and its util functions.