Refactor config #150

johananl · 2023-08-21T16:27:31Z

This PR refactors the KBS configuration scheme.

See commit message for details about the change set.

Fixes #78.

cc: @Xynnn007 @surajssd

fitzthum

This looks cool. I will try to do full review soon. Just one question for now.

docs/config.md

fitzthum

LGTM. I left a few very minor questions, but none of them really need to be followed.

I want to make sure @Xynnn007 and @jialez0 are fine with what is a fairly significant change.

This shouldn't yet affect the CI of any other parts of the project, right?

fitzthum · 2023-09-01T14:20:48Z

src/api/src/config.rs

+            .set_default("attestation_token_type", "CoCo")?
+            .set_default("sockets", vec!["127.0.0.1:8080"])?
+            .set_default("insecure_api", false)?
+            .set_default("insecure_http", false)?


Would it be slightly cleaner to define all of these as constants?

I assume you mean define the values as constants and not the keys. I've pushed new changes as follows:

I've used the default value of AttestationTokenVerifierType for attestation_token_type.

I've added a constant for the default socket address.

The two insecure_ values are bools. Does it make sense to you to use constants for these, too?

The timeout value was already a constant.

Could put the bools as constant just for completeness but I suspect we're not going to be changing those values anytime soon.

Done. I've used constants for the bools, too.

I resorted to adding a string constant for attestation_token_type because using the default from AttestationTokenVerifierType is tricky due to conditional compilation: It breaks for some Cargo features and I can't seem to find a way to conditionally call set_default().

fitzthum · 2023-09-01T14:29:58Z

src/api/src/attestation/coco/grpc.rs

@@ -36,13 +36,26 @@ fn to_grpc_tee(tee: Tee) -> GrpcTee {
    }
 }

