Copilot commented on code in PR #13142:
URL: https://github.com/apache/trafficserver/pull/13142#discussion_r3201876371
##########
src/mgmt/rpc/handlers/config/Configuration.cc:
##########
@@ -156,7 +159,19 @@ set_config_records(std::string_view const & /* id
ATS_UNUSED */, YAML::Node cons
}
// now set the value.
- auto const &[dataType, checkType, pattern, updateType] = recordCtx;
+ auto const &[dataType, checkType, pattern, updateType, accessType] =
recordCtx;
+
+ // Honour the registrant's access tier. RECA_READ_ONLY records may only
+ // be changed by in-process code (config file load, environment override,
+ // etc.); RECA_NO_ACCESS records are not exposed to the management plane
+ // at all. The management RPC must refuse both, with distinct error
+ // codes so callers can branch on the code instead of parsing messages.
+ if (accessType == RECA_READ_ONLY || accessType == RECA_NO_ACCESS) {
+ auto const ec =
+ std::error_code{accessType == RECA_READ_ONLY ?
err::RecordError::RECORD_READ_ONLY : err::RecordError::RECORD_NO_ACCESS};
+ resp.errata().assign(std::error_code{errors::Codes::RECORD}).note("{}",
ec);
+ continue;
Review Comment:
The PR description mentions returning distinct error codes
(`RECORD_READ_ONLY` / `RECORD_NO_ACCESS`) so callers can branch on codes.
However, the JSONRPC encoder currently sets each entry in `error.data[*].code`
to `errata.code().value()` (see `include/mgmt/rpc/jsonrpc/json/YAMLCodec.h`),
and this handler always does `resp.errata().assign(errors::Codes::RECORD)`.
That means the nested code will remain 2000 for both cases, and clients still
have to parse messages. To actually surface distinct codes, assign the errata
code to the specific `err::RecordError` value for these failures (or update the
encoder to carry per-note codes).
##########
src/mgmt/rpc/handlers/config/Configuration.cc:
##########
@@ -132,19 +133,21 @@ set_config_records(std::string_view const & /* id
ATS_UNUSED */, YAML::Node cons
}
LookupContext recordCtx;
+ std::get<RecAccessT>(recordCtx) = RECA_NULL;
// Get record info first. TODO: we may just want to get the full record
and then send it back as a response.
const auto ret = RecLookupRecord(
info.name.c_str(),
[](const RecRecord *record, void *data) {
- auto &[dataType, checkType, pattern, updateType] =
*static_cast<LookupContext *>(data);
+ auto &[dataType, checkType, pattern, updateType, accessType] =
*static_cast<LookupContext *>(data);
if (REC_TYPE_IS_CONFIG(record->rec_type)) {
dataType = record->data_type;
checkType = record->config_meta.check_type;
if (record->config_meta.check_expr) {
pattern = record->config_meta.check_expr;
}
updateType = record->config_meta.update_type;
+ accessType = record->config_meta.access_type;
Review Comment:
`LookupContext recordCtx;` leaves tuple elements indeterminate. In the
`RecLookupRecord` callback, `pattern` is only assigned when `check_expr` is
non-null, so `pattern` can remain uninitialized and the later `if (pattern !=
nullptr && ...)` is undefined behavior (could spuriously run the validity check
or crash). Please value-initialize the tuple (e.g., `LookupContext
recordCtx{};`) and/or explicitly set `pattern = nullptr` (and ideally always
assign `pattern = record->config_meta.check_expr;`). Also consider explicitly
rejecting non-CONFIG records here (e.g., track an `is_config` flag in the
context and return `RECORD_NOT_CONFIG`) to avoid using uninitialized
`dataType/updateType` when a caller passes a STAT name to
`admin_config_set_records`.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_set_read_only.test.py:
##########
@@ -0,0 +1,105 @@
+'''
+Verify that the management RPC refuses to write records registered as
+RECA_READ_ONLY.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from jsonrpc import Request, Response
+
+Test.Summary = '''
+Verify that records registered with RECA_READ_ONLY cannot be modified through
+the management RPC backing "traffic_ctl config set"
+(admin_config_set_records).
+'''
+
+Test.ContinueOnFail = True
+
+# proxy.config.thread.max_heartbeat_mseconds is registered as RECA_READ_ONLY
+# in src/records/RecordsConfig.cc with a default of 60. RECA_READ_ONLY = 2
+# in the RecAccessT enum (see include/records/RecDefs.h).
+READ_ONLY_RECORD = "proxy.config.thread.max_heartbeat_mseconds"
+DEFAULT_VALUE = "60"
+ATTEMPTED_VALUE = "999"
+RECA_READ_ONLY = "2"
+RECT_CONFIG_BIT = "1" # bit value in the rec_types filter
+
+ts = Test.MakeATSProcess("ts")
+
+
+def lookup_request():
+ """Build a JSONRPC request that fetches the read-only record."""
+ return Request.admin_lookup_records([{"record_name": READ_ONLY_RECORD,
"rec_types": [RECT_CONFIG_BIT]}])
+
+
+def assert_record_at_default(resp: Response):
+ """Validate the looked-up record is RECA_READ_ONLY and at its default
value."""
+ if resp.is_error():
+ return (False, f"unexpected error: {resp.error_as_str()}")
+
+ records = resp.result.get('recordList', [])
+ if len(records) != 1:
+ return (False, f"expected exactly 1 record, got {len(records)}")
+
+ rec = records[0]['record']
+ if rec.get('record_name') != READ_ONLY_RECORD:
+ return (False, f"record_name {rec.get('record_name')!r} !=
{READ_ONLY_RECORD!r}")
+ if rec.get('current_value') != DEFAULT_VALUE:
+ return (False, f"current_value {rec.get('current_value')!r} !=
{DEFAULT_VALUE!r}")
+
+ access = rec.get('config_meta', {}).get('access_type')
+ if str(access) != RECA_READ_ONLY:
+ return (False, f"access_type {access!r} != {RECA_READ_ONLY!r} (record
is not RECA_READ_ONLY)")
+
+ return (True, "record is RECA_READ_ONLY and at the registered default")
+
+
+def assert_set_was_rejected(resp: Response):
+ """Validate the set attempt produced the not-writable error."""
+ if not resp.is_error():
+ return (False, f"set should have failed but returned a result:
{resp.result!r}")
+ err = resp.error_as_str()
+ if "Record is read-only" not in err:
+ return (False, f"unexpected error message: {err!r}")
+ return (True, "set was refused with RECORD_READ_ONLY")
Review Comment:
This test only asserts on a substring of the error text. Since this PR
introduces new machine-actionable error codes, the test should also validate
the code in the JSONRPC error payload (e.g., using
`resp.contains_nested_error(code=...)` against the record error code for
`RECORD_READ_ONLY`) to ensure the intended API contract doesn’t regress even if
wording changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
