Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 66 additions & 12 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -101,28 +101,82 @@ jobs:
env:
PR_BODY: ${{ github.event.pull_request.body }}
run: |
# Any change to these needs the marker: a new exit code, a new config
# field or a new schema property extends a contract even when nothing
# existing moves.
PROTECTED_FILES=(
"docs/reference/rc/"
"schemas/output_v1.json"
"crates/cli/src/exit_code.rs"
"crates/core/src/config.rs"
)

CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD)
# The command reference is a contract too, but adding a section to it
# is how a new command gets documented — required by AGENTS.md § 3 of
# the Breaking Change process, in fact. Demanding the marker for that
# makes every additive PR claim to be breaking, so these paths need it
# only when existing lines move: a rewritten sentence, a removed flag,
# a deleted or renamed page.
ADDITIVE_OK_FILES=(
"docs/reference/rc/"
)

BASE="origin/${{ github.base_ref }}"
CHANGED_FILES=$(git diff --name-only "$BASE"...HEAD)

# Paths under a protected prefix, one per line.
matched_paths() {
local protected_path="${1%/}"
printf '%s\n' "$CHANGED_FILES" \
| grep -E "^$(printf '%s' "$protected_path" | sed 's/[.[\*^$]/\\&/g')(/|$)" || true
}

require_marker() {
local subject="$1"
echo "::warning::Protected file modified: $subject"
echo "This change requires the Breaking Change process. See AGENTS.md."
if ! grep -q "BREAKING" <<< "$PR_BODY"; then
echo "::error::Protected file $subject modified without BREAKING marker in PR description"
return 1
fi
}

status=0

for file in "${PROTECTED_FILES[@]}"; do
protected_path="${file%/}"
if echo "$CHANGED_FILES" | grep -Fxq "$protected_path" || echo "$CHANGED_FILES" | grep -q "^$protected_path/"; then
echo "::warning::Protected file modified: $file"
echo "This change requires the Breaking Change process. See AGENTS.md."

# Check if PR body contains BREAKING marker
if ! grep -q "BREAKING" <<< "$PR_BODY"; then
echo "::error::Protected file $file modified without BREAKING marker in PR description"
exit 1
fi
if [ -n "$(matched_paths "$file")" ]; then
require_marker "$file" || status=1
fi
done

for file in "${ADDITIVE_OK_FILES[@]}"; do
paths=$(matched_paths "$file")
[ -n "$paths" ] || continue

# Rename detection off on purpose: moving a reference page changes
# where readers and links land, so it should count as a rewrite
# rather than as a no-op.
removed=0
count=0
while IFS= read -r path; do
[ -n "$path" ] || continue
count=$((count + 1))
deleted=$(
git diff --numstat --no-renames "$BASE"...HEAD -- "$path" \
| awk '{ total += $2 } END { print total + 0 }'
)
removed=$((removed + deleted))
done <<< "$paths"

if [ "$removed" -gt 0 ]; then
require_marker "$file ($removed line(s) changed or removed)" || status=1
else
echo "$file changed by addition only ($count file(s)); no marker required"
fi
done

if [ "$status" -ne 0 ]; then
exit 1
fi
echo "Protected files check passed"

msrv:
Expand Down
66 changes: 65 additions & 1 deletion crates/cli/tests/admin_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,9 @@ fn mfa_status_succeeds_against_a_server_that_answers() {
}

