-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AA: Add Config file mechanism #454
Conversation
5eabaea
to
f1fb8f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
- Currently the config is read lazily when a get token request is received. What if we have the configuration file set at the launch of AA? e.g. firstly read the cmdline parameter to check if config-path is given. If not, try to read default path. If no default config is given, use a built-in default config.
- Due to some discussion. Now we might be interested in add another API of AA name
UpdateConfiguration
which could be only called once. See AA | Design ideas to verify the initdata #446 (comment)
@@ -3,7 +3,7 @@ | |||
// SPDX-License-Identifier: Apache-2.0 | |||
// | |||
|
|||
use attestation_agent::aa_kbc_params; | |||
use attestation_agent::config::aa_kbc_params; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR also moves aa_kbc_params
to another place. Could we bring this part out as a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these two parts can be classified as a commit, because aa_kbc_params
themselves are part of the AA configuration parameters, so I think it is OK to put them in a commit.
} | ||
} | ||
|
||
impl AttestationAgent { | ||
/// Create a new instance of [AttestationAgent]. | ||
pub fn new() -> Self { | ||
pub fn new(config_path: &str) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is never called due to the code. Is this intentded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is indeed not used in AA's gRPC binary, but AA library, as an independent Rust Crate, will provide support for a wider range of scenarios, so it is necessary to provide the API here.
cc @mkulke if he gets some time to look into this |
attestation-agent/lib/src/lib.rs
Outdated
let _url = match aa_kbc_params::get_params().await { | ||
Ok(params) => params.uri().to_string(), | ||
Err(_) => { | ||
let config = config::Config::try_from(self.config_file_path.as_str()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably don't want to re-read the configuration file on every get_token() call, maybe we can do this lazily or better yet, have the configuration option on the attestation-agent binary as a cli param and use it as initialization param for the AA struct:
AttestationAgent {
kbc_module_list: KbcModuleList::new(),
kbc_instance_map: HashMap::new(),
config: Option<config::Config>,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n/a
@Xynnn007 @mkulke The reason why the config here is read lazily is that we want to retain the ability to dynamically modify the configuration file when AA is running (for example, other components in TEE want to modify some configuration parameters of AA). I'm not sure whether this is necessary, but it is flexible enough, and it may bring convenience to other scheme designs in the future. |
@Xynnn007 Instead of designing such an interface, I think it's better to modify the configuration file of AA directly. |
a018887
to
1259e1a
Compare
It might be a bit unintuitive, the semantics of a configuration file is usually "read once, at launch". For a library the intended behaviour is a bit fuzzy, that's why I would suggest to:
If we do this, we would have to account for race conditions. If there is API in the AA lib that would rewrite the config file, we could put it behind a mutex guard, I guess. (still, going from AA -> filesystem -> AA is to propagate config changes feels unnecessary) If we modify the config file out-of-process this wouldn't work. |
This makes sense. I will modify this PR to support configuration reading at startup. |
@Xynnn007 @mkulke For how to maintain the dynamic change ability of AA configuration files, perhaps we can consider such a design (similar to systemd and nginx):
What do you think? |
sounds reasonable to me, maybe a bit cumbersome to implement. but we could implement such a reload-trigger logic once we need it and go with a read-once solution for now. |
Signed-off-by: Jiale Zhang <zhangjiale@linux.alibaba.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
cc @Xynnn007