Add API types for configuration metadata #14132

markylaing · 2024-09-19T10:37:21Z

This is so that we can manage configuration metadata with API extensions as we do with any other API response over /1.0

Adds an API extension for adding entity metadata to GET /1.0/configuration/metadata.
Adds types under shared/api for configuration metadata.
Updates entity metadata such that each entity maps to a JSON object, with requires_project and entitlements fields.
Enforces that generated metadata conforms to the new API type.
Updates the swagger doc for GET /1.0/configuration/metadata to include the newly defined type.

github-actions · 2024-09-19T10:37:36Z

Heads up @mionaalex - the "Documentation" label was applied to this issue.

kadinsayani

LGTM - thanks!

tomponline · 2024-09-23T08:06:51Z

lxd/lxd-metadata/lxd_metadata.go

@@ -285,7 +253,7 @@ func parse(path string, outputJSONPath string, excludedPaths []string, substitut
 						continue
 					}

-					configKeyEntry[metadataMap["key"]].(map[string]any)[dataKVMatch[1]] = detectType(dataKVMatch[2])
+					configKeyEntry[metadataMap["key"]].(map[string]any)[dataKVMatch[1]] = dataKVMatch[2]


I'm not sure about this change.

Where is the value in dataKVMatch[2] coming from?

As for how these are classified, whilst I agree that all config values are strings, my understanding of the metadata definition here is in order to indicate in the docs what sort of string values are allowed for a particular config key.

It looks like dataKVMatch is coming from this regular expression (?m)([\S]+):[\s]+([\S \"\']+) on the key value section of the lxdmeta:generate section of the doc block e.g.:

type: string defaultdesc: `auto` liveupdate: no shortdesc: What to do when evacuating the instance

In this example, the first dataKVMatch outputted from lxdDocDataRegex.FindAllStringSubmatch(data, -1) would have

dataKVMatch[1] -> "type" dataKVMatch[2] -> "string"

My feeling is that lxd-generate shouldn't be too clever about figuring out which config keys are which type. We should just include that in the description of the key. There is only one config key for which this appears to work.

There is only one config key for which this appears to work.
which one?

The commit directly after this change shows the result of make update-metadata. Only one field is changed: 2614bb5#diff-d3459341f992192c3b18b6130aa7e165d305183794a84f219c7ab7512b1b31bcL4316

It is changing the value of defaultdesc from a boolean true to a string "true".

tomponline · 2024-09-23T08:08:42Z

lxd/lxd-metadata/lxd_metadata.go

@@ -309,6 +309,13 @@ func parse(path string, outputJSONPath string, excludedPaths []string, substitut
 		return nil, fmt.Errorf("Error while marshaling project documentation: %v", err)
 	}

+	// Validate that what we've generated is valid against our API definition.
+	var metadataConfiguration api.MetadataConfiguration


tomponline

Apart from the data type change at the start, this seems all good.

tomponline · 2024-09-25T09:03:23Z

needs a rebase.

@edlerd are you happy with this change btw?

Signed-off-by: Mark Laing <mark.laing@canonical.com>

They may be something like "true", but this is still a string. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

This includes adding the `requires_project` field, which will be required for validating `lxc auth permission add/remove` command arguments. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

…finition. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing · 2024-09-25T09:41:49Z

needs a rebase.

Done 👍

edlerd · 2024-09-25T14:45:17Z

@edlerd are you happy with this change btw?

Happy is a big word for a change breaking our integration ;-) It looks all right to me.

One concern is the project_specific field being boolean. It seems not 100% correct, as some entities can be project specific or not, depending on how the project is set up. So IMHO for networks there should be a third option depends and the project_specific field be modelled as non-binary.

markylaing · 2024-09-25T15:09:22Z

@edlerd are you happy with this change btw?

Happy is a big word for a change breaking our integration ;-) It looks all right to me.

One concern is the project_specific field being boolean. It seems not 100% correct, as some entities can be project specific or not, depending on how the project is set up. So IMHO for networks there should be a third option depends and the project_specific field be modelled as non-binary.

I'm a bit confused about this, does this relate to the features.* config on a project?

edlerd · 2024-09-25T15:27:55Z

I'm a bit confused about this, does this relate to the features.* config on a project?

Yes, exactly. You can share a network between all projects, so it is not really project specific. Or you can enable the features.networks option on a project, and then the networks created in that project are project specific.

markylaing · 2024-09-25T16:16:33Z

I'm a bit confused about this, does this relate to the features.* config on a project?

Yes, exactly. You can share a network between all projects, so it is not really project specific. Or you can enable the features.networks option on a project, and then the networks created in that project are project specific.