#[test]
fn mfa_enroll_reports_unsupported_when_the_server_has_no_such_route() {
fn mfa_enroll_reports_unsupported_when_at_rest_protection_is_missing() {
// 501, which the server sends when `RUSTFS_IAM_MASTER_KEY` is unset. Not to
// be confused with an absent route, which is a 404 and a different code.
let config_dir = tempfile::tempdir().expect("create config dir");
let (endpoint, receiver, handle) = start_admin_response_test_server(
"501 Not Implemented",
Expand Down Expand Up @@ -540,3 +542,65 @@ fn user_mfa_status_reports_auth_error_when_the_server_refuses() {
.expect("captured admin request");
handle.join().expect("admin test server finished");
}

// ---------------------------------------------------------------------------
// The exit-code table in docs/reference/rc/admin.md
// ---------------------------------------------------------------------------

/// The reference documents how each failure is classified, and that file is a
/// protected contract. These pin the two rows a reader is most likely to build
/// retry logic around, so the table cannot drift away from the code silently.
#[test]
fn a_lockout_is_reported_as_retryable_rather_than_a_refusal() {
// The server answers `SlowDown` once the attempt limit is reached. That maps
// to the network code, which normally means "retry" — the reference says why
// it means "retry later" here, and this holds it to that class.
let config_dir = tempfile::tempdir().expect("create config dir");
let (endpoint, receiver, handle) = start_admin_response_test_server(
"503 Service Unavailable",
"application/json",
r#"{"Code":"SlowDown","Message":"too many attempts; try again in 900 seconds"}"#
.to_string(),
);

let output = rc()
.args([
"--json", "admin", "account", "mfa", "activate", "myalias", "--code", "123456",
])
.env("RC_CONFIG_DIR", config_dir.path())
.env("RC_HOST_myalias", rc_host_alias(&endpoint))
.output()
.expect("run rc command");

assert_eq!(output.status.code(), Some(3), "expected NetworkError");
receiver
.recv_timeout(Duration::from_secs(5))
.expect("captured admin request");
handle.join().expect("admin test server finished");
}

#[test]
fn an_absent_route_is_reported_as_not_found() {
// A server predating these endpoints. Distinct from the 501 an up-to-date
// server sends when at-rest protection is unconfigured, which is the row
// above it in the table.
let config_dir = tempfile::tempdir().expect("create config dir");
let (endpoint, receiver, handle) = start_admin_response_test_server(
"404 Not Found",
"application/json",
r#"{"Code":"NoSuchKey","Message":"unknown route"}"#.to_string(),
);

let output = rc()
.args(["--json", "admin", "account", "mfa", "status", "myalias"])
.env("RC_CONFIG_DIR", config_dir.path())
.env("RC_HOST_myalias", rc_host_alias(&endpoint))
.output()
.expect("run rc command");

assert_eq!(output.status.code(), Some(5), "expected NotFound");
receiver
.recv_timeout(Duration::from_secs(5))
.expect("captured admin request");
handle.join().expect("admin test server finished");
}
25 changes: 25 additions & 0 deletions docs/reference/rc/admin.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,31 @@ asks for confirmation; `--yes` is required in `--json` mode or when stdin is not
a terminal. The account is left protected by its password alone until the user
enrols again.

### Exit codes

Every other workflow in this reference states how its failures are classified, so
these do too. The account and two-factor commands map the server's answer onto
the standard codes:

| Condition | Code |
|---|---|
| Wrong verification code, recovery code, or password | authentication (4) |
| Locked out after repeated wrong codes | network (3) |
| At-rest protection not configured on the server | unsupported (7) |
| Route absent, on a server predating these endpoints | not found (5) |
| Recovery-code output path already occupied | conflict (6) |
| Missing, conflicting, or unprompted flags | usage (2) |
| Already enabled, not enabled, or no pending enrollment | general (1) |

The lockout deserves a note, because network (3) usually means "retry". It is
retryable here too, but only after the delay the server reports — the code comes
from `SlowDown`, which is the S3 vocabulary's closest analogue to a rate limit.
A script that retries immediately will simply be refused again.

A failed `--output-file` write is the one case where a non-zero exit does not mean
nothing happened: the server has already issued the set, so the codes are printed
and the exit code reports only that the file was not written.

### Root identities

A root identity provisioned from `RUSTFS_ACCESS_KEY` cannot have its password or
Expand Down