+#[derive(Clone, Debug, Deserialize)]
+pub struct GrpcConfig {


Is there any value to defining all the different configs in the main config.rs?

I'm not sure. It does make sense to me to let each implementation define which parameters it needs. In the same vein, KbsConfig isn't intended to be one central config struct which we pass around the entire codebase (I know we currently pass it to AttestationService::new() and am not fond of it but not sure if I have a better alternative since this constructor has conditional compilation and requires one of the following: as_config, grpc_config, amber_config).

But maybe there is merit to the idea of one central place for storing all config-related types.

WDYT?

P. S. As a side note to keep in mind, it's easy to run into circular dependencies here.

but not sure if I have a better alternative

Maybe we can have 3 constructors with feature toggles 👀

Yeah I think it's fine either way tbh.

OK, I've refactored so that we now explicitly pass an implementation-specific config to AttestationService::new() and use 3 implementations for this constructor. Looks cleaner to me this way.

fitzthum · 2023-09-01T14:30:57Z

src/api/src/resource/local_fs.rs

@@ -7,17 +7,17 @@ use anyhow::{Context, Result};
 use serde::Deserialize;
 use std::path::{Path, PathBuf};

-const DEFAULT_REPO_DIR_PATH: &str = "/opt/confidential-containers/kbs/repository";
+pub const DEFAULT_REPO_DIR_PATH: &str = "/opt/confidential-containers/kbs/repository";


Maybe constants like this could go in config.rs?

We have a default implementation for LocalFsRepoDesc which relies on this constant. If we move the constant to config.rs we'd have to import config.rs in local_fs.rs. I think this isn't ideal and opens the way for import cycles (because local_fs.rs is used in resource/mod.rs which is in turn used in config.rs.

What do you think?

johananl · 2023-09-01T15:03:03Z

This shouldn't yet affect the CI of any other parts of the project, right?

@fitzthum the CI is green as far as I can tell. I did need to touch the e2e config so that it matches the new config schema but this isn't a CI change really. Not sure about other parts of the project - I only touched KBS and can see that coco-keyprovider is able to talk to KBS when using the included docker-compose file. Is that enough?

fitzthum · 2023-09-01T17:36:29Z

I think there might be an instance of the KBS running on the TDX CI node for the CCv0 tests. There is some chance that would need to be adjusted somewhat, but not something you would need to worry about.

There are a couple of errors in the CI. Looks like one may be OpenSSL issue, which we've run into a few places recently.

johananl · 2023-09-04T10:59:30Z

I think there might be an instance of the KBS running on the TDX CI node for the CCv0 tests. There is some chance that would need to be adjusted somewhat, but not something you would need to worry about.

@fitzthum I'm not sure I understand the issue. Are we talking about a place where KBS gets executed in which we may need to adjust CLI args and config files to match the new config schema?

johananl · 2023-09-04T11:02:02Z

I'm looking into the e2e failures.

johananl · 2023-09-04T12:21:20Z

Huh. Looks like CI is green now 🤷

- Use `config` crate for configuration handling. - Use TOML as the canonical configuration format (JSON is still supported). - Consolidate all KBS config under a single KbsConfig struct. - Read all params except --config-file from a config file. - Refactor repository configuration. Consolidate all related properties under a new RepositoryConfig enum and express implementation-specific properties as structs rather than raw JSON. - Improve default handling for some types. - Provide config examples for the various attestation modes. - Reduce ambiguity by renaming multiple elements named "config" to more meaningful names. - Overhaul config.md to match new state. Add information about default values as well as feature flags. - Clean the coupling of crate::attestation::coco::grpc and the main module. grpc no longer depends on KbsConfig. - Pass explicit attestation configuration to Attest implementation constructors. - Update docker-compose and k8s files to match new config structure. Signed-off-by: Johanan Liebermann <[email protected]>

Signed-off-by: Johanan Liebermann <[email protected]>

johananl · 2023-09-04T14:18:41Z

Some CI tasks which were green previously are red again. I don't think I have permissions to rerun.

jialez0

LGTM. Nice changes, thanks! @johananl

johananl force-pushed the refactor-config branch from 0d76719 to 6e9bd7e Compare August 21, 2023 16:30

johananl mentioned this pull request Aug 22, 2023

Use configuration file to start KBS #78

Closed

fitzthum reviewed Aug 31, 2023

View reviewed changes

docs/config.md Show resolved Hide resolved

fitzthum approved these changes Sep 1, 2023

View reviewed changes

johananl force-pushed the refactor-config branch from f796ebe to 98eeaec Compare September 1, 2023 15:13

johananl force-pushed the refactor-config branch 2 times, most recently from 75c2cd2 to 577ae77 Compare September 4, 2023 10:56

johananl added 2 commits September 4, 2023 16:38

Update e2e

315a9e7

Signed-off-by: Johanan Liebermann <[email protected]>

johananl force-pushed the refactor-config branch from 577ae77 to 315a9e7 Compare September 4, 2023 13:39

jialez0 approved these changes Sep 5, 2023

View reviewed changes

jialez0 merged commit 9fc882a into confidential-containers:main Sep 5, 2023

johananl deleted the refactor-config branch September 5, 2023 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor config #150

Refactor config #150

johananl commented Aug 21, 2023

fitzthum left a comment

fitzthum left a comment

fitzthum Sep 1, 2023

johananl Sep 1, 2023

fitzthum Sep 1, 2023

johananl Sep 4, 2023 •

edited

Loading

fitzthum Sep 1, 2023

johananl Sep 1, 2023 •

edited

Loading

johananl Sep 1, 2023

fitzthum Sep 1, 2023

johananl Sep 4, 2023 •

edited

Loading

fitzthum Sep 1, 2023

johananl Sep 4, 2023

johananl commented Sep 1, 2023

fitzthum commented Sep 1, 2023

johananl commented Sep 4, 2023

johananl commented Sep 4, 2023

johananl commented Sep 4, 2023

johananl commented Sep 4, 2023

jialez0 left a comment

Refactor config #150

Refactor config #150

Conversation

johananl commented Aug 21, 2023

fitzthum left a comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl commented Sep 1, 2023

fitzthum commented Sep 1, 2023

johananl commented Sep 4, 2023

johananl commented Sep 4, 2023

johananl commented Sep 4, 2023

johananl commented Sep 4, 2023

jialez0 left a comment

Choose a reason for hiding this comment

johananl Sep 4, 2023 •

edited

Loading

johananl Sep 1, 2023 •

edited

Loading

johananl Sep 4, 2023 •

edited

Loading