I guess this is a difference in phrasing. When a network is "shared" it is just a network in the default project. It's still project specific because it is in the default project.

Let's say you create project "foo" with features.networks=false and lxdbr0 is available. lxdbr0 is still in the default project, so while lxc network show lxdbr0 --project foo would work, lxc auth group permission add <group> network lxdbr0 can_view project=foo would not work because the network is not truly in the foo project.

markylaing · 2024-09-25T16:20:24Z

I'm a bit confused about this, does this relate to the features.* config on a project?

Yes, exactly. You can share a network between all projects, so it is not really project specific. Or you can enable the features.networks option on a project, and then the networks created in that project are project specific.

I guess this is a difference in phrasing. When a network is "shared" it is just a network in the default project. It's still project specific because it is in the default project.

Let's say you create project "foo" with features.networks=false and lxdbr0 is available. lxdbr0 is still in the default project, so while lxc network show lxdbr0 --project foo would work, lxc auth group permission add <group> network lxdbr0 can_view project=foo would not work because the network is not truly in the foo project.

Project features have been a real headache for fine-grained authorization for this reason. I've added a warning to the documentation on this: https://documentation.ubuntu.com/lxd/en/latest/explanation/projects/#isolation-of-projects.

markylaing requested review from tomponline and gabrielmougard September 19, 2024 10:37

markylaing self-assigned this Sep 19, 2024

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Sep 19, 2024

markylaing requested a review from kadinsayani September 19, 2024 10:41

kadinsayani previously approved these changes Sep 19, 2024

View reviewed changes

kadinsayani mentioned this pull request Sep 20, 2024

lxc: Add completions for server keys and console #14115

Open

tomponline reviewed Sep 23, 2024

View reviewed changes

markylaing added 11 commits September 25, 2024 10:20

api: Add metadata_configuration_entity_types API extension.

3efef59

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd-metadata: All configuration keys are strings.

86b70de

They may be something like "true", but this is still a string. Signed-off-by: Mark Laing <mark.laing@canonical.com>

metadata: Runs make update-metadata.

cfbf1aa

Signed-off-by: Mark Laing <mark.laing@canonical.com>

shared/api: Add MetadataConfiguration API struct definitions.

0194217

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/auth/generate: Generate entity type JSON conforming to API type.

8dac37f

This includes adding the `requires_project` field, which will be required for validating `lxc auth permission add/remove` command arguments. Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd-metadata: Update lxd-metadata to use new entity metadata format.

312ab0c

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd-metadata: Validate that the generated JSON conforms to the API de…

9c0c1da

…finition. Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/metadata: Run make update-metadata.

284da9a

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd: Render api.MetadataConfiguration on /1.0/metadata/configuration.

b5ea215

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd: Add MetadataConfiguration to swagger doc string.

2420033

Signed-off-by: Mark Laing <mark.laing@canonical.com>

doc: Runs make update-api.

d594de4

Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing dismissed kadinsayani’s stale review via d594de4 September 25, 2024 09:20

markylaing force-pushed the metadata-api-types branch from 1b74932 to d594de4 Compare September 25, 2024 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API types for configuration metadata #14132

Add API types for configuration metadata #14132

markylaing commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

kadinsayani left a comment

tomponline Sep 23, 2024

markylaing Sep 24, 2024

tomponline Sep 25, 2024

markylaing Sep 25, 2024

tomponline Sep 23, 2024

tomponline left a comment

tomponline commented Sep 25, 2024

markylaing commented Sep 25, 2024

edlerd commented Sep 25, 2024

markylaing commented Sep 25, 2024

edlerd commented Sep 25, 2024

markylaing commented Sep 25, 2024

markylaing commented Sep 25, 2024

Add API types for configuration metadata #14132

Are you sure you want to change the base?

Add API types for configuration metadata #14132

Conversation

markylaing commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

kadinsayani left a comment

Choose a reason for hiding this comment

tomponline Sep 23, 2024

Choose a reason for hiding this comment

markylaing Sep 24, 2024

Choose a reason for hiding this comment

tomponline Sep 25, 2024

Choose a reason for hiding this comment

markylaing Sep 25, 2024

Choose a reason for hiding this comment

tomponline Sep 23, 2024

Choose a reason for hiding this comment

tomponline left a comment

Choose a reason for hiding this comment

tomponline commented Sep 25, 2024

markylaing commented Sep 25, 2024

edlerd commented Sep 25, 2024

markylaing commented Sep 25, 2024

edlerd commented Sep 25, 2024

markylaing commented Sep 25, 2024

markylaing commented Sep 25, 2024