You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/05/25 15:34:37 UTC
[GitHub] [trafficserver] brbzull0 opened a new pull request, #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
brbzull0 opened a new pull request, #8865:
URL: https://github.com/apache/trafficserver/pull/8865
So there are two changes here, I have them both in the same PR as the last one depends on the first one.
1 - We needed a way to let the handler to pass information to the registration which at some point later on could be validated or used in any way, so this is a base change for further checks(like client's socket credentials). This defines a new type in the `apidefs.h.in` because this will eventually be used from the Plugin API.
Please check this [commit](https://github.com/apache/trafficserver/commit/d6f888297ae46270e56c6a7798030630d7e2ed07) to see this changes only.
There is also a documentation update here in this [commit](https://github.com/apache/trafficserver/commit/69d79683efc73048b86c36326348ccb5061de274)
2 - The main driver for this change: adding peer's credentials check on rpc calls.
Context:
When moved from `traffic_manger` admin socket to the new `JSONRPC` api, the management api [restrictions](https://docs.trafficserver.apache.org/en/latest/admin-guide/files/records.config.en.html?highlight=restricted#proxy.config.admin.api.restricted) were left out, the only restriction in place was related to the creation of the jsonrpc unix socket(either `700` or `777`) just at user level losing the original capability to check the socket’s peers credentials and allow or deny certain operations, like read only calls(record lookup) vs operation that can change the state of ATS(like a record set). This was a useful method to allow some non-root users to perform some actions while we leave root users to have full access to the API. So this PR also includes the same feature but now on the JSONRPC node.
Please check this [commit](https://github.com/apache/trafficserver/commit/f60613c48f39b0dc43f5e27b915dd3bafa9b8bdc) to see this changes only.
---
This is an example of the output you will get when if request an unauthorized rpc method:
```
$ traffic_ctl config reload
Server Error found:
[10] Unauthorized action
- [2] Denied privileged API access for uid=1001 gid=1001
```
The raw json would look like:
```
{
"jsonrpc": "2.0",
"error":{
"code": 10,
"message": "Unauthorized action",
"data":[
{
"code": 2,
"message":"Denied privileged API access for uid=XXX gid=XXX"
}
]
},
"id":"5e273ec0-3e3b-4a81-90ec-aeee3d38073f"
}
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r890561743
##########
mgmt2/rpc/jsonrpc/JsonRPCManager.cc:
##########
@@ -54,9 +59,15 @@ std::condition_variable g_rpcHandlingCompletion;
ts::Rv<YAML::Node> g_rpcHandlerResponseData;
bool g_rpcHandlerProccessingCompleted{false};
-// jsonrpc log tag.
-static constexpr auto logTag = "rpc";
-static constexpr auto logTagMsg = "rpc.msg";
+// --- Helpers
+std::pair<ts::Errata, error::RPCErrorCode>
Review Comment:
Isn't pair kinda deprecated is favor of tuple?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r889399657
##########
mgmt2/config/FileManager.cc:
##########
@@ -91,12 +91,11 @@ FileManager::FileManager()
this->registerCallback(&handle_file_reload);
// Register the files registry jsonrpc endpoint
- rpc::add_method_handler(
- "filemanager.get_files_registry",
- [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
- return get_files_registry_rpc_endpoint(id, req);
- },
- &rpc::core_ats_rpc_service_provider_handle);
+ rpc::add_method_handler("filemanager.get_files_registry",
+ [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+ return get_files_registry_rpc_endpoint(id, req);
+ },
+ &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});
Review Comment:
Seems dangerous, capturing the **this** pointer, tricky to keep it from not becoming stale due to object destruction:
```
wkaras ~/REPOS/MISC/LAMBDA_THIS
$ cat x.cc
#include <iostream>
struct A;
A *ga;
struct A
{
int i = 5;
auto f() { return [this]() -> A & { return *this; }; }
auto g()
{
A a{*this};
return [a]() -> int { return a.i; };
}
};
int main()
{
ga = new A;
auto lambda_f = ga->f();
auto lambda_g = ga->g();
delete ga;
if (&lambda_f() == ga)
{
std::cout << "Dangling pointer!\n";
}
std::cout << lambda_g() << '\n';
return 0;
}
wkaras ~/REPOS/MISC/LAMBDA_THIS
$ gcc -Wall -Wextra -pedantic x.cc -lstdc++
wkaras ~/REPOS/MISC/LAMBDA_THIS
$ ./a.out
Dangling pointer!
5
wkaras ~/REPOS/MISC/LAMBDA_THIS
$
```
The g() member function shows a safe way to capture an object whose class the function is a member of.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891292632
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
This change does not include any changes on the plugin API. We have a separated PR for that, which still didn't make it into 10-Dev. I will update that PR after we agree on this.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898186868
##########
mgmt2/rpc/jsonrpc/JsonRPCManager.cc:
##########
@@ -54,9 +59,15 @@ std::condition_variable g_rpcHandlingCompletion;
ts::Rv<YAML::Node> g_rpcHandlerResponseData;
bool g_rpcHandlerProccessingCompleted{false};
-// jsonrpc log tag.
-static constexpr auto logTag = "rpc";
-static constexpr auto logTagMsg = "rpc.msg";
+// --- Helpers
+std::pair<ts::Errata, error::RPCErrorCode>
Review Comment:
Meh. The better question is, why `pair` instead of the `struct Error` defined earlier.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898175701
##########
mgmt2/rpc/jsonrpc/Context.cc:
##########
@@ -0,0 +1,35 @@
+/**
+ @section license License
+
+ 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.
+*/
+#include "Context.h"
+
+namespace rpc
+{
+// --- Call Context impl
+ts::Errata
+Context::Auth::any_blockers(TSRPCHandlerOptions const &options) const
+{
+ ts::Errata out;
+ // check every registered callback and see if they have something to say. Then report back to the manager
+ for (auto &&check : _checkers) {
Review Comment:
Shouldn't `check` return a `bool` rather than taking a `bool&`? Does it return something else that's being ignored?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891285808
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
OK but I don't see this used as the type of any parameter to any TS API function.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898108692
##########
mgmt2/rpc/jsonrpc/JsonRPC.h:
##########
@@ -33,17 +33,17 @@ extern RPCRegistryInfo core_ats_rpc_service_provider_handle;
/// @see JsonRPCManager::add_method_handler for details
template <typename Func>
inline bool
-add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info = nullptr)
+add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info, TSRPCHandlerOptions const &opt)
Review Comment:
@ywkaras -> https://github.com/apache/trafficserver/pull/8916
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898179084
##########
mgmt2/rpc/jsonrpc/Defs.h:
##########
@@ -45,11 +45,20 @@ class RPCHandlerResponse
};
struct RPCResponseInfo {
- RPCResponseInfo(std::optional<std::string> const &id_) : id(id_) {}
+ RPCResponseInfo(std::optional<std::string> const &id_) : id{id_} {}
Review Comment:
Why use `std::optional` with `std::string`? Isn't an empty string the same as not having one?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891036417
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
So can be directly used from both sides(core & plugins). isn't that the point of `apidefs.h`?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891060457
##########
mgmt2/rpc/jsonrpc/JsonRPC.h:
##########
@@ -33,17 +33,17 @@ extern RPCRegistryInfo core_ats_rpc_service_provider_handle;
/// @see JsonRPCManager::add_method_handler for details
template <typename Func>
inline bool
-add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info = nullptr)
+add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info, TSRPCHandlerOptions const &opt)
Review Comment:
Thanks Walt.
yes, I would probably have them constant defined and not use the name as literal in the call anyway, I wasn't very worry about this as it only happens at startup and at the end it will allocate to hold it internally.
I may add a new pr with some tweaks I have noted, takling this too possibly.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891316414
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
> So you're saying it's for future use?
short answer: yes.
long asnwer:
The future is here: https://github.com/apache/trafficserver/pull/8630, I will update 8630 after this one is done.
At yahoo we are running the rpc TS API in production since quite some time now, so all this changes will get into our yahoo ats as well very soon.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891047257
##########
mgmt2/config/FileManager.cc:
##########
@@ -91,12 +91,11 @@ FileManager::FileManager()
this->registerCallback(&handle_file_reload);
// Register the files registry jsonrpc endpoint
- rpc::add_method_handler(
- "filemanager.get_files_registry",
- [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
- return get_files_registry_rpc_endpoint(id, req);
- },
- &rpc::core_ats_rpc_service_provider_handle);
+ rpc::add_method_handler("filemanager.get_files_registry",
+ [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+ return get_files_registry_rpc_endpoint(id, req);
+ },
+ &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});
Review Comment:
`FileManager` is now a singleton class, by the time it's gone it shouldn't dangle.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#issuecomment-1149332266
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#issuecomment-1150431082
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 merged pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 merged PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r889369183
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
Why is this in apidefs.h ?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r889343535
##########
doc/admin-guide/files/jsonrpc.yaml.en.rst:
##########
@@ -68,9 +69,9 @@ Configuration
If a non-default configuration is needed, the following describes the configuration
structure.
-File `jsonrpc.yaml` is a YAML format. The default configuration is:
+File `jsonrpc.yaml` is a YAML format. The default configuration looks like:
Review Comment:
So it's like a default imposter?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r889399657
##########
mgmt2/config/FileManager.cc:
##########
@@ -91,12 +91,11 @@ FileManager::FileManager()
this->registerCallback(&handle_file_reload);
// Register the files registry jsonrpc endpoint
- rpc::add_method_handler(
- "filemanager.get_files_registry",
- [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
- return get_files_registry_rpc_endpoint(id, req);
- },
- &rpc::core_ats_rpc_service_provider_handle);
+ rpc::add_method_handler("filemanager.get_files_registry",
+ [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+ return get_files_registry_rpc_endpoint(id, req);
+ },
+ &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});
Review Comment:
Seems dangerous, capturing the **this** pointer, tricky to keep it from not becoming stale due to object destruction:
```
wkaras ~/REPOS/MISC/LAMBDA_THIS
$ cat x.cc
#include <iostream>
struct A;
A *ga;
struct A
{
int i = 5;
auto g() { return [this]() -> A & { return *this; }; }
auto g2()
{
A a{*this};
delete ga;
return [a]() -> int { return a.i; };
}
};
auto h()
{
return ga->g();
}
int main()
{
ga = new A;
A *pa = &(h()());
std::cout << ga->g2()() << '\n';
if (pa == ga)
{
std::cout << "SAME\n";
}
else
{
std::cout << "NOT SAME\n";
}
return 0;
}
wkaras ~/REPOS/MISC/LAMBDA_THIS
$ gcc x.cc -lstdc++
wkaras ~/REPOS/MISC/LAMBDA_THIS
$ ./a.out
5
SAME
wkaras ~/REPOS/MISC/LAMBDA_THIS
$
```
The g2() function shows safe way to capture an object whose class the function is a member of.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r890607497
##########
mgmt2/rpc/jsonrpc/JsonRPC.h:
##########
@@ -33,17 +33,17 @@ extern RPCRegistryInfo core_ats_rpc_service_provider_handle;
/// @see JsonRPCManager::add_method_handler for details
template <typename Func>
inline bool
-add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info = nullptr)
+add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info, TSRPCHandlerOptions const &opt)
Review Comment:
Since this seems to get called with a string literal, you could avoid a heap allocation if the type was std::string_view.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891292632
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
Although it sets the base, this change does not include any changes on the plugin API. We have a separated PR for that, which still didn't make it into 10-Dev. I will update that PR after we agree on this.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891302805
##########
include/ts/apidefs.h.in:
##########
@@ -1472,6 +1472,18 @@ namespace ts
}
#endif
+///
+/// @brief JSON-RPC Handler options
+///
+/// This class holds information about how a handler will be managed and delivered when called. The JSON-RPC manager would use this
+/// object to perform certain validation.
+///
+typedef struct TSRPCHandlerOptions_s {
+ struct Auth {
+ int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+ } auth;
+} TSRPCHandlerOptions;
+
Review Comment:
So you're saying it's for future use?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891479272
##########
mgmt2/config/FileManager.cc:
##########
@@ -91,12 +91,11 @@ FileManager::FileManager()
this->registerCallback(&handle_file_reload);
// Register the files registry jsonrpc endpoint
- rpc::add_method_handler(
- "filemanager.get_files_registry",
- [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
- return get_files_registry_rpc_endpoint(id, req);
- },
- &rpc::core_ats_rpc_service_provider_handle);
+ rpc::add_method_handler("filemanager.get_files_registry",
+ [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+ return get_files_registry_rpc_endpoint(id, req);
+ },
+ &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});
Review Comment:
I don't want to add unrelated changes to this PR -> https://github.com/apache/trafficserver/pull/8888 @ywkaras
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#issuecomment-1148889794
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891352066
##########
mgmt2/config/FileManager.cc:
##########
@@ -91,12 +91,11 @@ FileManager::FileManager()
this->registerCallback(&handle_file_reload);
// Register the files registry jsonrpc endpoint
- rpc::add_method_handler(
- "filemanager.get_files_registry",
- [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
- return get_files_registry_rpc_endpoint(id, req);
- },
- &rpc::core_ats_rpc_service_provider_handle);
+ rpc::add_method_handler("filemanager.get_files_registry",
+ [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+ return get_files_registry_rpc_endpoint(id, req);
+ },
+ &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});
Review Comment:
I suggest then that you make the FileManager constructor private.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898176528
##########
mgmt2/rpc/jsonrpc/Context.h:
##########
@@ -0,0 +1,77 @@
+/**
+ @section license License
+
+ 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.
+*/
+#pragma once
+
+#include <vector>
+#include <functional>
+#include <string_view>
+
+#include "tscore/Errata.h"
+#include "ts/apidefs.h"
+#include "rpc/handlers/common/ErrorUtils.h"
+
+namespace rpc
+{
+const bool RESTRICTED_API{true};
Review Comment:
Why not `constexpr`?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r891364157
##########
mgmt2/config/FileManager.cc:
##########
@@ -91,12 +91,11 @@ FileManager::FileManager()
this->registerCallback(&handle_file_reload);
// Register the files registry jsonrpc endpoint
- rpc::add_method_handler(
- "filemanager.get_files_registry",
- [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
- return get_files_registry_rpc_endpoint(id, req);
- },
- &rpc::core_ats_rpc_service_provider_handle);
+ rpc::add_method_handler("filemanager.get_files_registry",
+ [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+ return get_files_registry_rpc_endpoint(id, req);
+ },
+ &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});
Review Comment:
:open_mouth:
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898177586
##########
mgmt2/rpc/jsonrpc/Context.h:
##########
@@ -0,0 +1,77 @@
+/**
+ @section license License
+
+ 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.
+*/
+#pragma once
+
+#include <vector>
+#include <functional>
+#include <string_view>
+
+#include "tscore/Errata.h"
+#include "ts/apidefs.h"
+#include "rpc/handlers/common/ErrorUtils.h"
+
+namespace rpc
+{
+const bool RESTRICTED_API{true};
+const bool NON_RESTRICTED_API{false};
+///
+/// @brief RPC call context class.
+///
+/// This class is used to carry information from the transport logic to the rpc invocation logic, the transport may need to block
+/// some rpc handlers from being executed which at the time of finish ups reading the raw message is yet too early to know the
+/// actual handler.
+///
+class Context
+{
+ using checker_cb = std::function<void(TSRPCHandlerOptions const &, ts::Errata &)>;
+ /// @brief Internal class to hold the permission checker part.
+ struct Auth {
+ /// Checks for permissions. This function checks for every registered permission checker.
+ ///
+ /// @param options Registered handler options.
+ /// @return ts::Errata The errata will be filled by each of the registered checkers, if there was any issue validating the
+ /// call, then the errata reflects that.
+ ts::Errata any_blockers(TSRPCHandlerOptions const &options) const;
Review Comment:
Might be better named "is_blocked".
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r901787309
##########
mgmt2/rpc/jsonrpc/Defs.h:
##########
@@ -45,11 +45,20 @@ class RPCHandlerResponse
};
struct RPCResponseInfo {
- RPCResponseInfo(std::optional<std::string> const &id_) : id(id_) {}
+ RPCResponseInfo(std::optional<std::string> const &id_) : id{id_} {}
+ RPCResponseInfo(std::error_code const &ec_) : error{ec_, {}} {}
+ RPCResponseInfo(std::optional<std::string> const &id_, std::error_code const &ec_) : error{ec_, {}}, id{id_} {}
+ RPCResponseInfo(std::optional<std::string> const &id_, std::error_code const &ec_, ts::Errata const &errata)
+ : error{ec_, errata}, id{id_}
+ {
+ }
RPCResponseInfo() = default;
RPCHandlerResponse callResult;
- std::error_code rpcError;
+ struct Error {
+ std::error_code ec;
Review Comment:
Yes, this is on my TODO list once we have libswoc in our private repo.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898185365
##########
mgmt2/rpc/jsonrpc/Defs.h:
##########
@@ -45,11 +45,20 @@ class RPCHandlerResponse
};
struct RPCResponseInfo {
- RPCResponseInfo(std::optional<std::string> const &id_) : id(id_) {}
+ RPCResponseInfo(std::optional<std::string> const &id_) : id{id_} {}
+ RPCResponseInfo(std::error_code const &ec_) : error{ec_, {}} {}
+ RPCResponseInfo(std::optional<std::string> const &id_, std::error_code const &ec_) : error{ec_, {}}, id{id_} {}
+ RPCResponseInfo(std::optional<std::string> const &id_, std::error_code const &ec_, ts::Errata const &errata)
+ : error{ec_, errata}, id{id_}
+ {
+ }
RPCResponseInfo() = default;
RPCHandlerResponse callResult;
- std::error_code rpcError;
+ struct Error {
+ std::error_code ec;
Review Comment:
We can fix this once we switch to `swoc::Errata`. It supports embedding the `ec` in the `Errata`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #8865: JSON-RPC: Add support for handler to pass information about the specifics when registering.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #8865:
URL: https://github.com/apache/trafficserver/pull/8865#discussion_r898180168
##########
mgmt2/rpc/jsonrpc/Defs.h:
##########
@@ -45,11 +45,20 @@ class RPCHandlerResponse
};
struct RPCResponseInfo {
- RPCResponseInfo(std::optional<std::string> const &id_) : id(id_) {}
+ RPCResponseInfo(std::optional<std::string> const &id_) : id{id_} {}
+ RPCResponseInfo(std::error_code const &ec_) : error{ec_, {}} {}
Review Comment:
`std::optional` is even odder if you have these overloads - why not have a constructor with no parameters at all?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org