* [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-16 9:50 ` Wolfgang Bumiller
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 2/5] Cluster: add get_guest_config_properties Dominik Csapak
` (21 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
for getting multiple properties from the in memory config of the
guests in one go. Keep the existing IPC call as is for backward
compatibility and add this as separate, new one.
It basically behaves the same as
CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
instead and returns multiple properties per guest.
The old way of getting a single property is now also done by
the new function, since it basically performs identically.
Here a short benchmark:
Setup:
PVE in a VM with cpu type host (12700k) and 4 cores
10000 typical configs with both 'lock' and 'tags' set at the end
and fairly long tags ('test-tag1,test-tag2,test-tag3')
(normal vm with a snapshot, ~1KiB)
i let it run 100 times each, times in ms
old code:
total time time per iteration
1054.2 10.2
new code:
num props total time time per iter function
2 1332.2 13.2 get_properties
1 1051.2 10.2 get_properties
2 2082.2 20.2 get_property (2 calls to get_property)
1 1034.2 10.2 get_property
so a call with the new code for one property is the same as with the
old code, and adding a second property only adds a bit of additional
time (in this case ~30%)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* line_len, prop_len -> size_t
* document min/max
* remove setting of null byte in loop (it's already split at the null byte)
* improved the loop for skipping whitespace at the beginning
data/src/cfs-ipc-ops.h | 2 +
data/src/server.c | 63 ++++++++++++++
data/src/status.c | 184 ++++++++++++++++++++++++++++-------------
data/src/status.h | 3 +
4 files changed, 194 insertions(+), 58 deletions(-)
diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
index 003e233..249308d 100644
--- a/data/src/cfs-ipc-ops.h
+++ b/data/src/cfs-ipc-ops.h
@@ -43,4 +43,6 @@
#define CFS_IPC_VERIFY_TOKEN 12
+#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13
+
#endif
diff --git a/data/src/server.c b/data/src/server.c
index 549788a..80f838b 100644
--- a/data/src/server.c
+++ b/data/src/server.c
@@ -89,6 +89,13 @@ typedef struct {
char property[];
} cfs_guest_config_propery_get_request_header_t;
+typedef struct {
+ struct qb_ipc_request_header req_header;
+ uint32_t vmid;
+ uint8_t num_props;
+ char props[]; /* list of \0 terminated properties */
+} cfs_guest_config_properties_get_request_header_t;
+
typedef struct {
struct qb_ipc_request_header req_header;
char token[];
@@ -348,6 +355,62 @@ static int32_t s1_msg_process_fn(
result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
}
+ } else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
+
+ cfs_guest_config_properties_get_request_header_t *rh =
+ (cfs_guest_config_properties_get_request_header_t *) data;
+
+ size_t remaining = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);
+
+ result = 0;
+ if (rh->vmid < 100 && rh->vmid != 0) {
+ cfs_debug("vmid out of range %u", rh->vmid);
+ result = -EINVAL;
+ } else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
+ result = -ENOENT;
+ } else if (rh->num_props == 0) {
+ cfs_debug("num_props == 0");
+ result = -EINVAL;
+ } else if (remaining <= 1) {
+ cfs_debug("property length <= 1, %ld", remaining);
+ result = -EINVAL;
+ } else {
+ const char **properties = malloc(sizeof(char*) * rh->num_props);
+ char *current = (rh->props);
+ for (uint8_t i = 0; i < rh->num_props; i++) {
+ size_t proplen = strnlen(current, remaining);
+ if (proplen == 0) {
+ cfs_debug("property length 0");
+ result = -EINVAL;
+ break;
+ }
+ if (proplen == remaining || current[proplen] != '\0') {
+ cfs_debug("property not \\0 terminated");
+ result = -EINVAL;
+ break;
+ }
+ if (current[0] < 'a' || current[0] > 'z') {
+ cfs_debug("property does not start with [a-z]");
+ result = -EINVAL;
+ break;
+ }
+ properties[i] = current;
+ remaining -= (proplen + 1);
+ current += proplen + 1;
+ }
+
+ if (remaining != 0) {
+ cfs_debug("leftover data after parsing %u properties", rh->num_props);
+ result = -EINVAL;
+ }
+
+ if (result == 0) {
+ cfs_debug("cfs_get_guest_config_properties: basic validity checked, do request");
+ result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
+ }
+
+ free(properties);
+ }
} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;
diff --git a/data/src/status.c b/data/src/status.c
index 9bceaeb..47cb42e 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -804,25 +804,55 @@ cfs_create_vmlist_msg(GString *str)
return 0;
}
-// checks the conf for a line starting with '$prop:' and returns the value
-// afterwards, whitout initial whitespace(s), we only deal with the format
-// restricion imposed by our perl VM config parser, main reference is
+// checks if a config line starts with the given prop. if yes, writes a '\0'
+// at the end of the value, and returns the pointer where the value starts
+char *
+_get_property_value_from_line(char *line, size_t line_len, const char *prop, size_t prop_len)
+{
+ if (line_len <= prop_len + 1) return NULL;
+
+ if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
+ char *v_start = &line[prop_len + 1];
+ char *v_end = &line[line_len - 1];
+
+ // drop initial value whitespaces here already
+ while (v_end > v_start && *v_start && isspace(*v_start)) v_start++;
+
+ if (!*v_start) return NULL;
+
+ while (v_end > v_start && isspace(*v_end)) v_end--;
+ v_end[1] = '\0';
+
+ return v_start;
+ }
+
+ return NULL;
+}
+
+// checks the conf for lines starting with the given props and
+// writes the pointers into the correct positions into the 'found' array
+// afterwards, without initial whitespace(s), we only deal with the format
+// restriction imposed by our perl VM config parser, main reference is
// PVE::QemuServer::parse_vm_config this allows to be very fast and still
// relatively simple
-// main restrictions used for our advantage is the properties match reges:
+// main restrictions used for our advantage is the properties match regex:
// ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) from parse_vm_config
// currently we only look at the current configuration in place, i.e., *no*
-// snapshort and *no* pending changes
-static char *
-_get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
+// snapshot and *no* pending changes
+//
+// min..max is the char range of the first character of the given props,
+// so that we can return early when checking the line
+void
+_get_property_values(char **found, char *conf, int conf_size, const char **props, uint8_t num_props, char min, char max)
{
const char *const conf_end = conf + conf_size;
char *line = conf;
- size_t remaining_size;
+ size_t remaining_size = conf_size;
+ int count = 0;
char *next_newline = memchr(conf, '\n', conf_size);
if (next_newline == NULL) {
- return NULL; // valid property lines end with \n, but none in the config
+ return; // valid property lines end with \n, but none in the config
}
*next_newline = '\0';
@@ -830,41 +860,32 @@ _get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
if (!line[0]) goto next;
// snapshot or pending section start, but nothing found yet -> not found
- if (line[0] == '[') return NULL;
- // properties start with /^[a-z]/, so continue early if not
- if (line[0] < 'a' || line[0] > 'z') goto next;
-
- int line_len = strlen(line);
- if (line_len <= prop_len + 1) goto next;
-
- if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
- char *v_start = &line[prop_len + 1];
-
- // drop initial value whitespaces here already
- while (*v_start && isspace(*v_start)) v_start++;
-
- if (!*v_start) return NULL;
-
- char *v_end = &line[line_len - 1];
- while (v_end > v_start && isspace(*v_end)) v_end--;
- v_end[1] = '\0';
-
- return v_start;
+ if (line[0] == '[') return;
+ // continue early if line does not begin with the min/max char of the properties
+ if (line[0] < min || line[0] > max) goto next;
+
+ size_t line_len = strlen(line);
+ for (uint8_t i = 0; i < num_props; i++) {
+ char * value = _get_property_value_from_line(line, line_len, props[i], strlen(props[i]));
+ if (value != NULL) {
+ count += (found[i] != NULL) & 0x1; // count newly found lines
+ found[i] = value;
+ }
+ }
+ if (count == num_props) {
+ return; // found all
}
next:
line = next_newline + 1;
remaining_size = conf_end - line;
- if (remaining_size <= prop_len) {
- return NULL;
- }
next_newline = memchr(line, '\n', remaining_size);
if (next_newline == NULL) {
- return NULL; // valid property lines end with \n, but none in the config
+ return; // valid property lines end with \n, but none in the config
}
*next_newline = '\0';
}
- return NULL; // not found
+ return;
}
static void
@@ -883,24 +904,77 @@ _g_str_append_kv_jsonescaped(GString *str, const char *k, const char *v)
}
int
-cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid)
+_print_found_properties(
+ GString *str,
+ gpointer conf,
+ int size,
+ const char **props,
+ uint8_t num_props,
+ uint32_t vmid,
+ char **values,
+ char min,
+ char max,
+ int first)
+{
+ _get_property_values(values, conf, size, props, num_props, min, max);
+
+ uint8_t found = 0;
+ for (uint8_t i = 0; i < num_props; i++) {
+ if (values[i] == NULL) {
+ continue;
+ }
+ if (found) {
+ g_string_append_c(str, ',');
+ } else {
+ if (!first) {
+ g_string_append_printf(str, ",\n");
+ } else {
+ first = 0;
+ }
+ g_string_append_printf(str, "\"%u\":{", vmid);
+ found = 1;
+ }
+ _g_str_append_kv_jsonescaped(str, props[i], values[i]);
+ }
+
+ if (found) {
+ g_string_append_c(str, '}');
+ }
+
+ return first;
+}
+
+int
+cfs_create_guest_conf_properties_msg(GString *str, memdb_t *memdb, const char **props, uint8_t num_props, uint32_t vmid)
{
g_return_val_if_fail(cfs_status.vmlist != NULL, -EINVAL);
g_return_val_if_fail(str != NULL, -EINVAL);
- int prop_len = strlen(prop);
- int res = 0;
- GString *path = NULL;
-
// Prelock &memdb->mutex in order to enable the usage of memdb_read_nolock
// to prevent Deadlocks as in #2553
g_mutex_lock (&memdb->mutex);
g_mutex_lock (&mutex);
- g_string_printf(str,"{\n");
+ g_string_printf(str, "{\n");
GHashTable *ht = cfs_status.vmlist;
+
+ int res = 0;
+ GString *path = NULL;
gpointer tmp = NULL;
+ char **values = calloc(num_props, sizeof(char*));
+ char min = 'z', max = 'a';
+
+ for (uint8_t i = 0; i < num_props; i++) {
+ if (props[i][0] > max) {
+ max = props[i][0];
+ }
+
+ if (props[i][0] < min) {
+ min = props[i][0];
+ }
+ }
+
if (!g_hash_table_size(ht)) {
goto ret;
}
@@ -919,15 +993,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
// use memdb_read_nolock because lock is handled here
int size = memdb_read_nolock(memdb, path->str, &tmp);
if (tmp == NULL) goto err;
- if (size <= prop_len) goto ret;
-
- char *val = _get_property_value(tmp, size, prop, prop_len);
- if (val == NULL) goto ret;
-
- g_string_append_printf(str, "\"%u\":{", vmid);
- _g_str_append_kv_jsonescaped(str, prop, val);
- g_string_append_c(str, '}');
+ _print_found_properties(str, tmp, size, props, num_props, vmid, values, min, max, 1);
} else {
GHashTableIter iter;
g_hash_table_iter_init (&iter, ht);
@@ -943,21 +1010,16 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
tmp = NULL;
// use memdb_read_nolock because lock is handled here
int size = memdb_read_nolock(memdb, path->str, &tmp);
- if (tmp == NULL || size <= prop_len) continue;
+ if (tmp == NULL) continue;
- char *val = _get_property_value(tmp, size, prop, prop_len);
- if (val == NULL) continue;
-
- if (!first) g_string_append_printf(str, ",\n");
- else first = 0;
-
- g_string_append_printf(str, "\"%u\":{", vminfo->vmid);
- _g_str_append_kv_jsonescaped(str, prop, val);
- g_string_append_c(str, '}');
+ memset(values, 0, sizeof(char*)*num_props); // reset array
+ first = _print_found_properties(str, tmp, size, props, num_props,
+ vminfo->vmid, values, min, max, first);
}
}
ret:
g_free(tmp);
+ free(values);
if (path != NULL) {
g_string_free(path, TRUE);
}
@@ -973,6 +1035,12 @@ enoent:
goto ret;
}
+int
+cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid)
+{
+ return cfs_create_guest_conf_properties_msg(str, memdb, &prop, 1, vmid);
+}
+
void
record_memdb_change(const char *path)
{
diff --git a/data/src/status.h b/data/src/status.h
index bbf0948..041cb34 100644
--- a/data/src/status.h
+++ b/data/src/status.h
@@ -163,4 +163,7 @@ cfs_create_memberlist_msg(
int
cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid);
+int
+cfs_create_guest_conf_properties_msg(GString *str, memdb_t *memdb, const char **props, uint8_t num_props, uint32_t vmid);
+
#endif /* _PVE_STATUS_H_ */
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
@ 2022-11-16 9:50 ` Wolfgang Bumiller
0 siblings, 0 replies; 44+ messages in thread
From: Wolfgang Bumiller @ 2022-11-16 9:50 UTC (permalink / raw)
To: Dominik Csapak; +Cc: pve-devel
On Tue, Nov 15, 2022 at 02:02:26PM +0100, Dominik Csapak wrote:
> for getting multiple properties from the in memory config of the
> guests in one go. Keep the existing IPC call as is for backward
> compatibility and add this as separate, new one.
>
> It basically behaves the same as
> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
> instead and returns multiple properties per guest.
>
> The old way of getting a single property is now also done by
> the new function, since it basically performs identically.
>
> Here a short benchmark:
>
> Setup:
> PVE in a VM with cpu type host (12700k) and 4 cores
> 10000 typical configs with both 'lock' and 'tags' set at the end
> and fairly long tags ('test-tag1,test-tag2,test-tag3')
> (normal vm with a snapshot, ~1KiB)
>
> i let it run 100 times each, times in ms
>
> old code:
>
> total time time per iteration
> 1054.2 10.2
>
> new code:
>
> num props total time time per iter function
> 2 1332.2 13.2 get_properties
> 1 1051.2 10.2 get_properties
> 2 2082.2 20.2 get_property (2 calls to get_property)
> 1 1034.2 10.2 get_property
>
> so a call with the new code for one property is the same as with the
> old code, and adding a second property only adds a bit of additional
> time (in this case ~30%)
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
Sort-of
Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
but I do have a bit of a maintainability-complaint and potential actual
issue (see below).
> changes from v9:
> * line_len, prop_len -> size_t
> * document min/max
> * remove setting of null byte in loop (it's already split at the null byte)
> * improved the loop for skipping whitespace at the beginning
> data/src/cfs-ipc-ops.h | 2 +
> data/src/server.c | 63 ++++++++++++++
> data/src/status.c | 184 ++++++++++++++++++++++++++++-------------
> data/src/status.h | 3 +
> 4 files changed, 194 insertions(+), 58 deletions(-)
>
> diff --git a/data/src/status.c b/data/src/status.c
> index 9bceaeb..47cb42e 100644
> --- a/data/src/status.c
> +++ b/data/src/status.c
> @@ -804,25 +804,55 @@ cfs_create_vmlist_msg(GString *str)
> return 0;
> }
>
> -// checks the conf for a line starting with '$prop:' and returns the value
> -// afterwards, whitout initial whitespace(s), we only deal with the format
> -// restricion imposed by our perl VM config parser, main reference is
> +// checks if a config line starts with the given prop. if yes, writes a '\0'
> +// at the end of the value, and returns the pointer where the value starts
Please add:
// `line[line_len]` is guaranteed to be `\0`
keep reading for why ;-)
> +char *
> +_get_property_value_from_line(char *line, size_t line_len, const char *prop, size_t prop_len)
> +{
> + if (line_len <= prop_len + 1) return NULL;
> +
> + if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
> + char *v_start = &line[prop_len + 1];
> + char *v_end = &line[line_len - 1];
nit: in most C code an start and end pointers are exclusive on the end side
(iow. end should point to the first invalid address) it's a bit
confusing doing it this way and may lead to confusion...
> +
> + // drop initial value whitespaces here already
> + while (v_end > v_start && *v_start && isspace(*v_start)) v_start++;
(((that's fine, but I find 'v_start < v_end' much easier to parse)))
> +
> + if (!*v_start) return NULL;
> +
this is where the v_end tripped me up a bit:
- if there's no value, v_start == v_end and the loop won't run
> + while (v_end > v_start && isspace(*v_end)) v_end--;
> + v_end[1] = '\0';
which makes this read as "write to 1 past the end"
which *seems* fine at first given that we split the lines by replacing
'\n' with '\0', but then there's another case to think about:
The original data just came from a `memdb_read()` which just gives us a
"binary blob" and may not be newline-terminated.
However, the *caller* guarantees that in this case, we simply ignore the
final line.
BUT: do we do the same in perl?
> +
> + return v_start;
> + }
> +
> + return NULL;
> +}
> +
> +// checks the conf for lines starting with the given props and
> +// writes the pointers into the correct positions into the 'found' array
> +// afterwards, without initial whitespace(s), we only deal with the format
> +// restriction imposed by our perl VM config parser, main reference is
> // PVE::QemuServer::parse_vm_config this allows to be very fast and still
> // relatively simple
> -// main restrictions used for our advantage is the properties match reges:
> +// main restrictions used for our advantage is the properties match regex:
> // ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) from parse_vm_config
> // currently we only look at the current configuration in place, i.e., *no*
> -// snapshort and *no* pending changes
> -static char *
> -_get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
> +// snapshot and *no* pending changes
> +//
> +// min..max is the char range of the first character of the given props,
> +// so that we can return early when checking the line
> +void
> +_get_property_values(char **found, char *conf, int conf_size, const char **props, uint8_t num_props, char min, char max)
> {
> const char *const conf_end = conf + conf_size;
> char *line = conf;
> - size_t remaining_size;
> + size_t remaining_size = conf_size;
> + int count = 0;
^ I'd prefer an uint8_t here and call it `prop_count` as it's strictly
to count towards the above `uint8_t num_props`, so `int` makes little
sense, and the generic name `count` with type `uint8_t` is also a bit
awkward ;-)
>
> char *next_newline = memchr(conf, '\n', conf_size);
> if (next_newline == NULL) {
> - return NULL; // valid property lines end with \n, but none in the config
> + return; // valid property lines end with \n, but none in the config
> }
> *next_newline = '\0';
>
> @@ -830,41 +860,32 @@ _get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
> if (!line[0]) goto next;
>
> // snapshot or pending section start, but nothing found yet -> not found
> - if (line[0] == '[') return NULL;
> - // properties start with /^[a-z]/, so continue early if not
> - if (line[0] < 'a' || line[0] > 'z') goto next;
> -
> - int line_len = strlen(line);
> - if (line_len <= prop_len + 1) goto next;
> -
> - if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
> - char *v_start = &line[prop_len + 1];
> -
> - // drop initial value whitespaces here already
> - while (*v_start && isspace(*v_start)) v_start++;
> -
> - if (!*v_start) return NULL;
> -
> - char *v_end = &line[line_len - 1];
> - while (v_end > v_start && isspace(*v_end)) v_end--;
> - v_end[1] = '\0';
> -
> - return v_start;
> + if (line[0] == '[') return;
> + // continue early if line does not begin with the min/max char of the properties
> + if (line[0] < min || line[0] > max) goto next;
> +
> + size_t line_len = strlen(line);
This is only fine because we skip the final line if it's not
`\n`-terminated (as it's otherwise not guaranteed to be `\0`-terminated)
But we don't even need to re-scan the line, since we already did that,
and should just be able to use:
line_len = next_newline - line;
no?
> + for (uint8_t i = 0; i < num_props; i++) {
> + char * value = _get_property_value_from_line(line, line_len, props[i], strlen(props[i]));
> + if (value != NULL) {
> + count += (found[i] != NULL) & 0x1; // count newly found lines
> + found[i] = value;
> + }
> + }
> + if (count == num_props) {
> + return; // found all
> }
> next:
> line = next_newline + 1;
> remaining_size = conf_end - line;
> - if (remaining_size <= prop_len) {
> - return NULL;
> - }
> next_newline = memchr(line, '\n', remaining_size);
> if (next_newline == NULL) {
> - return NULL; // valid property lines end with \n, but none in the config
> + return; // valid property lines end with \n, but none in the config
> }
> *next_newline = '\0';
> }
>
> - return NULL; // not found
> + return;
> }
>
> static void
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH cluster v10 2/5] Cluster: add get_guest_config_properties
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 3/5] datacenter.cfg: add option for tag-style Dominik Csapak
` (20 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
akin to get_guest_config_property, but with a list of properties.
uses the new CFS_IPC_GET_GUEST_CONFIG_PROPERTIES
also adds the same NOTEs regarding parsing/permissions to the comment
of get_guest_config_property
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
data/PVE/Cluster.pm | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index abcc46d..99e7975 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -339,10 +339,37 @@ sub get_node_kv {
return $res;
}
+# properties: an array-ref of config properties you want to get, e.g., this
+# is perfect to get multiple properties of a guest _fast_
+# (>100 faster than manual parsing here)
+# vmid: optional, if a valid is passed we only check that one, else return all
+# NOTE: does *not* searches snapshot and PENDING entries sections!
+# NOTE: returns the guest config lines (excluding trailing whitespace) as is,
+# so for non-trivial properties, checking the validity must be done
+# NOTE: no permission check is done, that is the responsibilty of the caller
+sub get_guest_config_properties {
+ my ($properties, $vmid) = @_;
+
+ die "properties required" if !defined($properties);
+
+ my $num_props = scalar(@$properties);
+ die "only up to 255 properties supported" if $num_props > 255;
+ my $bindata = pack "VC", $vmid // 0, $num_props;
+ for my $property (@$properties) {
+ $bindata .= pack "Z*", $property;
+ }
+ my $res = $ipcc_send_rec_json->(CFS_IPC_GET_GUEST_CONFIG_PROPERTIES, $bindata);
+
+ return $res;
+}
+
# property: a config property you want to get, e.g., this is perfect to get
# the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
# vmid: optional, if a valid is passed we only check that one, else return all
# NOTE: does *not* searches snapshot and PENDING entries sections!
+# NOTE: returns the guest config lines (excluding trailing whitespace) as is,
+# so for non-trivial properties, checking the validity must be done
+# NOTE: no permission check is done, that is the responsibilty of the caller
sub get_guest_config_property {
my ($property, $vmid) = @_;
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH cluster v10 3/5] datacenter.cfg: add option for tag-style
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 2/5] Cluster: add get_guest_config_properties Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config Dominik Csapak
` (19 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
its a property string containing 'tree-shape' and 'colors'
the colors are formatted like this:
<tag>:<background-color>[:<text-color>]
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
data/PVE/DataCenterConfig.pm | 37 ++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/data/PVE/DataCenterConfig.pm b/data/PVE/DataCenterConfig.pm
index 6a2adee..532e5e5 100644
--- a/data/PVE/DataCenterConfig.pm
+++ b/data/PVE/DataCenterConfig.pm
@@ -131,6 +131,29 @@ sub pve_verify_mac_prefix {
return $mac_prefix;
}
+my $COLOR_RE = '[0-9a-fA-F]{6}';
+my $TAG_COLOR_OVERRIDE_RE = "(?:${PVE::JSONSchema::PVE_TAG_RE}:${COLOR_RE}(?:\:${COLOR_RE})?)";
+
+my $tag_style_format = {
+ 'shape' => {
+ optional => 1,
+ type => 'string',
+ enum => ['full', 'circle', 'dense', 'none'],
+ default => 'circle',
+ description => "Tag shape for the web ui tree. 'full' draws the full tag. "
+ ."'circle' draws only a circle with the background color. "
+ ."'dense' only draws a small rectancle (useful when many tags are assigned to each guest)."
+ ."'none' disables showing the tags.",
+ },
+ 'color-map' => {
+ optional => 1,
+ type => 'string',
+ pattern => "${TAG_COLOR_OVERRIDE_RE}(?:\;$TAG_COLOR_OVERRIDE_RE)*",
+ typetext => '<tag>:<hex-color>[:<hex-color-for-text>][;<tag>=...]',
+ description => "Manual color mapping for tags (semicolon separated).",
+ },
+};
+
my $datacenter_schema = {
type => "object",
additionalProperties => 0,
@@ -256,6 +279,12 @@ my $datacenter_schema = {
maxLength => 64 * 1024,
optional => 1,
},
+ 'tag-style' => {
+ optional => 1,
+ type => 'string',
+ description => "Tag style options.",
+ format => $tag_style_format,
+ },
},
};
@@ -300,6 +329,10 @@ sub parse_datacenter_config {
$res->{webauthn} = parse_property_string($webauthn_format, $webauthn);
}
+ if (my $tag_style = $res->{'tag-style'}) {
+ $res->{'tag-style'} = parse_property_string($tag_style_format, $tag_style);
+ }
+
# for backwards compatibility only, new migration property has precedence
if (defined($res->{migration_unsecure})) {
if (defined($res->{migration}->{type})) {
@@ -359,6 +392,10 @@ sub write_datacenter_config {
$cfg->{webauthn} = PVE::JSONSchema::print_property_string($webauthn, $webauthn_format);
}
+ if (ref(my $tag_style = $cfg->{'tag-style'})) {
+ $cfg->{'tag-style'} = PVE::JSONSchema::print_property_string($tag_style, $tag_style_format);
+ }
+
my $comment = '';
# add description as comment to top of file
my $description = $cfg->{description} || '';
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (2 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 3/5] datacenter.cfg: add option for tag-style Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 15:17 ` Fabian Grünbichler
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 5/5] datacenter.cfg: add 'ordering' to 'tag-style' config Dominik Csapak
` (18 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
by adding a 'user-tag-privileges' and 'admin-tags' option.
The first sets the policy by which "normal" users (with
'VM.Config.Options' on the respective guest) can create/delete tags
and the second is a list of tags only settable by 'admins'
('Sys.Modify' on '/')
also add a helper 'get_allowed_tags' that returns the allowed (existing)
tags, the privileged tags, and if a user can enter 'freeform' tags.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* get_allowed_tags now takes a bool + closure for checking the tag
access, prevents cyclic dependency between cluster and access-control
* use 'for' instead of 'map'
* fix indentation
data/PVE/DataCenterConfig.pm | 105 +++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/data/PVE/DataCenterConfig.pm b/data/PVE/DataCenterConfig.pm
index 532e5e5..f7f90e9 100644
--- a/data/PVE/DataCenterConfig.pm
+++ b/data/PVE/DataCenterConfig.pm
@@ -154,6 +154,32 @@ my $tag_style_format = {
},
};
+my $user_tag_privs_format = {
+ 'user-allow' => {
+ optional => 1,
+ type => 'string',
+ enum => ['none', 'list', 'existing', 'free'],
+ default => 'free',
+ description => "Controls tag usage for users without `Sys.Modify` on `/` bey either "
+ ."allowing `none`, a `list`, already `existing` or anything (`free`).",
+ verbose_description => "Controls which tags can be set or deleted on resources an user "
+ ."controls (such as guests). Users iwth the `Sys.Modify` privilege on `/` are always "
+ ." unrestricted. "
+ ."'none' means no tags are modifiable. "
+ ."'list' allows tags from the given list. "
+ ."'existing' means only already existing tags of resources able to access or from the"
+ ."given list. "
+ ."'free' means users can assign any tags."
+ },
+ 'user-allow-list' => {
+ optional => 1,
+ type => 'string',
+ pattern => "${PVE::JSONSchema::PVE_TAG_RE}(?:\;${PVE::JSONSchema::PVE_TAG_RE})*",
+ typetext => "<tag>[;<tag>...]",
+ description => "List of tags users are allowed to set and delete (semicolon separated).",
+ },
+};
+
my $datacenter_schema = {
type => "object",
additionalProperties => 0,
@@ -285,12 +311,66 @@ my $datacenter_schema = {
description => "Tag style options.",
format => $tag_style_format,
},
+ 'user-tag-access' => {
+ optional => 1,
+ type => 'string',
+ description => "Privilege options for user settable tags",
+ format => $user_tag_privs_format,
+ },
+ 'privileged-tags' => {
+ optional => 1,
+ type => 'string',
+ description => "A list of tags that require a `Sys.Modify` on '/') to set and delete. "
+ ."Tags set here that are also in 'user-tag-access' also require `Sys.Modify`.",
+ pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
+ typetext => "<tag>[;<tag>...]",
+ },
},
};
# make schema accessible from outside (for documentation)
sub get_datacenter_schema { return $datacenter_schema };
+# in scalar context, returns the list of allowed tags that exist
+# in list context, returns a tuple of allowed tags, privileged tags, and if freeform is enabled
+#
+# first parameter is a bool if the user is 'privileged' (normally Sys.Modify on /)
+# second parameter is a closure which takes the vmid. should check if the user can see the vm tags
+sub get_allowed_tags {
+ my ($privileged_user, $can_see_vm_tags) = @_;
+
+ my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
+
+ my $allowed_tags = {};
+ my $privileged_tags = {};
+ if (my $tags = $dc->{'privileged-tags'}) {
+ $privileged_tags->{$_} = 1 for $tags->@*;
+ }
+ my $user_tag_privs = $dc->{'user-tag-access'} // {};
+ my $user_allow = $user_tag_privs->{'user-allow'} // 'free';
+ my $freeform = $user_allow eq 'free';
+
+ if ($user_allow ne 'none' || $privileged_user) {
+ $allowed_tags->{$_} = 1 for ($user_tag_privs->{'user-allow-list'} // [])->@*;
+ }
+
+ if ($user_allow eq 'free' || $user_allow eq 'existing' || $privileged_user) {
+ my $props = PVE::Cluster::get_guest_config_properties(['tags']);
+ for my $vmid (keys $props->%*) {
+ next if !$privileged_user && !$can_see_vm_tags->($vmid);
+ $allowed_tags->{$_} = 1 for PVE::Tools::split_list($props->{$vmid}->{tags});
+ }
+ }
+
+ if ($privileged_user) {
+ $allowed_tags->{$_} = 1 for keys $privileged_tags->%*;
+ } else {
+ delete $allowed_tags->{$_} for keys $privileged_tags->%*;
+ }
+
+ return wantarray ? ($allowed_tags, $privileged_tags, $freeform) : $allowed_tags;
+}
+
sub parse_datacenter_config {
my ($filename, $raw) = @_;
@@ -333,6 +413,19 @@ sub parse_datacenter_config {
$res->{'tag-style'} = parse_property_string($tag_style_format, $tag_style);
}
+ if (my $user_tag_privs = $res->{'user-tag-access'}) {
+ $res->{'user-tag-access'} =
+ parse_property_string($user_tag_privs_format, $user_tag_privs);
+
+ if (my $user_tags = $res->{'user-tag-access'}->{'user-allow-list'}) {
+ $res->{'user-tag-access'}->{'user-allow-list'} = [split(';', $user_tags)];
+ }
+ }
+
+ if (my $admin_tags = $res->{'privileged-tags'}) {
+ $res->{'privileged-tags'} = [split(';', $admin_tags)];
+ }
+
# for backwards compatibility only, new migration property has precedence
if (defined($res->{migration_unsecure})) {
if (defined($res->{migration}->{type})) {
@@ -396,6 +489,18 @@ sub write_datacenter_config {
$cfg->{'tag-style'} = PVE::JSONSchema::print_property_string($tag_style, $tag_style_format);
}
+ if (ref(my $user_tag_privs = $cfg->{'user-tag-access'})) {
+ if (my $user_tags = $user_tag_privs->{'user-allow-list'}) {
+ $user_tag_privs->{'user-allow-list'} = join(';', sort $user_tags->@*);
+ }
+ $cfg->{'user-tag-access'} =
+ PVE::JSONSchema::print_property_string($user_tag_privs, $user_tag_privs_format);
+ }
+
+ if (ref(my $admin_tags = $cfg->{'privileged-tags'})) {
+ $cfg->{'privileged-tags'} = join(';', sort $admin_tags->@*);
+ }
+
my $comment = '';
# add description as comment to top of file
my $description = $cfg->{description} || '';
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config Dominik Csapak
@ 2022-11-15 15:17 ` Fabian Grünbichler
2022-11-16 7:48 ` Thomas Lamprecht
2022-11-16 8:47 ` Dominik Csapak
0 siblings, 2 replies; 44+ messages in thread
From: Fabian Grünbichler @ 2022-11-15 15:17 UTC (permalink / raw)
To: Proxmox VE development discussion
On November 15, 2022 2:02 pm, Dominik Csapak wrote:
> by adding a 'user-tag-privileges' and 'admin-tags' option.
> The first sets the policy by which "normal" users (with
> 'VM.Config.Options' on the respective guest) can create/delete tags
> and the second is a list of tags only settable by 'admins'
> ('Sys.Modify' on '/')
>
> also add a helper 'get_allowed_tags' that returns the allowed (existing)
> tags, the privileged tags, and if a user can enter 'freeform' tags.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v9:
> * get_allowed_tags now takes a bool + closure for checking the tag
> access, prevents cyclic dependency between cluster and access-control
> * use 'for' instead of 'map'
> * fix indentation
> data/PVE/DataCenterConfig.pm | 105 +++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/data/PVE/DataCenterConfig.pm b/data/PVE/DataCenterConfig.pm
> index 532e5e5..f7f90e9 100644
> --- a/data/PVE/DataCenterConfig.pm
> +++ b/data/PVE/DataCenterConfig.pm
> @@ -154,6 +154,32 @@ my $tag_style_format = {
> },
> };
>
> +my $user_tag_privs_format = {
> + 'user-allow' => {
> + optional => 1,
> + type => 'string',
> + enum => ['none', 'list', 'existing', 'free'],
> + default => 'free',
> + description => "Controls tag usage for users without `Sys.Modify` on `/` bey either "
by
> + ."allowing `none`, a `list`, already `existing` or anything (`free`).",
> + verbose_description => "Controls which tags can be set or deleted on resources an user "
"a" user
> + ."controls (such as guests). Users iwth the `Sys.Modify` privilege on `/` are always "
with
> + ." unrestricted. "
> + ."'none' means no tags are modifiable. "
> + ."'list' allows tags from the given list. "
> + ."'existing' means only already existing tags of resources able to access or from the"
> + ."given list. "
this is hard to parse, maybe reword?
> + ."'free' means users can assign any tags."
this one is worded differently than the rest (maybe):
'none' no tags are modifiable.
'list' tags from 'user-allow-list' are modifiable.
'existing' like list, but already existing tags of resource are also modifiable.
'free' no tag restrictions.
> + },
> + 'user-allow-list' => {
> + optional => 1,
> + type => 'string',
> + pattern => "${PVE::JSONSchema::PVE_TAG_RE}(?:\;${PVE::JSONSchema::PVE_TAG_RE})*",
> + typetext => "<tag>[;<tag>...]",
> + description => "List of tags users are allowed to set and delete (semicolon separated).",
maybe add "for 'user-allow' values 'list' or 'existing'"?
> + },
> +};
> +
> my $datacenter_schema = {
> type => "object",
> additionalProperties => 0,
> @@ -285,12 +311,66 @@ my $datacenter_schema = {
> description => "Tag style options.",
> format => $tag_style_format,
> },
> + 'user-tag-access' => {
> + optional => 1,
> + type => 'string',
> + description => "Privilege options for user settable tags",
user-settable
> + format => $user_tag_privs_format,
> + },
> + 'privileged-tags' => {
> + optional => 1,
> + type => 'string',
> + description => "A list of tags that require a `Sys.Modify` on '/') to set and delete. "
> + ."Tags set here that are also in 'user-tag-access' also require `Sys.Modify`.",
> + pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
> + typetext => "<tag>[;<tag>...]",
stray 'a' and ')' in first sentence.
I am not sure the second sentence is necessary, or rather, wouldn't it be better
to make the two lists mutually exclusive? e.g., by removing privileged tags from
the other list?
> + },
> },
> };
>
> # make schema accessible from outside (for documentation)
> sub get_datacenter_schema { return $datacenter_schema };
>
> +# in scalar context, returns the list of allowed tags that exist
> +# in list context, returns a tuple of allowed tags, privileged tags, and if freeform is enabled
> +#
meh, this is a bit ugly, but okay
> +# first parameter is a bool if the user is 'privileged' (normally Sys.Modify on /)
> +# second parameter is a closure which takes the vmid. should check if the user can see the vm tags
> +sub get_allowed_tags {
> + my ($privileged_user, $can_see_vm_tags) = @_;
couldn't this live somewhere else instead of having this weird interface? e.g.,
guest-common? it's not needed at all for parsing or writing the config, which is
what lives here in this module/repo/package..
the other priv related helper for this already lives there as well..
> +
> + my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +
> + my $allowed_tags = {};
> + my $privileged_tags = {};
> + if (my $tags = $dc->{'privileged-tags'}) {
> + $privileged_tags->{$_} = 1 for $tags->@*;
> + }
> + my $user_tag_privs = $dc->{'user-tag-access'} // {};
> + my $user_allow = $user_tag_privs->{'user-allow'} // 'free';
> + my $freeform = $user_allow eq 'free';
> +
> + if ($user_allow ne 'none' || $privileged_user) {
> + $allowed_tags->{$_} = 1 for ($user_tag_privs->{'user-allow-list'} //
[])->@*;
> + }
> +
> + if ($user_allow eq 'free' || $user_allow eq 'existing' || $privileged_user) {
> + my $props = PVE::Cluster::get_guest_config_properties(['tags']);
> + for my $vmid (keys $props->%*) {
> + next if !$privileged_user && !$can_see_vm_tags->($vmid);
> + $allowed_tags->{$_} = 1 for PVE::Tools::split_list($props->{$vmid}->{tags});
> + }
> + }
> +
> + if ($privileged_user) {
> + $allowed_tags->{$_} = 1 for keys $privileged_tags->%*;
> + } else {
> + delete $allowed_tags->{$_} for keys $privileged_tags->%*;
> + }
> +
> + return wantarray ? ($allowed_tags, $privileged_tags, $freeform) : $allowed_tags;
> +}
> +
> sub parse_datacenter_config {
> my ($filename, $raw) = @_;
>
> @@ -333,6 +413,19 @@ sub parse_datacenter_config {
> $res->{'tag-style'} = parse_property_string($tag_style_format, $tag_style);
> }
>
> + if (my $user_tag_privs = $res->{'user-tag-access'}) {
> + $res->{'user-tag-access'} =
> + parse_property_string($user_tag_privs_format, $user_tag_privs);
> +
> + if (my $user_tags = $res->{'user-tag-access'}->{'user-allow-list'}) {
> + $res->{'user-tag-access'}->{'user-allow-list'} = [split(';',
$user_tags)];
could (warn and?) filter out privileged tags here)
> + }
> + }
> +
> + if (my $admin_tags = $res->{'privileged-tags'}) {
> + $res->{'privileged-tags'} = [split(';', $admin_tags)];
> + }
> +
> # for backwards compatibility only, new migration property has precedence
> if (defined($res->{migration_unsecure})) {
> if (defined($res->{migration}->{type})) {
> @@ -396,6 +489,18 @@ sub write_datacenter_config {
> $cfg->{'tag-style'} = PVE::JSONSchema::print_property_string($tag_style, $tag_style_format);
> }
>
> + if (ref(my $user_tag_privs = $cfg->{'user-tag-access'})) {
> + if (my $user_tags = $user_tag_privs->{'user-allow-list'}) {
> + $user_tag_privs->{'user-allow-list'} = join(';', sort $user_tags->@*);
same here
> + }
> + $cfg->{'user-tag-access'} =
> + PVE::JSONSchema::print_property_string($user_tag_privs, $user_tag_privs_format);
> + }
> +
> + if (ref(my $admin_tags = $cfg->{'privileged-tags'})) {
> + $cfg->{'privileged-tags'} = join(';', sort $admin_tags->@*);
> + }
> +
> my $comment = '';
> # add description as comment to top of file
> my $description = $cfg->{description} || '';
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-15 15:17 ` Fabian Grünbichler
@ 2022-11-16 7:48 ` Thomas Lamprecht
2022-11-16 8:47 ` Dominik Csapak
1 sibling, 0 replies; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 7:48 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
Am 15/11/2022 um 16:17 schrieb Fabian Grünbichler:
> this one is worded differently than the rest (maybe):
>
> 'none' no tags are modifiable.
> 'list' tags from 'user-allow-list' are modifiable.
> 'existing' like list, but already existing tags of resource are also modifiable.
> 'free' no tag restrictions.
also switch "modifiable" to "useable" (or settable, if that's a word)? As
modifying here implies the tags as object themselves, which could be confusing
if we really mean that those are the ones a user can set or remove to a virtual
guest.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-15 15:17 ` Fabian Grünbichler
2022-11-16 7:48 ` Thomas Lamprecht
@ 2022-11-16 8:47 ` Dominik Csapak
2022-11-16 8:51 ` Fabian Grünbichler
2022-11-16 8:54 ` Thomas Lamprecht
1 sibling, 2 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-16 8:47 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
most of the points are clear and ok for me, but
[snip]
>> + format => $user_tag_privs_format,
>> + },
>> + 'privileged-tags' => {
>> + optional => 1,
>> + type => 'string',
>> + description => "A list of tags that require a `Sys.Modify` on '/') to set and delete. "
>> + ."Tags set here that are also in 'user-tag-access' also require `Sys.Modify`.",
>> + pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
>> + typetext => "<tag>[;<tag>...]",
>
> stray 'a' and ')' in first sentence.
>
> I am not sure the second sentence is necessary, or rather, wouldn't it be better
> to make the two lists mutually exclusive? e.g., by removing privileged tags from
> the other list?
i don't really want to auto remove stuff from one option when set on another.
maybe it'd make more sense if we don't allow setting and admin tag when
it's already set in the 'user-allow-list' and vice versa? then
there cannot be a situation where a tag is in both lists at the same time?
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 8:47 ` Dominik Csapak
@ 2022-11-16 8:51 ` Fabian Grünbichler
2022-11-16 8:54 ` Thomas Lamprecht
1 sibling, 0 replies; 44+ messages in thread
From: Fabian Grünbichler @ 2022-11-16 8:51 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
On November 16, 2022 9:47 am, Dominik Csapak wrote:
> most of the points are clear and ok for me, but
> [snip]
>>> + format => $user_tag_privs_format,
>>> + },
>>> + 'privileged-tags' => {
>>> + optional => 1,
>>> + type => 'string',
>>> + description => "A list of tags that require a `Sys.Modify` on '/') to set and delete. "
>>> + ."Tags set here that are also in 'user-tag-access' also require `Sys.Modify`.",
>>> + pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
>>> + typetext => "<tag>[;<tag>...]",
>>
>> stray 'a' and ')' in first sentence.
>>
>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>> the other list?
>
> i don't really want to auto remove stuff from one option when set on another.
> maybe it'd make more sense if we don't allow setting and admin tag when
> it's already set in the 'user-allow-list' and vice versa? then
> there cannot be a situation where a tag is in both lists at the same time?
forbidding it on the API level (and maybe, to catch bugs, when writing the
config) is only part of it though - such duplicates would need to be filtered
out when parsing as well, else they can sneak in via a manual config file edit.
but yeah, that would work as well I think.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 8:47 ` Dominik Csapak
2022-11-16 8:51 ` Fabian Grünbichler
@ 2022-11-16 8:54 ` Thomas Lamprecht
2022-11-16 9:04 ` Dominik Csapak
1 sibling, 1 reply; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 8:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak,
Fabian Grünbichler
Am 16/11/2022 um 09:47 schrieb Dominik Csapak:
>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>> the other list?
>
> i don't really want to auto remove stuff from one option when set on another.
> maybe it'd make more sense if we don't allow setting and admin tag when
> it's already set in the 'user-allow-list' and vice versa? then
> there cannot be a situation where a tag is in both lists at the same time?
>
Limits use cases, as we'll only ever allow priv'd tags to be used for things
like backup job guest-source selection, and there may be scenarios where an
admin wants to allow the user to set a specific privileged tags in the VMs
they control.
To make that work we'd:
- explicitly allow such listed tags for "normal" VM users even if they're in the
privileged-tags (that's why I used the term "registered" in previous comments,
might be better suited if we deem that privileged is then confusing)
- highlight the fact if a tag is in both
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 8:54 ` Thomas Lamprecht
@ 2022-11-16 9:04 ` Dominik Csapak
2022-11-16 9:10 ` Thomas Lamprecht
0 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-16 9:04 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion,
Fabian Grünbichler
On 11/16/22 09:54, Thomas Lamprecht wrote:
> Am 16/11/2022 um 09:47 schrieb Dominik Csapak:
>>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>>> the other list?
>>
>> i don't really want to auto remove stuff from one option when set on another.
>> maybe it'd make more sense if we don't allow setting and admin tag when
>> it's already set in the 'user-allow-list' and vice versa? then
>> there cannot be a situation where a tag is in both lists at the same time?
>>
>
>
> Limits use cases, as we'll only ever allow priv'd tags to be used for things
> like backup job guest-source selection, and there may be scenarios where an
> admin wants to allow the user to set a specific privileged tags in the VMs
> they control.
>
> To make that work we'd:
> - explicitly allow such listed tags for "normal" VM users even if they're in the
> privileged-tags (that's why I used the term "registered" in previous comments,
> might be better suited if we deem that privileged is then confusing)
>
> - highlight the fact if a tag is in both
>
ok, then i have to change the permission checking code (currently i forbid
'normal' users the tag if it was in the 'privileged-tags' section, regardless
if it was in the 'user-allow-list' or not)
how would you highlight that? a warning on the cli/syslog/etc. is not
visible, but on the ui we don't really have an obvious place to do so
i could try to add a seperate 'warning' row in the object grid when
that happens, not sure if that's what you meant though
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 9:04 ` Dominik Csapak
@ 2022-11-16 9:10 ` Thomas Lamprecht
2022-11-16 9:31 ` Fabian Grünbichler
0 siblings, 1 reply; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 9:10 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak,
Fabian Grünbichler
Am 16/11/2022 um 10:04 schrieb Dominik Csapak:
> On 11/16/22 09:54, Thomas Lamprecht wrote:
>> Am 16/11/2022 um 09:47 schrieb Dominik Csapak:
>>>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>>>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>>>> the other list?
>>>
>>> i don't really want to auto remove stuff from one option when set on another.
>>> maybe it'd make more sense if we don't allow setting and admin tag when
>>> it's already set in the 'user-allow-list' and vice versa? then
>>> there cannot be a situation where a tag is in both lists at the same time?
>>>
>>
>>
>> Limits use cases, as we'll only ever allow priv'd tags to be used for things
>> like backup job guest-source selection, and there may be scenarios where an
>> admin wants to allow the user to set a specific privileged tags in the VMs
>> they control.
>>
>> To make that work we'd:
>> - explicitly allow such listed tags for "normal" VM users even if they're in the
>> privileged-tags (that's why I used the term "registered" in previous comments,
>> might be better suited if we deem that privileged is then confusing)
>>
>> - highlight the fact if a tag is in both
>>
>
> ok, then i have to change the permission checking code (currently i forbid
> 'normal' users the tag if it was in the 'privileged-tags' section, regardless
> if it was in the 'user-allow-list' or not)
maybe wait on Fabian's opinion on that, I don't want to push this to strongly
but can imagine that it might be sensible and useful, and hard to change later.
>
> how would you highlight that? a warning on the cli/syslog/etc. is not
> visible, but on the ui we don't really have an obvious place to do so
>
> i could try to add a seperate 'warning' row in the object grid when
> that happens, not sure if that's what you meant though
>
Syslog is never the place for such things, needs to happen on edit, and for
now there's no CLI so GUI is the only place we need to care about (edit cfgs
manually -> be on your own).
So a bottom section that shows a hints about the tags that are in both lists,
the hint would then be located in the edit windows for registered and allowed-list
of tags, so it doesn't necessarily needs to be inline (i.e., some highlight in
the existing tag edit).
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 9:10 ` Thomas Lamprecht
@ 2022-11-16 9:31 ` Fabian Grünbichler
2022-11-16 9:38 ` Dominik Csapak
2022-11-16 9:40 ` Thomas Lamprecht
0 siblings, 2 replies; 44+ messages in thread
From: Fabian Grünbichler @ 2022-11-16 9:31 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion, Thomas Lamprecht
On November 16, 2022 10:10 am, Thomas Lamprecht wrote:
> Am 16/11/2022 um 10:04 schrieb Dominik Csapak:
>> On 11/16/22 09:54, Thomas Lamprecht wrote:
>>> Am 16/11/2022 um 09:47 schrieb Dominik Csapak:
>>>>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>>>>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>>>>> the other list?
>>>>
>>>> i don't really want to auto remove stuff from one option when set on another.
>>>> maybe it'd make more sense if we don't allow setting and admin tag when
>>>> it's already set in the 'user-allow-list' and vice versa? then
>>>> there cannot be a situation where a tag is in both lists at the same time?
>>>>
>>>
>>>
>>> Limits use cases, as we'll only ever allow priv'd tags to be used for things
>>> like backup job guest-source selection, and there may be scenarios where an
>>> admin wants to allow the user to set a specific privileged tags in the VMs
>>> they control.
>>>
>>> To make that work we'd:
>>> - explicitly allow such listed tags for "normal" VM users even if they're in the
>>> privileged-tags (that's why I used the term "registered" in previous comments,
>>> might be better suited if we deem that privileged is then confusing)
>>>
>>> - highlight the fact if a tag is in both
>>>
>>
>> ok, then i have to change the permission checking code (currently i forbid
>> 'normal' users the tag if it was in the 'privileged-tags' section, regardless
>> if it was in the 'user-allow-list' or not)
>
> maybe wait on Fabian's opinion on that, I don't want to push this to strongly
> but can imagine that it might be sensible and useful, and hard to change later.
If we say vzdump should only use privileged tags for backup inclusion logic (to
avoid unprivileged users adding that tag to their VM and causing it to be backed
up), but then make some of those tags effectively non-privileged (which allows
exactly that), why do we have the restriction in vzdump in the first place?
that sounds like a complicated way (with lots of side-effects, because
privileged tags might be used in other places in the future as well) to make the
"vzdump should only use privileged tags" part configurable.. maybe there should
simply be a list of "vzdump tags" in addition to the privileged ones? those
would then be unprivileged, but the scope of "these allow vzdump job inclusion"
is clear and limited. or we just keep "vzdump only looks at privileged tags" for
now to keep it simple - extending that one way or another in the future is
always possible if it is restricted now, the other way is harder ;)
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 9:31 ` Fabian Grünbichler
@ 2022-11-16 9:38 ` Dominik Csapak
2022-11-16 9:40 ` Thomas Lamprecht
1 sibling, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-16 9:38 UTC (permalink / raw)
To: Fabian Grünbichler, Proxmox VE development discussion,
Thomas Lamprecht
On 11/16/22 10:31, Fabian Grünbichler wrote:
> On November 16, 2022 10:10 am, Thomas Lamprecht wrote:
>> Am 16/11/2022 um 10:04 schrieb Dominik Csapak:
>>> On 11/16/22 09:54, Thomas Lamprecht wrote:
>>>> Am 16/11/2022 um 09:47 schrieb Dominik Csapak:
>>>>>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>>>>>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>>>>>> the other list?
>>>>>
>>>>> i don't really want to auto remove stuff from one option when set on another.
>>>>> maybe it'd make more sense if we don't allow setting and admin tag when
>>>>> it's already set in the 'user-allow-list' and vice versa? then
>>>>> there cannot be a situation where a tag is in both lists at the same time?
>>>>>
>>>>
>>>>
>>>> Limits use cases, as we'll only ever allow priv'd tags to be used for things
>>>> like backup job guest-source selection, and there may be scenarios where an
>>>> admin wants to allow the user to set a specific privileged tags in the VMs
>>>> they control.
>>>>
>>>> To make that work we'd:
>>>> - explicitly allow such listed tags for "normal" VM users even if they're in the
>>>> privileged-tags (that's why I used the term "registered" in previous comments,
>>>> might be better suited if we deem that privileged is then confusing)
>>>>
>>>> - highlight the fact if a tag is in both
>>>>
>>>
>>> ok, then i have to change the permission checking code (currently i forbid
>>> 'normal' users the tag if it was in the 'privileged-tags' section, regardless
>>> if it was in the 'user-allow-list' or not)
>>
>> maybe wait on Fabian's opinion on that, I don't want to push this to strongly
>> but can imagine that it might be sensible and useful, and hard to change later.
>
> If we say vzdump should only use privileged tags for backup inclusion logic (to
> avoid unprivileged users adding that tag to their VM and causing it to be backed
> up), but then make some of those tags effectively non-privileged (which allows
> exactly that), why do we have the restriction in vzdump in the first place?
>
> that sounds like a complicated way (with lots of side-effects, because
> privileged tags might be used in other places in the future as well) to make the
> "vzdump should only use privileged tags" part configurable.. maybe there should
> simply be a list of "vzdump tags" in addition to the privileged ones? those
> would then be unprivileged, but the scope of "these allow vzdump job inclusion"
> is clear and limited. or we just keep "vzdump only looks at privileged tags" for
> now to keep it simple - extending that one way or another in the future is
> always possible if it is restricted now, the other way is harder ;)
i think the reasoning of thomas here is that when we allow setting it in both
it's up to the admin how to interpret that for example:
we have x places where we only use privileged tags (e.g vzdump, migration, ...)
but now the admin wants to allow the other admins to add the tags
necessary for migration, but not for the others
then we'd have to add a seperate list for each occurance of privileged tags,
or we do it like thomas described then the admin simply sets in the
'user-allow-list' too
does that make sense?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 9:31 ` Fabian Grünbichler
2022-11-16 9:38 ` Dominik Csapak
@ 2022-11-16 9:40 ` Thomas Lamprecht
2022-11-16 9:51 ` Fabian Grünbichler
1 sibling, 1 reply; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 9:40 UTC (permalink / raw)
To: Fabian Grünbichler, Dominik Csapak,
Proxmox VE development discussion
Am 16/11/2022 um 10:31 schrieb Fabian Grünbichler:
>>> ok, then i have to change the permission checking code (currently i forbid
>>> 'normal' users the tag if it was in the 'privileged-tags' section, regardless
>>> if it was in the 'user-allow-list' or not)
>> maybe wait on Fabian's opinion on that, I don't want to push this to strongly
>> but can imagine that it might be sensible and useful, and hard to change later.
> If we say vzdump should only use privileged tags for backup inclusion logic (to
> avoid unprivileged users adding that tag to their VM and causing it to be backed
> up), but then make some of those tags effectively non-privileged (which allows
> exactly that), why do we have the restriction in vzdump in the first place?
maybe re-read my scenario, feels like you're missing a bit here, maybe name it
"registered-tags" as suggested to make the confusion go away.
>
> that sounds like a complicated way (with lots of side-effects, because
it's very simple?
> privileged tags might be used in other places in the future as well) to make the
> "vzdump should only use privileged tags" part configurable.. maybe there should
> simply be a list of "vzdump tags" in addition to the privileged ones? those
> would then be unprivileged, but the scope of "these allow vzdump job inclusion"
> is clear and limited. or we just keep "vzdump only looks at privileged tags" for
> now to keep it simple - extending that one way or another in the future is
> always possible if it is restricted now, the other way is harder 😉
not sure where you get complicated?
- You have a list of tags that are useable for backup source
- You have a mode where you can say that a list of tags that "normal VM admins" can use
- If they intersect then a "normal VM admin" can use it too
If you want to give a user control of what a (admin controlled!) job includes in
terms of guests then you can do so easily by also allowing the registered tag, if
not then don't? Note that not all setups host externally mostly untrusted guests/
users, the bigger market for us is those where a admin has a trust basis and also
no problem in giving control
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 9:40 ` Thomas Lamprecht
@ 2022-11-16 9:51 ` Fabian Grünbichler
2022-11-16 13:56 ` Thomas Lamprecht
0 siblings, 1 reply; 44+ messages in thread
From: Fabian Grünbichler @ 2022-11-16 9:51 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion, Thomas Lamprecht
On November 16, 2022 10:40 am, Thomas Lamprecht wrote:
> Am 16/11/2022 um 10:31 schrieb Fabian Grünbichler:
>>>> ok, then i have to change the permission checking code (currently i forbid
>>>> 'normal' users the tag if it was in the 'privileged-tags' section, regardless
>>>> if it was in the 'user-allow-list' or not)
>>> maybe wait on Fabian's opinion on that, I don't want to push this to strongly
>>> but can imagine that it might be sensible and useful, and hard to change later.
>> If we say vzdump should only use privileged tags for backup inclusion logic (to
>> avoid unprivileged users adding that tag to their VM and causing it to be backed
>> up), but then make some of those tags effectively non-privileged (which allows
>> exactly that), why do we have the restriction in vzdump in the first place?
>
> maybe re-read my scenario, feels like you're missing a bit here, maybe name it
> "registered-tags" as suggested to make the confusion go away.
>
>>
>> that sounds like a complicated way (with lots of side-effects, because
>
> it's very simple?
>
>> privileged tags might be used in other places in the future as well) to make the
>> "vzdump should only use privileged tags" part configurable.. maybe there should
>> simply be a list of "vzdump tags" in addition to the privileged ones? those
>> would then be unprivileged, but the scope of "these allow vzdump job inclusion"
>> is clear and limited. or we just keep "vzdump only looks at privileged tags" for
>> now to keep it simple - extending that one way or another in the future is
>> always possible if it is restricted now, the other way is harder 😉
>
> not sure where you get complicated?
>
> - You have a list of tags that are useable for backup source
>
the problem is that this list of tags might also be used for other things than
backup source (either by PVE, or by custom tooling) where the difference between
(who can set a) "regular tag" and (a) "privileged/registered/.. tag" matters.
> - You have a mode where you can say that a list of tags that "normal VM admins" can use
>
> - If they intersect then a "normal VM admin" can use it too
>
> If you want to give a user control of what a (admin controlled!) job includes in
> terms of guests then you can do so easily by also allowing the registered tag, if
> not then don't? Note that not all setups host externally mostly untrusted guests/
> users, the bigger market for us is those where a admin has a trust basis and also
> no problem in giving control
I understand the issue/scenario, but I think the missing scope restricts us down
the line when we want to start using the difference between "registered"
(restricted?) tags and normal ones for other things besides vzdump - because the
admin when lifting the restriction might not be aware of the implication that
this doesn't just affect vzdump jobs (for example, because the other feature
that also uses the list of special tags is not even implemented yet at that
point). if we don't care about that then sure, we can just have two lists and
allow tags being in both, but it should come with a warning about the
implications ;)
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
2022-11-16 9:51 ` Fabian Grünbichler
@ 2022-11-16 13:56 ` Thomas Lamprecht
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 13:56 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler,
Dominik Csapak
Am 16/11/2022 um 10:51 schrieb Fabian Grünbichler:
>> - You have a list of tags that are useable for backup source
>>
> the problem is that this list of tags might also be used for other things than
> backup source (either by PVE, or by custom tooling) where the difference between
> (who can set a) "regular tag" and (a) "privileged/registered/.. tag" matters.
>
>> - You have a mode where you can say that a list of tags that "normal VM admins" can use
>>
>> - If they intersect then a "normal VM admin" can use it too
>>
>> If you want to give a user control of what a (admin controlled!) job includes in
>> terms of guests then you can do so easily by also allowing the registered tag, if
>> not then don't? Note that not all setups host externally mostly untrusted guests/
>> users, the bigger market for us is those where a admin has a trust basis and also
>> no problem in giving control
> I understand the issue/scenario, but I think the missing scope restricts us down
> the line when we want to start using the difference between "registered"
> (restricted?) tags and normal ones for other things besides vzdump - because the
> admin when lifting the restriction might not be aware of the implication that
> this doesn't just affect vzdump jobs (for example, because the other feature
> that also uses the list of special tags is not even implemented yet at that
> point). if we don't care about that then sure, we can just have two lists and
> allow tags being in both, but it should come with a warning about the
> implications 😉
meh, that is somewhat true for a lot of feature expansions, but yes, we don't have
any such feature (as we need tags to work first -> chicken/egg), and deciding that
stuff now for the future is probably always missing some things; so I recommended
Dominik off-list to go for the "require / Sys.Modify" approach for now and ideally
add a frontend warning/hint if there are shared ones between the registered/privileged
and user allow-list. We can then expand on this when actually implementing features
and getting user feedback, that is def. safer for now.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH cluster v10 5/5] datacenter.cfg: add 'ordering' to 'tag-style' config
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (3 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH guest-common v10 1/1] GuestHelpers: add 'assert_tag_permissions' Dominik Csapak
` (17 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
such that the admin can decide if the tags should be sorted in the gui
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v10
data/PVE/DataCenterConfig.pm | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/data/PVE/DataCenterConfig.pm b/data/PVE/DataCenterConfig.pm
index ae138f9..658cfd9 100644
--- a/data/PVE/DataCenterConfig.pm
+++ b/data/PVE/DataCenterConfig.pm
@@ -152,6 +152,13 @@ my $tag_style_format = {
typetext => '<tag>:<hex-color>[:<hex-color-for-text>][;<tag>=...]',
description => "Manual color mapping for tags (semicolon separated).",
},
+ ordering => {
+ optional => 1,
+ type => 'string',
+ enum => ['config', 'alphabetical'],
+ default => 'config',
+ description => 'Controls the sorting of the tags in the web ui.',
+ }
};
my $user_tag_privs_format = {
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH guest-common v10 1/1] GuestHelpers: add 'assert_tag_permissions'
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (4 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 5/5] datacenter.cfg: add 'ordering' to 'tag-style' config Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 15:34 ` Fabian Grünbichler
2022-11-15 13:02 ` [pve-devel] [PATCH qemu-server v10 1/1] api: update: check for tags permissions with 'assert_tag_permissions' Dominik Csapak
` (16 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
helper to check permissions for tag setting/updating/deleting
for both container and qemu-server
gets the list of allowed tags from the DataCenterConfig and the current
user permissions, and checks for each tag that is added/removed if
the user has permissions to modify it
'normal' tags require 'VM.Config.Options' on '/vms/<vmid>', but not
allowed tags (either limited with 'user-tag-access' or
'privileged-tags' in the datacenter.cfg) requrie 'Sys.Modify' on '/'
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v10, but the code is mostly the same as the previous qemu-server
container one, with a few changes:
* adapt to new get_allowed_tags signature
* change 'map {foo} bar' into 'foo for bar'
* save all tags into $all_tags so that we only have to loop once
* use raise_perm_exc instead of die for permission errors (sets the
correct http status code)
debian/control | 3 ++-
src/PVE/GuestHelpers.pm | 54 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/debian/control b/debian/control
index 83bade3..ffff22b 100644
--- a/debian/control
+++ b/debian/control
@@ -12,7 +12,8 @@ Homepage: https://www.proxmox.com
Package: libpve-guest-common-perl
Architecture: all
-Depends: libpve-cluster-perl,
+Depends: libpve-access-control,
+ libpve-cluster-perl,
libpve-common-perl (>= 7.2-6),
libpve-storage-perl (>= 7.0-14),
pve-cluster,
diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
index 0fe3fd6..2742501 100644
--- a/src/PVE/GuestHelpers.pm
+++ b/src/PVE/GuestHelpers.pm
@@ -3,6 +3,7 @@ package PVE::GuestHelpers;
use strict;
use warnings;
+use PVE::Exception qw(raise_perm_exc);
use PVE::Tools;
use PVE::Storage;
@@ -11,7 +12,7 @@ use Scalar::Util qw(weaken);
use base qw(Exporter);
-our @EXPORT_OK = qw(safe_string_ne safe_boolean_ne safe_num_ne typesafe_ne);
+our @EXPORT_OK = qw(safe_string_ne safe_boolean_ne safe_num_ne typesafe_ne assert_tag_permissions);
# We use a separate lock to block migration while a replication job
# is running.
@@ -246,4 +247,55 @@ sub config_with_pending_array {
return $res;
}
+# checks the permissions for setting/updating/removing tags for guests
+# tagopt_old and tagopt_new expect the tags as they are in the config
+#
+# either returns gracefully or raises a permission exception
+sub assert_tag_permissions {
+ my ($vmid, $tagopt_old, $tagopt_new, $rpcenv, $authuser) = @_;
+
+ my $allowed_tags;
+ my $privileged_tags;
+ my $freeform;
+ my $privileged_user = $rpcenv->check($authuser, '/', ['Sys.Modify'], 1);
+ my $has_config_options =
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Options'], 0, 1);
+
+ my $check_single_tag = sub {
+ my ($tag) = @_;
+ return if $privileged_user;
+
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Options'])
+ if !$has_config_options;
+
+ if (!defined($allowed_tags) && !defined($privileged_tags) && !defined($freeform)) {
+ ($allowed_tags, $privileged_tags, $freeform) = PVE::DataCenterConfig::get_allowed_tags(
+ $privileged_user,
+ sub {
+ my ($vmid_to_check) = @_;
+ return $rpcenv->check_vm_perm($authuser, $vmid_to_check, undef, ['VM.Audit'], 0, 1);
+ },
+ );
+ }
+
+ if ((!$allowed_tags->{$tag} && !$freeform) || $privileged_tags->{$tag}) {
+ raise_perm_exc("/, Sys.Modify for modifying tag '$tag'");
+ }
+
+ return;
+ };
+
+ my $old_tags = {};
+ my $new_tags = {};
+ my $all_tags = {};
+
+ $all_tags->{$_} = $old_tags->{$_} += 1 for PVE::Tools::split_list($tagopt_old // '');
+ $all_tags->{$_} = $new_tags->{$_} += 1 for PVE::Tools::split_list($tagopt_new // '');
+
+ for my $tag (keys $all_tags->%*) {
+ next if ($new_tags->{$tag} // 0) == ($old_tags->{$tag} // 0);
+ $check_single_tag->($tag);
+ }
+}
+
1;
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH guest-common v10 1/1] GuestHelpers: add 'assert_tag_permissions'
2022-11-15 13:02 ` [pve-devel] [PATCH guest-common v10 1/1] GuestHelpers: add 'assert_tag_permissions' Dominik Csapak
@ 2022-11-15 15:34 ` Fabian Grünbichler
0 siblings, 0 replies; 44+ messages in thread
From: Fabian Grünbichler @ 2022-11-15 15:34 UTC (permalink / raw)
To: Proxmox VE development discussion
On November 15, 2022 2:02 pm, Dominik Csapak wrote:
> helper to check permissions for tag setting/updating/deleting
> for both container and qemu-server
>
> gets the list of allowed tags from the DataCenterConfig and the current
> user permissions, and checks for each tag that is added/removed if
> the user has permissions to modify it
>
> 'normal' tags require 'VM.Config.Options' on '/vms/<vmid>', but not
> allowed tags (either limited with 'user-tag-access' or
> 'privileged-tags' in the datacenter.cfg) requrie 'Sys.Modify' on '/'
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> new in v10, but the code is mostly the same as the previous qemu-server
> container one, with a few changes:
> * adapt to new get_allowed_tags signature
> * change 'map {foo} bar' into 'foo for bar'
> * save all tags into $all_tags so that we only have to loop once
> * use raise_perm_exc instead of die for permission errors (sets the
> correct http status code)
> debian/control | 3 ++-
> src/PVE/GuestHelpers.pm | 54 ++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/debian/control b/debian/control
> index 83bade3..ffff22b 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -12,7 +12,8 @@ Homepage: https://www.proxmox.com
>
> Package: libpve-guest-common-perl
> Architecture: all
> -Depends: libpve-cluster-perl,
> +Depends: libpve-access-control,
> + libpve-cluster-perl,
> libpve-common-perl (>= 7.2-6),
> libpve-storage-perl (>= 7.0-14),
> pve-cluster,
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
> index 0fe3fd6..2742501 100644
> --- a/src/PVE/GuestHelpers.pm
> +++ b/src/PVE/GuestHelpers.pm
> @@ -3,6 +3,7 @@ package PVE::GuestHelpers;
> use strict;
> use warnings;
>
> +use PVE::Exception qw(raise_perm_exc);
> use PVE::Tools;
> use PVE::Storage;
>
> @@ -11,7 +12,7 @@ use Scalar::Util qw(weaken);
>
> use base qw(Exporter);
>
> -our @EXPORT_OK = qw(safe_string_ne safe_boolean_ne safe_num_ne typesafe_ne);
> +our @EXPORT_OK = qw(safe_string_ne safe_boolean_ne safe_num_ne typesafe_ne assert_tag_permissions);
>
> # We use a separate lock to block migration while a replication job
> # is running.
> @@ -246,4 +247,55 @@ sub config_with_pending_array {
> return $res;
> }
>
> +# checks the permissions for setting/updating/removing tags for guests
> +# tagopt_old and tagopt_new expect the tags as they are in the config
> +#
> +# either returns gracefully or raises a permission exception
> +sub assert_tag_permissions {
> + my ($vmid, $tagopt_old, $tagopt_new, $rpcenv, $authuser) = @_;
> +
> + my $allowed_tags;
> + my $privileged_tags;
> + my $freeform;
> + my $privileged_user = $rpcenv->check($authuser, '/', ['Sys.Modify'], 1);
> + my $has_config_options =
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Options'], 0, 1);
> +
> + my $check_single_tag = sub {
> + my ($tag) = @_;
> + return if $privileged_user;
> +
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Options'])
> + if !$has_config_options;
> +
nit: this confused me - but it's basically to only check once and fail if a tag has
changed? would be more readable by just recording whether it was checked already
and skipping based on that bool IMHO, instead of skipping based on the result of
a noerr redundant check..
> + if (!defined($allowed_tags) && !defined($privileged_tags) && !defined($freeform)) {
> + ($allowed_tags, $privileged_tags, $freeform) = PVE::DataCenterConfig::get_allowed_tags(
> + $privileged_user,
> + sub {
> + my ($vmid_to_check) = @_;
> + return $rpcenv->check_vm_perm($authuser, $vmid_to_check, undef, ['VM.Audit'], 0, 1);
> + },
> + );
see response to "get_allowed_tags" patch ;)
> + }
> +
> + if ((!$allowed_tags->{$tag} && !$freeform) || $privileged_tags->{$tag}) {
> + raise_perm_exc("/, Sys.Modify for modifying tag '$tag'");
> + }
> +
> + return;
> + };
> +
> + my $old_tags = {};
> + my $new_tags = {};
> + my $all_tags = {};
> +
> + $all_tags->{$_} = $old_tags->{$_} += 1 for PVE::Tools::split_list($tagopt_old // '');
> + $all_tags->{$_} = $new_tags->{$_} += 1 for PVE::Tools::split_list($tagopt_new // '');
> +
> + for my $tag (keys $all_tags->%*) {
> + next if ($new_tags->{$tag} // 0) == ($old_tags->{$tag} // 0);
> + $check_single_tag->($tag);
> + }
> +}
> +
> 1;
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH qemu-server v10 1/1] api: update: check for tags permissions with 'assert_tag_permissions'
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (5 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH guest-common v10 1/1] GuestHelpers: add 'assert_tag_permissions' Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH container v10 1/1] check_ct_modify_config_perm: " Dominik Csapak
` (15 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
from GuestHelpers. This function checks all necessary permissions and
raises an exception if the user does not have the correct ones.
This is necessary for the new 'privileged' tags and 'user-tag-access'
permissions to work.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* use GuestHelpers::assert_tag_permissions
PVE/API2/Qemu.pm | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 30348e6..848a1bc 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -18,7 +18,7 @@ use PVE::Storage;
use PVE::JSONSchema qw(get_standard_option);
use PVE::RESTHandler;
use PVE::ReplicationConfig;
-use PVE::GuestHelpers;
+use PVE::GuestHelpers qw(assert_tag_permissions);
use PVE::QemuConfig;
use PVE::QemuServer;
use PVE::QemuServer::Cloudinit;
@@ -539,7 +539,6 @@ my $generaloptions = {
'startup' => 1,
'tdf' => 1,
'template' => 1,
- 'tags' => 1,
};
my $vmpoweroptions = {
@@ -609,6 +608,7 @@ my $check_vm_modify_config_perm = sub {
next if PVE::QemuServer::is_valid_drivename($opt);
next if $opt eq 'cdrom';
next if $opt =~ m/^(?:unused|serial|usb)\d+$/;
+ next if $opt eq 'tags';
if ($cpuoptions->{$opt} || $opt =~ m/^numa\d+$/) {
@@ -1689,6 +1689,10 @@ my $update_vm_api = sub {
}
PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt eq 'tags') {
+ assert_tag_permissions($vmid, $val, '', $rpcenv, $authuser);
+ delete $conf->{$opt};
+ PVE::QemuConfig->write_config($vmid, $conf);
} else {
PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
PVE::QemuConfig->write_config($vmid, $conf);
@@ -1749,6 +1753,9 @@ my $update_vm_api = sub {
die "only root can modify '$opt' config for real devices\n";
}
$conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt eq 'tags') {
+ assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
+ $conf->{pending}->{$opt} = $param->{$opt};
} else {
$conf->{pending}->{$opt} = $param->{$opt};
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH container v10 1/1] check_ct_modify_config_perm: check for tags permissions with 'assert_tag_permissions'
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (6 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH qemu-server v10 1/1] api: update: check for tags permissions with 'assert_tag_permissions' Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 1/2] add tag related helpers Dominik Csapak
` (14 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
from GuestHelpers. This function checks all necessary permissions and
raises an exception if the user does not have the correct ones.
This is necessary for the new 'privileged' tags and 'user-tag-access'
permissions to work.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* use GuestHelpers::assert_tag_permissions
src/PVE/LXC.pm | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 4bbd739..3cedc9a 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1336,6 +1336,10 @@ sub check_ct_modify_config_perm {
} elsif ($opt eq 'hookscript') {
# For now this is restricted to root@pam
raise_perm_exc("changing the hookscript is only allowed for root\@pam");
+ } elsif ($opt eq 'tags') {
+ my $old = $oldconf->{$opt};
+ my $new = $delete ? '' : $newconf->{$opt};
+ PVE::GuestHelpers::assert_tag_permissions($vmid, $old, $new, $rpcenv, $authuser);
} else {
$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
}
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH widget-toolkit v10 1/2] add tag related helpers
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (7 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH container v10 1/1] check_ct_modify_config_perm: " Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-16 13:48 ` [pve-devel] applied: " Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 2/2] Toolkit: add override for Ext.dd.DragDropManager Dominik Csapak
` (13 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
helpers to
* generate a color from a string consistently
* generate a html tag for a tag
* related css classes
contrast is calculated according to SAPC draft:
https://github.com/Myndex/SAPC-APCA
which is likely to become a w3c guideline in the future and seems
to be a better algorithm for this
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* tags are now 'inline-block' and for boundlists have a line-height
of 15px (fixes an overflow issue)
src/Utils.js | 88 ++++++++++++++++++++++++++++++++++++++++++++
src/css/ext6-pmx.css | 52 ++++++++++++++++++++++++++
2 files changed, 140 insertions(+)
diff --git a/src/Utils.js b/src/Utils.js
index e9b84f8..7255e3f 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -1272,6 +1272,94 @@ utilities: {
.map(val => val.charCodeAt(0)),
);
},
+
+ stringToRGB: function(string) {
+ let hash = 0;
+ if (!string) {
+ return hash;
+ }
+ string += 'prox'; // give short strings more variance
+ for (let i = 0; i < string.length; i++) {
+ hash = string.charCodeAt(i) + ((hash << 5) - hash);
+ hash = hash & hash; // to int
+ }
+
+ let alpha = 0.7; // make the color a bit brighter
+ let bg = 255; // assume white background
+
+ return [
+ (hash & 255) * alpha + bg * (1 - alpha),
+ ((hash >> 8) & 255) * alpha + bg * (1 - alpha),
+ ((hash >> 16) & 255) * alpha + bg * (1 - alpha),
+ ];
+ },
+
+ rgbToCss: function(rgb) {
+ return `rgb(${rgb[0]}, ${rgb[1]}, ${rgb[2]})`;
+ },
+
+ rgbToHex: function(rgb) {
+ let r = Math.round(rgb[0]).toString(16);
+ let g = Math.round(rgb[1]).toString(16);
+ let b = Math.round(rgb[2]).toString(16);
+ return `${r}${g}${b}`;
+ },
+
+ hexToRGB: function(hex) {
+ if (!hex) {
+ return undefined;
+ }
+ if (hex.length === 7) {
+ hex = hex.slice(1);
+ }
+ let r = parseInt(hex.slice(0, 2), 16);
+ let g = parseInt(hex.slice(2, 4), 16);
+ let b = parseInt(hex.slice(4, 6), 16);
+ return [r, g, b];
+ },
+
+ // optimized & simplified SAPC function
+ // https://github.com/Myndex/SAPC-APCA
+ getTextContrastClass: function(rgb) {
+ const blkThrs = 0.022;
+ const blkClmp = 1.414;
+
+ // linearize & gamma correction
+ let r = (rgb[0] / 255) ** 2.4;
+ let g = (rgb[1] / 255) ** 2.4;
+ let b = (rgb[2] / 255) ** 2.4;
+
+ // relative luminance sRGB
+ let bg = r * 0.2126729 + g * 0.7151522 + b * 0.0721750;
+
+ // black clamp
+ bg = bg > blkThrs ? bg : bg + (blkThrs - bg) ** blkClmp;
+
+ // SAPC with white text
+ let contrastLight = bg ** 0.65 - 1;
+ // SAPC with black text
+ let contrastDark = bg ** 0.56 - 0.046134502;
+
+ if (Math.abs(contrastLight) >= Math.abs(contrastDark)) {
+ return 'light';
+ } else {
+ return 'dark';
+ }
+ },
+
+ getTagElement: function(string, color_overrides) {
+ let rgb = color_overrides?.[string] || Proxmox.Utils.stringToRGB(string);
+ let style = `background-color: ${Proxmox.Utils.rgbToCss(rgb)};`;
+ let cls;
+ if (rgb.length > 3) {
+ style += `color: ${Proxmox.Utils.rgbToCss([rgb[3], rgb[4], rgb[5]])}`;
+ cls = "proxmox-tag-dark";
+ } else {
+ let txtCls = Proxmox.Utils.getTextContrastClass(rgb);
+ cls = `proxmox-tag-${txtCls}`;
+ }
+ return `<span class="${cls}" style="${style}">${string}</span>`;
+ },
},
singleton: true,
diff --git a/src/css/ext6-pmx.css b/src/css/ext6-pmx.css
index 231b4ce..37cadde 100644
--- a/src/css/ext6-pmx.css
+++ b/src/css/ext6-pmx.css
@@ -6,6 +6,58 @@
background-color: LightYellow;
}
+.proxmox-tags-full .proxmox-tag-light,
+.proxmox-tags-full .proxmox-tag-dark {
+ border-radius: 3px;
+ padding: 1px 6px;
+ margin: 0px 1px;
+ display: inline-block;
+}
+
+.x-boundlist-item > .proxmox-tag-light,
+.x-boundlist-item > .proxmox-tag-dark {
+ line-height: 15px;
+}
+
+
+.proxmox-tags-circle .proxmox-tag-light,
+.proxmox-tags-circle .proxmox-tag-dark {
+ margin: 0px 1px;
+ position: relative;
+ top: 2px;
+ border-radius: 6px;
+ height: 12px;
+ width: 12px;
+ display: inline-block;
+ color: transparent !important;
+ overflow: hidden;
+}
+
+.proxmox-tags-none .proxmox-tag-light,
+.proxmox-tags-none .proxmox-tag-dark {
+ display: none;
+}
+
+.proxmox-tags-dense .proxmox-tag-light,
+.proxmox-tags-dense .proxmox-tag-dark {
+ width: 6px;
+ margin-right: 1px;
+ display: inline-block;
+ color: transparent !important;
+ overflow: hidden;
+ vertical-align: bottom;
+}
+
+.proxmox-tags-full .proxmox-tag-light {
+ color: #fff;
+ background-color: #383838;
+}
+
+.proxmox-tags-full .proxmox-tag-dark {
+ color: #000;
+ background-color: #f0f0f0;
+}
+
.x-mask-msg-text {
text-align: center;
}
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] applied: [PATCH widget-toolkit v10 1/2] add tag related helpers
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 1/2] add tag related helpers Dominik Csapak
@ 2022-11-16 13:48 ` Thomas Lamprecht
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 13:48 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 15/11/2022 um 14:02 schrieb Dominik Csapak:
> helpers to
> * generate a color from a string consistently
> * generate a html tag for a tag
> * related css classes
>
> contrast is calculated according to SAPC draft:
> https://github.com/Myndex/SAPC-APCA
>
> which is likely to become a w3c guideline in the future and seems
> to be a better algorithm for this
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v9:
> * tags are now 'inline-block' and for boundlists have a line-height
> of 15px (fixes an overflow issue)
> src/Utils.js | 88 ++++++++++++++++++++++++++++++++++++++++++++
> src/css/ext6-pmx.css | 52 ++++++++++++++++++++++++++
> 2 files changed, 140 insertions(+)
>
>
applied, thanks!
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH widget-toolkit v10 2/2] Toolkit: add override for Ext.dd.DragDropManager
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (8 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 1/2] add tag related helpers Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-16 13:49 ` [pve-devel] applied: " Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 01/13] api: /cluster/resources: add tags to returned properties Dominik Csapak
` (12 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
to fix selection behavior for Ext.dd.DragZone.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
src/Toolkit.js | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/Toolkit.js b/src/Toolkit.js
index 4381f46..2f4a01a 100644
--- a/src/Toolkit.js
+++ b/src/Toolkit.js
@@ -686,6 +686,22 @@ Ext.define('Proxmox.view.DragZone', {
},
});
+// Fix text selection on drag when using DragZone,
+// see https://forum.sencha.com/forum/showthread.php?335100
+Ext.define('Proxmox.dd.DragDropManager', {
+ override: 'Ext.dd.DragDropManager',
+
+ stopEvent: function(e) {
+ if (this.stopPropagation) {
+ e.stopPropagation();
+ }
+
+ if (this.preventDefault) {
+ e.preventDefault();
+ }
+ },
+});
+
// force alert boxes to be rendered with an Error Icon
// since Ext.Msg is an object and not a prototype, we need to override it
// after the framework has been initiated
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 01/13] api: /cluster/resources: add tags to returned properties
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (9 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 2/2] Toolkit: add override for Ext.dd.DragDropManager Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-16 8:02 ` Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 02/13] api: add /ui-options api call Dominik Csapak
` (11 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
by querying 'lock' and 'tags' with 'get_guest_config_properties'
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2/Cluster.pm | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 49e319a5c..34a6e08cd 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -371,7 +371,8 @@ __PACKAGE__->register_method({
# we try to generate 'numbers' by using "$X + 0"
if (!$param->{type} || $param->{type} eq 'vm') {
- my $locked_vms = PVE::Cluster::get_guest_config_property('lock');
+ my $prop_list = [qw(lock tags)];
+ my $props = PVE::Cluster::get_guest_config_properties($prop_list);
for my $vmid (sort keys %$idlist) {
@@ -403,8 +404,10 @@ __PACKAGE__->register_method({
# only skip now to next to ensure that the pool stats above are filled, if eligible
next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
- if (defined(my $lock = $locked_vms->{$vmid}->{lock})) {
- $entry->{lock} = $lock;
+ for my $prop (@$prop_list) {
+ if (defined(my $value = $props->{$vmid}->{$prop})) {
+ $entry->{$prop} = $value;
+ }
}
if (defined($entry->{pool}) &&
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH manager v10 01/13] api: /cluster/resources: add tags to returned properties
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 01/13] api: /cluster/resources: add tags to returned properties Dominik Csapak
@ 2022-11-16 8:02 ` Thomas Lamprecht
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 8:02 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
would adapt this a bit, see inline, but no hard feelings
Am 15/11/2022 um 14:02 schrieb Dominik Csapak:
> by querying 'lock' and 'tags' with 'get_guest_config_properties'
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> PVE/API2/Cluster.pm | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
> index 49e319a5c..34a6e08cd 100644
> --- a/PVE/API2/Cluster.pm
> +++ b/PVE/API2/Cluster.pm
> @@ -371,7 +371,8 @@ __PACKAGE__->register_method({
>
> # we try to generate 'numbers' by using "$X + 0"
> if (!$param->{type} || $param->{type} eq 'vm') {
> - my $locked_vms = PVE::Cluster::get_guest_config_property('lock');
> + my $prop_list = [qw(lock tags)];
> + my $props = PVE::Cluster::get_guest_config_properties($prop_list);
props is a bit generic and having the prop_list here and below adds not much
value IMO, maybe rather here:
my $extra_guest_info = PVE::Cluster::get_guest_config_properties(['lock', 'tags']);
>
> for my $vmid (sort keys %$idlist) {
>
> @@ -403,8 +404,10 @@ __PACKAGE__->register_method({
> # only skip now to next to ensure that the pool stats above are filled, if eligible
> next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
>
> - if (defined(my $lock = $locked_vms->{$vmid}->{lock})) {
> - $entry->{lock} = $lock;
> + for my $prop (@$prop_list) {
and then here:
for my $key (keys $extra_guest_info->{$vmid}->%) {
my $value = $extra_guest_info->{$vmid}->{$key} // next;
$entry->{$prop} = $extra_guest_info->{$vmid}->{$key};
}
> + if (defined(my $value = $props->{$vmid}->{$prop})) {
> + $entry->{$prop} = $value;
> + }
> }
>
> if (defined($entry->{pool}) &&
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH manager v10 02/13] api: add /ui-options api call
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (10 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 01/13] api: /cluster/resources: add tags to returned properties Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 03/13] ui: call '/ui-options' and save the result in PVE.UIOptions Dominik Csapak
` (10 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
which contains ui relevant options, like the console preference and tag-style
also contains the list of allowed tags
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2.pm | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/PVE/API2.pm b/PVE/API2.pm
index a42561604..7c1d0830d 100644
--- a/PVE/API2.pm
+++ b/PVE/API2.pm
@@ -118,6 +118,7 @@ __PACKAGE__->register_method ({
my $res = {};
+ # TODO remove with next major release
my $datacenter_confg = eval { PVE::Cluster::cfs_read_file('datacenter.cfg') } // {};
for my $k (qw(console)) {
$res->{$k} = $datacenter_confg->{$k} if exists $datacenter_confg->{$k};
@@ -130,4 +131,64 @@ __PACKAGE__->register_method ({
return $res;
}});
+__PACKAGE__->register_method ({
+ name => 'ui-options',
+ path => 'ui-options',
+ method => 'GET',
+ permissions => { user => 'all' },
+ description => "Global options regarding the UI.",
+ parameters => {
+ additionalProperties => 0,
+ properties => {},
+ },
+ returns => {
+ type => "object",
+ properties => {
+ console => {
+ type => 'string',
+ enum => ['applet', 'vv', 'html5', 'xtermjs'],
+ optional => 1,
+ description => 'The default console viewer to use.',
+ },
+ 'tag-style' => {
+ type => 'string',
+ optional => 1,
+ description => 'Cluster wide tag style overrides',
+ },
+ 'allowed-tags' => {
+ type => 'array',
+ optional => 1,
+ description => 'A list of allowed tags that exist.',
+ items => {
+ type => 'string',
+ description => 'A tag the user is allowed to set.',
+ },
+ },
+ },
+ },
+ code => sub {
+ my ($param) = @_;
+
+ my $res = {};
+
+ my $rpcenv = PVE::RPCEnvironment::get();
+ my $authuser = $rpcenv->get_user();
+
+ my $datacenter_confg = eval { PVE::Cluster::cfs_read_file('datacenter.cfg') } // {};
+ for my $k (qw(console tag-style)) {
+ $res->{$k} = $datacenter_confg->{$k} if exists $datacenter_confg->{$k};
+ }
+
+ my $privileged_user = $rpcenv->check($authuser, '/', ['Sys.Modify'], 1);
+
+ my $tags = PVE::DataCenterConfig::get_allowed_tags($privileged_user, sub {
+ my ($vmid) = @_;
+ return $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Audit'], 0, 1);
+ });
+
+ $res->{'allowed-tags'} = [sort keys $tags->%*];
+
+ return $res;
+ }});
+
1;
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 03/13] ui: call '/ui-options' and save the result in PVE.UIOptions
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (11 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 02/13] api: add /ui-options api call Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 04/13] ui: parse and save tag infos from /ui-options Dominik Csapak
` (9 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
and move the use of the console from VersionInfo to here, since
this will be the future place for ui related backend options.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/Utils.js | 12 +++++++++++-
www/manager6/Workspace.js | 2 ++
www/manager6/dc/OptionView.js | 4 ++--
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index adcf082ff..29bf667b4 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1332,7 +1332,7 @@ Ext.define('PVE.Utils', {
allowSpice = consoles.spice;
allowXtermjs = !!consoles.xtermjs;
}
- let dv = PVE.VersionInfo.console || (type === 'kvm' ? 'vv' : 'xtermjs');
+ let dv = PVE.UIOptions.console || (type === 'kvm' ? 'vv' : 'xtermjs');
if (dv === 'vv' && !allowSpice) {
dv = allowXtermjs ? 'xtermjs' : 'html5';
} else if (dv === 'xtermjs' && !allowXtermjs) {
@@ -1854,6 +1854,16 @@ Ext.define('PVE.Utils', {
},
notesTemplateVars: ['cluster', 'guestname', 'node', 'vmid'],
+
+ updateUIOptions: function() {
+ Proxmox.Utils.API2Request({
+ url: '/ui-options',
+ method: 'GET',
+ success: function(response) {
+ PVE.UIOptions = response?.result?.data ?? {};
+ },
+ });
+ },
},
singleton: true,
diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
index 2bb502e0c..a7423508e 100644
--- a/www/manager6/Workspace.js
+++ b/www/manager6/Workspace.js
@@ -158,6 +158,8 @@ Ext.define('PVE.StdWorkspace', {
},
});
+ PVE.Utils.updateUIOptions();
+
Proxmox.Utils.API2Request({
url: '/cluster/sdn',
method: 'GET',
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index 5a2be182e..ff96351d5 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -343,9 +343,9 @@ Ext.define('PVE.dc.OptionView', {
}
var rec = store.getById('console');
- PVE.VersionInfo.console = rec.data.value;
+ PVE.UIOptions.console = rec.data.value;
if (rec.data.value === '__default__') {
- delete PVE.VersionInfo.console;
+ delete PVE.UIOptions.console;
}
});
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 04/13] ui: parse and save tag infos from /ui-options
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (12 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 03/13] ui: call '/ui-options' and save the result in PVE.UIOptions Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 05/13] ui: add form/TagColorGrid Dominik Csapak
` (8 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
stores the color-map into a global list of overrides. on update, also parse
the values from the browser localstore. Also emits a GlobalEvent
'loadedUiOptions' so that e.g. the tags can listen to that and refresh their
colors
also saves the list of 'allowed-tags' into PVE.Utils
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* remove unused global 'override' event
* refactored the UIOptions update function and updateTagSettings a bit
(now takes the full 'tag-style' object and handles it)
www/manager6/Utils.js | 56 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 29bf667b4..4ff8a1a55 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1860,10 +1860,64 @@ Ext.define('PVE.Utils', {
url: '/ui-options',
method: 'GET',
success: function(response) {
- PVE.UIOptions = response?.result?.data ?? {};
+ PVE.UIOptions = response?.result?.data ?? {
+ 'allowed-tags': [],
+ };
+
+ PVE.Utils.updateTagList(PVE.UIOptions['allowed-tags']);
+ PVE.Utils.updateTagSettings(PVE.UIOptions?.['tag-style']);
},
});
},
+
+ tagList: new Set(),
+
+ updateTagList: function(tags) {
+ PVE.Utils.tagList = [...new Set([...tags])].sort();
+ },
+
+ parseTagOverrides: function(overrides) {
+ let colors = {};
+ (overrides || "").split(';').forEach(color => {
+ if (!color) {
+ return;
+ }
+ let [tag, color_hex, font_hex] = color.split(':');
+ let r = parseInt(color_hex.slice(0, 2), 16);
+ let g = parseInt(color_hex.slice(2, 4), 16);
+ let b = parseInt(color_hex.slice(4, 6), 16);
+ colors[tag] = [r, g, b];
+ if (font_hex) {
+ colors[tag].push(parseInt(font_hex.slice(0, 2), 16));
+ colors[tag].push(parseInt(font_hex.slice(2, 4), 16));
+ colors[tag].push(parseInt(font_hex.slice(4, 6), 16));
+ }
+ });
+ return colors;
+ },
+
+ tagOverrides: {},
+
+ updateTagOverrides: function(colors) {
+ let sp = Ext.state.Manager.getProvider();
+ let color_state = sp.get('colors', '');
+ let browser_colors = PVE.Utils.parseTagOverrides(color_state);
+ PVE.Utils.tagOverrides = Ext.apply({}, browser_colors, colors);
+ },
+
+ updateTagSettings: function(style) {
+ let overrides = style?.['color-map'];
+ PVE.Utils.updateTagOverrides(PVE.Utils.parseTagOverrides(overrides ?? ""));
+
+ let shape = style?.shape ?? 'circle';
+ if (shape === '__default__') {
+ style = 'circle';
+ }
+
+ Ext.ComponentQuery.query('pveResourceTree')[0].setUserCls(`proxmox-tags-${shape}`);
+ PVE.data.ResourceStore.fireEvent('load');
+ Ext.GlobalEvents.fireEvent('loadedUiOptions');
+ },
},
singleton: true,
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 05/13] ui: add form/TagColorGrid
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (13 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 04/13] ui: parse and save tag infos from /ui-options Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 06/13] ui: add PVE.form.ListField Dominik Csapak
` (7 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
this provides a basic grid to edit a list of tag color overrides.
We'll use this for editing the datacenter.cfg overrides and the
browser storage overrides.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/css/ext6-pve.css | 5 +
www/manager6/Makefile | 1 +
www/manager6/Utils.js | 2 +
www/manager6/form/TagColorGrid.js | 357 ++++++++++++++++++++++++++++++
4 files changed, 365 insertions(+)
create mode 100644 www/manager6/form/TagColorGrid.js
diff --git a/www/css/ext6-pve.css b/www/css/ext6-pve.css
index dadb84a97..f7d0c4201 100644
--- a/www/css/ext6-pve.css
+++ b/www/css/ext6-pve.css
@@ -651,3 +651,8 @@ table.osds td:first-of-type {
background-color: rgb(245, 245, 245);
color: #000;
}
+
+.x-pveColorPicker-default-cell > .x-grid-cell-inner {
+ padding-top: 0px;
+ padding-bottom: 0px;
+}
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 2802cbacd..572aa9392 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -73,6 +73,7 @@ JSSRC= \
form/VNCKeyboardSelector.js \
form/ViewSelector.js \
form/iScsiProviderSelector.js \
+ form/TagColorGrid.js \
grid/BackupView.js \
grid/FirewallAliases.js \
grid/FirewallOptions.js \
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 4ff8a1a55..7cb77083d 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1918,6 +1918,8 @@ Ext.define('PVE.Utils', {
PVE.data.ResourceStore.fireEvent('load');
Ext.GlobalEvents.fireEvent('loadedUiOptions');
},
+
+ tagCharRegex: /^[a-z0-9+_.-]$/i,
},
singleton: true,
diff --git a/www/manager6/form/TagColorGrid.js b/www/manager6/form/TagColorGrid.js
new file mode 100644
index 000000000..3ad8e07f0
--- /dev/null
+++ b/www/manager6/form/TagColorGrid.js
@@ -0,0 +1,357 @@
+Ext.define('PVE.form.ColorPicker', {
+ extend: 'Ext.form.FieldContainer',
+ alias: 'widget.pveColorPicker',
+
+ defaultBindProperty: 'value',
+
+ config: {
+ value: null,
+ },
+
+ height: 24,
+
+ layout: {
+ type: 'hbox',
+ align: 'stretch',
+ },
+
+ getValue: function() {
+ return this.realvalue.slice(1);
+ },
+
+ setValue: function(value) {
+ let me = this;
+ me.setColor(value);
+ if (value && value.length === 6) {
+ me.picker.value = value[0] !== '#' ? `#${value}` : value;
+ }
+ },
+
+ setColor: function(value) {
+ let me = this;
+ let oldValue = me.realvalue;
+ me.realvalue = value;
+ let color = value.length === 6 ? `#${value}` : undefined;
+ me.down('#picker').setStyle('background-color', color);
+ me.down('#text').setValue(value ?? "");
+ me.fireEvent('change', me, me.realvalue, oldValue);
+ },
+
+ initComponent: function() {
+ let me = this;
+ me.picker = document.createElement('input');
+ me.picker.type = 'color';
+ me.picker.style = `opacity: 0; border: 0px; width: 100%; height: ${me.height}px`;
+ me.picker.value = `${me.value}`;
+
+ me.items = [
+ {
+ xtype: 'textfield',
+ itemId: 'text',
+ minLength: !me.allowBlank ? 6 : undefined,
+ maxLength: 6,
+ enforceMaxLength: true,
+ allowBlank: me.allowBlank,
+ emptyText: me.allowBlank ? gettext('Automatic') : undefined,
+ maskRe: /[a-f0-9]/i,
+ regex: /^[a-f0-9]{6}$/i,
+ flex: 1,
+ listeners: {
+ change: function(field, value) {
+ me.setValue(value);
+ },
+ },
+ },
+ {
+ xtype: 'box',
+ style: {
+ 'margin-left': '1px',
+ border: '1px solid #cfcfcf',
+ },
+ itemId: 'picker',
+ width: 24,
+ contentEl: me.picker,
+ },
+ ];
+
+ me.callParent();
+ me.picker.oninput = function() {
+ me.setColor(me.picker.value.slice(1));
+ };
+ },
+});
+
+Ext.define('PVE.form.TagColorGrid', {
+ extend: 'Ext.grid.Panel',
+ alias: 'widget.pveTagColorGrid',
+
+ mixins: [
+ 'Ext.form.field.Field',
+ ],
+
+ allowBlank: true,
+ selectAll: false,
+ isFormField: true,
+ deleteEmpty: false,
+ selModel: 'checkboxmodel',
+
+ config: {
+ deleteEmpty: false,
+ },
+
+ emptyText: gettext('No Overrides'),
+ viewConfig: {
+ deferEmptyText: false,
+ },
+
+ setValue: function(value) {
+ let me = this;
+ let colors;
+ if (Ext.isObject(value)) {
+ colors = value.colors;
+ } else {
+ colors = value;
+ }
+ if (!colors) {
+ me.getStore().removeAll();
+ me.checkChange();
+ return me;
+ }
+ let entries = (colors.split(';') || []).map((entry) => {
+ let [tag, bg, fg] = entry.split(':');
+ fg = fg || "";
+ return {
+ tag,
+ color: bg,
+ text: fg,
+ };
+ });
+ me.getStore().setData(entries);
+ me.checkChange();
+ return me;
+ },
+
+ getValue: function() {
+ let me = this;
+ let values = [];
+ me.getStore().each((rec) => {
+ if (rec.data.tag) {
+ let val = `${rec.data.tag}:${rec.data.color}`;
+ if (rec.data.text) {
+ val += `:${rec.data.text}`;
+ }
+ values.push(val);
+ }
+ });
+ return values.join(';');
+ },
+
+ getErrors: function(value) {
+ let me = this;
+ let emptyTag = false;
+ let notValidColor = false;
+ let colorRegex = new RegExp(/^[0-9a-f]{6}$/i);
+ me.getStore().each((rec) => {
+ if (!rec.data.tag) {
+ emptyTag = true;
+ }
+ if (!rec.data.color?.match(colorRegex)) {
+ notValidColor = true;
+ }
+ if (rec.data.text && !rec.data.text?.match(colorRegex)) {
+ notValidColor = true;
+ }
+ });
+ let errors = [];
+ if (emptyTag) {
+ errors.push(gettext('Tag must not be empty.'));
+ }
+ if (notValidColor) {
+ errors.push(gettext('Not a valid color.'));
+ }
+ return errors;
+ },
+
+ // override framework function to implement deleteEmpty behaviour
+ getSubmitData: function() {
+ let me = this,
+ data = null,
+ val;
+ if (!me.disabled && me.submitValue) {
+ val = me.getValue();
+ if (val !== null && val !== '') {
+ data = {};
+ data[me.getName()] = val;
+ } else if (me.getDeleteEmpty()) {
+ data = {};
+ data.delete = me.getName();
+ }
+ }
+ return data;
+ },
+
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ addLine: function() {
+ let me = this;
+ me.getView().getStore().add({
+ tag: '',
+ color: '',
+ text: '',
+ });
+ },
+
+ removeSelection: function() {
+ let me = this;
+ let view = me.getView();
+ let selection = view.getSelection();
+ if (selection === undefined) {
+ return;
+ }
+
+ selection.forEach((sel) => {
+ view.getStore().remove(sel);
+ });
+ view.checkChange();
+ },
+
+ tagChange: function(field, newValue, oldValue) {
+ let me = this;
+ let rec = field.getWidgetRecord();
+ if (!rec) {
+ return;
+ }
+ if (newValue && newValue !== oldValue) {
+ let newrgb = Proxmox.Utils.stringToRGB(newValue);
+ let newvalue = Proxmox.Utils.rgbToHex(newrgb);
+ if (!rec.get('color')) {
+ rec.set('color', newvalue);
+ } else if (oldValue) {
+ let oldrgb = Proxmox.Utils.stringToRGB(oldValue);
+ let oldvalue = Proxmox.Utils.rgbToHex(oldrgb);
+ if (rec.get('color') === oldvalue) {
+ rec.set('color', newvalue);
+ }
+ }
+ }
+ me.fieldChange(field, newValue, oldValue);
+ },
+
+ backgroundChange: function(field, newValue, oldValue) {
+ let me = this;
+ let rec = field.getWidgetRecord();
+ if (!rec) {
+ return;
+ }
+ if (newValue && newValue !== oldValue) {
+ let newrgb = Proxmox.Utils.hexToRGB(newValue);
+ let newcls = Proxmox.Utils.getTextContrastClass(newrgb);
+ let hexvalue = newcls === 'dark' ? '000000' : 'FFFFFF';
+ if (!rec.get('text')) {
+ rec.set('text', hexvalue);
+ } else if (oldValue) {
+ let oldrgb = Proxmox.Utils.hexToRGB(oldValue);
+ let oldcls = Proxmox.Utils.getTextContrastClass(oldrgb);
+ let oldvalue = oldcls === 'dark' ? '000000' : 'FFFFFF';
+ if (rec.get('text') === oldvalue) {
+ rec.set('text', hexvalue);
+ }
+ }
+ }
+ me.fieldChange(field, newValue, oldValue);
+ },
+
+ fieldChange: function(field, newValue, oldValue) {
+ let me = this;
+ let view = me.getView();
+ let rec = field.getWidgetRecord();
+ if (!rec) {
+ return;
+ }
+ let column = field.getWidgetColumn();
+ rec.set(column.dataIndex, newValue);
+ view.checkChange();
+ },
+ },
+
+ tbar: [
+ {
+ text: gettext('Add'),
+ handler: 'addLine',
+ },
+ {
+ xtype: 'proxmoxButton',
+ text: gettext('Remove'),
+ handler: 'removeSelection',
+ disabled: true,
+ },
+ ],
+
+ columns: [
+ {
+ header: 'Tag',
+ dataIndex: 'tag',
+ xtype: 'widgetcolumn',
+ onWidgetAttach: function(col, widget, rec) {
+ widget.getStore().setData(PVE.Utils.tagList.map(v => ({ tag: v })));
+ },
+ widget: {
+ xtype: 'combobox',
+ isFormField: false,
+ maskRe: PVE.Utils.tagCharRegex,
+ allowBlank: false,
+ queryMode: 'local',
+ displayField: 'tag',
+ valueField: 'tag',
+ store: {},
+ listeners: {
+ change: 'tagChange',
+ },
+ },
+ flex: 1,
+ },
+ {
+ header: gettext('Background'),
+ xtype: 'widgetcolumn',
+ flex: 1,
+ dataIndex: 'color',
+ widget: {
+ xtype: 'pveColorPicker',
+ isFormField: false,
+ listeners: {
+ change: 'backgroundChange',
+ },
+ },
+ },
+ {
+ header: gettext('Text'),
+ xtype: 'widgetcolumn',
+ flex: 1,
+ dataIndex: 'text',
+ widget: {
+ xtype: 'pveColorPicker',
+ allowBlank: true,
+ isFormField: false,
+ listeners: {
+ change: 'fieldChange',
+ },
+ },
+ },
+ ],
+
+ store: {
+ listeners: {
+ update: function() {
+ this.commitChanges();
+ },
+ },
+ },
+
+ initComponent: function() {
+ let me = this;
+ me.callParent();
+ me.initField();
+ },
+});
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 06/13] ui: add PVE.form.ListField
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (14 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 05/13] ui: add form/TagColorGrid Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 07/13] ui: dc/OptionView: add editors for tag settings Dominik Csapak
` (6 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
a field which contains just a list of textfields, e.g. useful for a list
of simple tags
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/Makefile | 1 +
www/manager6/form/ListField.js | 165 +++++++++++++++++++++++++++++++++
2 files changed, 166 insertions(+)
create mode 100644 www/manager6/form/ListField.js
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 572aa9392..83c2effcf 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -74,6 +74,7 @@ JSSRC= \
form/ViewSelector.js \
form/iScsiProviderSelector.js \
form/TagColorGrid.js \
+ form/ListField.js \
grid/BackupView.js \
grid/FirewallAliases.js \
grid/FirewallOptions.js \
diff --git a/www/manager6/form/ListField.js b/www/manager6/form/ListField.js
new file mode 100644
index 000000000..faa8e168b
--- /dev/null
+++ b/www/manager6/form/ListField.js
@@ -0,0 +1,165 @@
+Ext.define('PVE.form.ListField', {
+ extend: 'Ext.grid.Panel',
+ alias: 'widget.pveListField',
+
+ mixins: [
+ 'Ext.form.field.Field',
+ ],
+
+ // override for column header
+ fieldTitle: gettext('Item'),
+
+ // will be applied to the textfields
+ maskRe: undefined,
+
+ allowBlank: true,
+ selectAll: false,
+ isFormField: true,
+ deleteEmpty: false,
+ selModel: 'checkboxmodel',
+
+ config: {
+ deleteEmpty: false,
+ },
+
+ viewConfig: {
+ deferEmptyText: false,
+ },
+
+ setValue: function(list) {
+ let me = this;
+ list = Ext.isArray(list) ? list : (list ?? '').split(';');
+ if (list.length > 0) {
+ me.getStore().setData(list.map(item => ({ item })));
+ } else {
+ me.getStore().removeAll();
+ }
+ me.checkChange();
+ return me;
+ },
+
+ getValue: function() {
+ let me = this;
+ let values = [];
+ me.getStore().each((rec) => {
+ if (rec.data.item) {
+ values.push(rec.data.item);
+ }
+ });
+ return values.join(';');
+ },
+
+ getErrors: function(value) {
+ let me = this;
+ let empty = false;
+ me.getStore().each((rec) => {
+ if (!rec.data.item) {
+ empty = true;
+ }
+ });
+ if (empty) {
+ return [gettext('Tag must not be empty.')];
+ }
+ return [];
+ },
+
+ // override framework function to implement deleteEmpty behaviour
+ getSubmitData: function() {
+ let me = this,
+ data = null,
+ val;
+ if (!me.disabled && me.submitValue) {
+ val = me.getValue();
+ if (val !== null && val !== '') {
+ data = {};
+ data[me.getName()] = val;
+ } else if (me.getDeleteEmpty()) {
+ data = {};
+ data.delete = me.getName();
+ }
+ }
+ return data;
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ addLine: function() {
+ let me = this;
+ me.getView().getStore().add({
+ item: '',
+ });
+ },
+
+ removeSelection: function() {
+ let me = this;
+ let view = me.getView();
+ let selection = view.getSelection();
+ if (selection === undefined) {
+ return;
+ }
+
+ selection.forEach((sel) => {
+ view.getStore().remove(sel);
+ });
+ view.checkChange();
+ },
+
+ itemChange: function(field, newValue) {
+ let rec = field.getWidgetRecord();
+ if (!rec) {
+ return;
+ }
+ let column = field.getWidgetColumn();
+ rec.set(column.dataIndex, newValue);
+ field.up('pveListField').checkChange();
+ },
+ },
+
+ tbar: [
+ {
+ text: gettext('Add'),
+ handler: 'addLine',
+ },
+ {
+ xtype: 'proxmoxButton',
+ text: gettext('Remove'),
+ handler: 'removeSelection',
+ disabled: true,
+ },
+ ],
+
+ store: {
+ listeners: {
+ update: function() {
+ this.commitChanges();
+ },
+ },
+ },
+
+ initComponent: function() {
+ let me = this;
+
+ me.columns = [
+ {
+ header: me.fieldTtitle,
+ dataIndex: 'item',
+ xtype: 'widgetcolumn',
+ widget: {
+ xtype: 'textfield',
+ isFormField: false,
+ maskRe: me.maskRe,
+ allowBlank: false,
+ queryMode: 'local',
+ listeners: {
+ change: 'itemChange',
+ },
+ },
+ flex: 1,
+ },
+ ];
+
+ me.callParent();
+ me.initField();
+ },
+});
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 07/13] ui: dc/OptionView: add editors for tag settings
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (15 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 06/13] ui: add PVE.form.ListField Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 08/13] ui: add form/Tag Dominik Csapak
` (5 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
namely for 'tag-tree-style' and 'tag-colors'.
display the tag overrides directly as they will appear as tags
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* now also sets PVE.UIOptions
* adapt to change of updateTagSettings
www/manager6/Utils.js | 20 ++++
www/manager6/dc/OptionView.js | 200 ++++++++++++++++++++++++++++++++++
2 files changed, 220 insertions(+)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 7cb77083d..2e819ce84 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1919,6 +1919,26 @@ Ext.define('PVE.Utils', {
Ext.GlobalEvents.fireEvent('loadedUiOptions');
},
+ tagTreeStyles: {
+ '__default__': Proxmox.Utils.defaultText,
+ 'full': gettext('Full'),
+ 'circle': gettext('Circle'),
+ 'dense': gettext('Dense'),
+ 'none': Proxmox.Utils.NoneText,
+ },
+
+ renderTags: function(tagstext, overrides) {
+ let text = '';
+ if (tagstext) {
+ let tags = (tagstext.split(/[,; ]/) || []).filter(t => !!t);
+ text += ' ';
+ tags.forEach((tag) => {
+ text += Proxmox.Utils.getTagElement(tag, overrides);
+ });
+ }
+ return text;
+ },
+
tagCharRegex: /^[a-z0-9+_.-]$/i,
},
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index ff96351d5..ef2989d6c 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -5,6 +5,7 @@ Ext.define('PVE.dc.OptionView', {
onlineHelp: 'datacenter_configuration_file',
monStoreErrors: true,
+ userCls: 'proxmox-tags-full',
add_inputpanel_row: function(name, text, opts) {
var me = this;
@@ -312,6 +313,202 @@ Ext.define('PVE.dc.OptionView', {
submitValue: true,
}],
});
+ me.rows['tag-style'] = {
+ required: true,
+ renderer: (value) => {
+ if (value === undefined) {
+ return gettext('No Overrides');
+ }
+ let colors = PVE.Utils.parseTagOverrides(value?.['color-map']);
+ let shape = value.shape;
+ let shapeText = PVE.Utils.tagTreeStyles[shape] ?? Proxmox.Utils.defaultText;
+ let txt = Ext.String.format(gettext("Tree Shape: {0}"), shapeText);
+ if (Object.keys(colors).length > 0) {
+ txt += ', ';
+ }
+ for (const tag of Object.keys(colors)) {
+ txt += Proxmox.Utils.getTagElement(tag, colors);
+ }
+ return txt;
+ },
+ header: gettext('Tag Style Override'),
+ editor: {
+ xtype: 'proxmoxWindowEdit',
+ width: 800,
+ subject: gettext('Tag Color Override'),
+ onlineHelp: 'datacenter_configuration_file',
+ fieldDefaults: {
+ labelWidth: 100,
+ },
+ url: '/api2/extjs/cluster/options',
+ items: [
+ {
+ xtype: 'inputpanel',
+ setValues: function(values) {
+ if (values === undefined) {
+ return undefined;
+ }
+ values = values?.['tag-style'] ?? {};
+ values.shape = values.shape || '__default__';
+ values.colors = values['color-map'];
+ return Proxmox.panel.InputPanel.prototype.setValues.call(this, values);
+ },
+ onGetValues: function(values) {
+ let style = {};
+ if (values.colors) {
+ style['color-map'] = values.colors;
+ }
+ if (values.shape) {
+ style.shape = values.shape;
+ }
+ let value = PVE.Parser.printPropertyString(style);
+ if (value === '') {
+ return {
+ 'delete': 'tag-style',
+ };
+ }
+ return {
+ 'tag-style': value,
+ };
+ },
+ items: [
+ {
+ name: 'shape',
+ xtype: 'proxmoxKVComboBox',
+ fieldLabel: gettext('Tree Shape'),
+ comboItems: Object.entries(PVE.Utils.tagTreeStyles),
+ defaultValue: '__default__',
+ deleteEmpty: true,
+ },
+ {
+ xtype: 'displayfield',
+ fieldLabel: gettext('Color Overrides'),
+ },
+ {
+ name: 'colors',
+ xtype: 'pveTagColorGrid',
+ deleteEmpty: true,
+ height: 300,
+ },
+ ],
+ },
+ ],
+ },
+ };
+
+ me.rows['user-tag-access'] = {
+ required: true,
+ renderer: (value) => {
+ if (value === undefined) {
+ return Ext.String.format(gettext('Mode: {0}'), 'free');
+ }
+ let mode = value?.['user-allow'] ?? 'free';
+ let list = value?.['user-allow-list'].join(',');
+ let modeTxt = Ext.String.format(gettext('Mode {0}'), mode);
+ let overrides = PVE.Utils.tagOverrides;
+ let tags = PVE.Utils.renderTags(list, overrides);
+
+ return `${modeTxt}, ${gettext('Pre-defined:')} ${tags}`;
+ },
+ header: gettext('User Tag Access'),
+ editor: {
+ xtype: 'proxmoxWindowEdit',
+ subject: gettext('User Tag Access'),
+ onlineHelp: 'datacenter_configuration_file',
+ url: '/api2/extjs/cluster/options',
+ items: [
+ {
+ xtype: 'inputpanel',
+ setValues: function(values) {
+ let data = values?.['user-tag-access'] ?? {};
+ console.log(data);
+ return Proxmox.panel.InputPanel.prototype.setValues.call(this, data);
+ },
+ onGetValues: function(values) {
+ if (values === undefined || Object.keys(values).length === 0) {
+ return { 'delete': name };
+ }
+ return {
+ 'user-tag-access': PVE.Parser.printPropertyString(values),
+ };
+ },
+ items: [
+ {
+ name: 'user-allow',
+ fieldLabel: gettext('Mode'),
+ xtype: 'proxmoxKVComboBox',
+ deleteEmpty: false,
+ value: '__default__',
+ comboItems: [
+ ['__default__', Proxmox.Utils.defaultText + ' (free)'],
+ ['free', 'free'],
+ ['existing', 'existing'],
+ ['list', 'list'],
+ ['none', 'none'],
+ ],
+ defaultValue: '__default__',
+ },
+ {
+ xtype: 'displayfield',
+ fieldLabel: gettext('Predefined Tags'),
+ },
+ {
+ name: 'user-allow-list',
+ xtype: 'pveListField',
+ emptyText: gettext('No Tags defined'),
+ fieldTitle: gettext('Tag'),
+ maskRe: PVE.Utils.tagCharRegex,
+ height: 200,
+ scrollable: true,
+ },
+ ],
+ },
+ ],
+ },
+ };
+
+ me.rows['privileged-tags'] = {
+ required: true,
+ renderer: (value) => {
+ if (value === undefined) {
+ return gettext('No Privilged Tags');
+ }
+ let overrides = PVE.Utils.tagOverrides;
+ return PVE.Utils.renderTags(value.join(','), overrides);
+ },
+ header: gettext('Privileged Tags'),
+ editor: {
+ xtype: 'proxmoxWindowEdit',
+ subject: gettext('Privileged Tags'),
+ onlineHelp: 'datacenter_configuration_file',
+ url: '/api2/extjs/cluster/options',
+ items: [
+ {
+ xtype: 'inputpanel',
+ setValues: function(values) {
+ let tags = values?.['privileged-tags'] ?? '';
+ return Proxmox.panel.InputPanel.prototype.setValues.call(this, { tags });
+ },
+ onGetValues: function(values) {
+ return {
+ 'privileged-tags': values,
+ };
+ },
+ items: [
+ {
+ name: 'tags',
+ xtype: 'pveListField',
+ emptyText: gettext('No Tags defined'),
+ fieldTitle: gettext('Tag'),
+ maskRe: PVE.Utils.tagCharRegex,
+ height: 200,
+ scrollable: true,
+ },
+ ],
+ },
+ ],
+ },
+ };
me.selModel = Ext.create('Ext.selection.RowModel', {});
@@ -347,6 +544,9 @@ Ext.define('PVE.dc.OptionView', {
if (rec.data.value === '__default__') {
delete PVE.UIOptions.console;
}
+
+ PVE.UIOptions['tag-style'] = store.getById('tag-style')?.data?.value;
+ PVE.Utils.updateTagSettings(PVE.UIOptions['tag-style']);
});
me.on('activate', me.rstore.startUpdate);
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 08/13] ui: add form/Tag
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (16 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 07/13] ui: dc/OptionView: add editors for tag settings Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-16 14:57 ` Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js Dominik Csapak
` (4 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
displays a single tag, with the ability to edit inline on click (when
the mode is set to editable). This brings up a list of globally available tags
for simple selection.
this is a basic ext component, with 'i' tags for the icons (handle and
add/remove button) and a span (for the tag text)
shows the tag by default, and if put in editable mode, makes the
span editable. when actually starting the edit, shows a picker
with available tags to select from
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* don't listen to the global event anymore
www/css/ext6-pve.css | 32 ++++++
www/manager6/Makefile | 1 +
www/manager6/form/Tag.js | 232 +++++++++++++++++++++++++++++++++++++++
3 files changed, 265 insertions(+)
create mode 100644 www/manager6/form/Tag.js
diff --git a/www/css/ext6-pve.css b/www/css/ext6-pve.css
index f7d0c4201..daaffa6ec 100644
--- a/www/css/ext6-pve.css
+++ b/www/css/ext6-pve.css
@@ -656,3 +656,35 @@ table.osds td:first-of-type {
padding-top: 0px;
padding-bottom: 0px;
}
+
+.pve-edit-tag > i {
+ cursor: pointer;
+ font-size: 14px;
+}
+
+.pve-edit-tag > i.handle {
+ padding-right: 5px;
+ cursor: grab;
+}
+
+.pve-edit-tag > i.action {
+ padding-left: 5px;
+}
+
+.pve-edit-tag.normal > i {
+ display: none;
+}
+
+.pve-edit-tag.editable span,
+.pve-edit-tag.inEdit span {
+ background-color: #ffffff;
+ border: 1px solid #a8a8a8;
+ color: #000;
+ padding-left: 2px;
+ padding-right: 2px;
+ min-width: 2em;
+}
+
+.pve-edit-tag.inEdit span {
+ border: 1px solid #000;
+}
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 83c2effcf..21be8da8e 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -75,6 +75,7 @@ JSSRC= \
form/iScsiProviderSelector.js \
form/TagColorGrid.js \
form/ListField.js \
+ form/Tag.js \
grid/BackupView.js \
grid/FirewallAliases.js \
grid/FirewallOptions.js \
diff --git a/www/manager6/form/Tag.js b/www/manager6/form/Tag.js
new file mode 100644
index 000000000..cc4413d29
--- /dev/null
+++ b/www/manager6/form/Tag.js
@@ -0,0 +1,232 @@
+Ext.define('Proxmox.Tag', {
+ extend: 'Ext.Component',
+ alias: 'widget.pmxTag',
+
+ mode: 'editable',
+
+ icons: {
+ editable: 'fa fa-minus-square',
+ normal: '',
+ inEdit: 'fa fa-check-square',
+ },
+
+ tag: '',
+ cls: 'pve-edit-tag',
+
+ tpl: [
+ '<i class="handle fa fa-bars"></i>',
+ '<span>{tag}</span>',
+ '<i class="action {iconCls}"></i>',
+ ],
+
+ // we need to do this in mousedown, because that triggers before
+ // focusleave (which triggers before click)
+ onMouseDown: function(event) {
+ let me = this;
+ if (event.target.tagName !== 'I' || event.target.classList.contains('handle')) {
+ return;
+ }
+ switch (me.mode) {
+ case 'editable':
+ me.setVisible(false);
+ me.setTag('');
+ break;
+ case 'inEdit':
+ me.setTag(me.tagEl().innerHTML);
+ me.setMode('editable');
+ break;
+ default: break;
+ }
+ },
+
+ onClick: function(event) {
+ let me = this;
+ if (event.target.tagName !== 'SPAN' || me.mode !== 'editable') {
+ return;
+ }
+ me.setMode('inEdit');
+
+ // select text in the element
+ let tagEl = me.tagEl();
+ tagEl.contentEditable = true;
+ let range = document.createRange();
+ range.selectNodeContents(tagEl);
+ let sel = window.getSelection();
+ sel.removeAllRanges();
+ sel.addRange(range);
+
+ me.showPicker();
+ },
+
+ showPicker: function() {
+ let me = this;
+ if (!me.picker) {
+ me.picker = Ext.widget({
+ xtype: 'boundlist',
+ minWidth: 70,
+ scrollable: true,
+ floating: true,
+ hidden: true,
+ userCls: 'proxmox-tags-full',
+ displayField: 'tag',
+ itemTpl: [
+ '{[Proxmox.Utils.getTagElement(values.tag, PVE.Utils.tagOverrides)]}',
+ ],
+ store: [],
+ listeners: {
+ select: function(picker, rec) {
+ me.setTag(rec.data.tag);
+ me.setMode('editable');
+ me.picker.hide();
+ },
+ },
+ });
+ }
+ me.picker.getStore()?.clearFilter();
+ let taglist = PVE.Utils.tagList.map(v => ({ tag: v }));
+ if (taglist.length < 1) {
+ return;
+ }
+ me.picker.getStore().setData(taglist);
+ me.picker.showBy(me, 'tl-bl');
+ me.picker.setMaxHeight(200);
+ },
+
+ setMode: function(mode) {
+ let me = this;
+ if (me.icons[mode] === undefined) {
+ throw "invalid mode";
+ }
+ let tagEl = me.tagEl();
+ if (tagEl) {
+ tagEl.contentEditable = mode === 'inEdit';
+ }
+ me.removeCls(me.mode);
+ me.addCls(mode);
+ me.mode = mode;
+ me.updateData();
+ },
+
+ onKeyPress: function(event) {
+ let me = this;
+ let key = event.browserEvent.key;
+ switch (key) {
+ case 'Enter':
+ if (me.tagEl().innerHTML !== '') {
+ me.setTag(me.tagEl().innerHTML);
+ me.setMode('editable');
+ return;
+ }
+ break;
+ case 'Escape':
+ me.cancelEdit();
+ return;
+ case 'Backspace':
+ case 'Delete':
+ return;
+ default:
+ if (key.match(PVE.Utils.tagCharRegex)) {
+ return;
+ }
+ }
+ event.browserEvent.preventDefault();
+ event.browserEvent.stopPropagation();
+ },
+
+ beforeInput: function(event) {
+ let me = this;
+ me.updateLayout();
+ let tag = event.event.data ?? event.event.dataTransfer?.getData('text/plain');
+ if (!tag) {
+ return;
+ }
+ if (tag.match(PVE.Utils.tagCharRegex) === null) {
+ event.event.preventDefault();
+ event.event.stopPropagation();
+ }
+ },
+
+ onInput: function(event) {
+ let me = this;
+ me.picker.getStore().filter({
+ property: 'tag',
+ value: me.tagEl().innerHTML,
+ anyMatch: true,
+ });
+ },
+
+ cancelEdit: function(list, event) {
+ let me = this;
+ if (me.mode === 'inEdit') {
+ me.setTag(me.tag);
+ me.setMode('editable');
+ }
+ me.picker?.hide();
+ },
+
+
+ setTag: function(tag) {
+ let me = this;
+ let oldtag = me.tag;
+ me.tag = tag;
+ let rgb = PVE.Utils.tagOverrides[tag] ?? Proxmox.Utils.stringToRGB(tag);
+
+ let cls = Proxmox.Utils.getTextContrastClass(rgb);
+ let color = Proxmox.Utils.rgbToCss(rgb);
+ me.setUserCls(`proxmox-tag-${cls}`);
+ me.setStyle('background-color', color);
+ if (rgb.length > 3) {
+ let fgcolor = Proxmox.Utils.rgbToCss([rgb[3], rgb[4], rgb[5]]);
+
+ me.setStyle('color', fgcolor);
+ } else {
+ me.setStyle('color');
+ }
+ me.updateData();
+ if (oldtag !== tag) {
+ me.fireEvent('change', me, tag, oldtag);
+ }
+ },
+
+ updateData: function() {
+ let me = this;
+ if (me.destroying || me.destroyed) {
+ return;
+ }
+ me.update({
+ tag: me.tag,
+ iconCls: me.icons[me.mode],
+ });
+ },
+
+ tagEl: function() {
+ return this.el?.dom?.getElementsByTagName('span')?.[0];
+ },
+
+ listeners: {
+ mousedown: 'onMouseDown',
+ click: 'onClick',
+ focusleave: 'cancelEdit',
+ keydown: 'onKeyPress',
+ beforeInput: 'beforeInput',
+ input: 'onInput',
+ element: 'el',
+ scope: 'this',
+ },
+
+ initComponent: function() {
+ let me = this;
+
+ me.setTag(me.tag);
+ me.setMode(me.mode ?? 'normal');
+ me.callParent();
+ },
+
+ destroy: function() {
+ let me = this;
+ if (me.picker) {
+ Ext.destroy(me.picker);
+ }
+ me.callParent();
+ },
+});
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH manager v10 08/13] ui: add form/Tag
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 08/13] ui: add form/Tag Dominik Csapak
@ 2022-11-16 14:57 ` Thomas Lamprecht
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 14:57 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
For the subject: s!form/Tag!Proxmox.form.Tag component!
Am 15/11/2022 um 14:02 schrieb Dominik Csapak:
> +Ext.define('Proxmox.Tag', {
Would define it as 'Proxmox.form.Tag' matching the file hierarchy.
> + extend: 'Ext.Component',
> + alias: 'widget.pmxTag',
it's not wrong but it feels like there should be something more added to the
widget alias, but lacking a significantly better proposal tbh., pmxTagPicker
pmxTagDisplayEdit feel not really good either and it def. isn't a issue and
can stay as is too.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (17 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 08/13] ui: add form/Tag Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-16 15:00 ` Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 10/13] ui: {lxc, qemu}/Config: show Tags and make them editable Dominik Csapak
` (3 subsequent siblings)
22 siblings, 1 reply; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
this is a wrapper container for holding a list of (editable) tags
intended to be used in the lxc/qemu status toolbar
to add a new tag, we reuse the 'pmxTag' class, but overwrite some of
its behaviour and css classes so that it properly adds tags
also handles the drag/drop feature for the tags in the list
when done with editing (by clicking the checkmark), sends a 'change'
event with the new tags
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v9:
* listen to glable event and render list of tags new
www/css/ext6-pve.css | 23 ++-
www/manager6/Makefile | 1 +
www/manager6/form/TagEdit.js | 325 +++++++++++++++++++++++++++++++++++
3 files changed, 345 insertions(+), 4 deletions(-)
create mode 100644 www/manager6/form/TagEdit.js
diff --git a/www/css/ext6-pve.css b/www/css/ext6-pve.css
index daaffa6ec..4fc83a878 100644
--- a/www/css/ext6-pve.css
+++ b/www/css/ext6-pve.css
@@ -657,7 +657,8 @@ table.osds td:first-of-type {
padding-bottom: 0px;
}
-.pve-edit-tag > i {
+.pve-edit-tag > i,
+.pve-add-tag > i {
cursor: pointer;
font-size: 14px;
}
@@ -667,7 +668,8 @@ table.osds td:first-of-type {
cursor: grab;
}
-.pve-edit-tag > i.action {
+.pve-edit-tag > i.action,
+.pve-add-tag > i.action {
padding-left: 5px;
}
@@ -676,7 +678,9 @@ table.osds td:first-of-type {
}
.pve-edit-tag.editable span,
-.pve-edit-tag.inEdit span {
+.pve-edit-tag.inEdit span,
+.pve-add-tag.editable span,
+.pve-add-tag.inEdit span {
background-color: #ffffff;
border: 1px solid #a8a8a8;
color: #000;
@@ -685,6 +689,17 @@ table.osds td:first-of-type {
min-width: 2em;
}
-.pve-edit-tag.inEdit span {
+.pve-edit-tag.inEdit span,
+.pve-add-tag.inEdit span {
border: 1px solid #000;
}
+
+.pve-add-tag {
+ background-color: #d5d5d5 ! important;
+ color: #000000 ! important;
+}
+
+.pve-tag-inline-button {
+ cursor: pointer;
+ padding-left: 2px;
+}
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 21be8da8e..481c2bebc 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -76,6 +76,7 @@ JSSRC= \
form/TagColorGrid.js \
form/ListField.js \
form/Tag.js \
+ form/TagEdit.js \
grid/BackupView.js \
grid/FirewallAliases.js \
grid/FirewallOptions.js \
diff --git a/www/manager6/form/TagEdit.js b/www/manager6/form/TagEdit.js
new file mode 100644
index 000000000..7200e3c09
--- /dev/null
+++ b/www/manager6/form/TagEdit.js
@@ -0,0 +1,325 @@
+Ext.define('PVE.panel.TagEditContainer', {
+ extend: 'Ext.container.Container',
+ alias: 'widget.pveTagEditContainer',
+
+ layout: {
+ type: 'hbox',
+ align: 'stretch',
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ loadTags: function(tagstring = '', force = false) {
+ let me = this;
+ let view = me.getView();
+
+ if (me.oldTags === tagstring && !force) {
+ return;
+ }
+
+ view.suspendLayout = true;
+ me.forEachTag((tag) => {
+ view.remove(tag);
+ });
+ me.getViewModel().set('tagCount', 0);
+ let newtags = tagstring.split(/[;, ]/).filter((t) => !!t) || [];
+ newtags.forEach((tag) => {
+ me.addTag(tag);
+ });
+ view.suspendLayout = false;
+ view.updateLayout();
+ if (!force) {
+ me.oldTags = tagstring;
+ }
+ },
+
+ onRender: function(v) {
+ let me = this;
+ let view = me.getView();
+ view.dragzone = Ext.create('Ext.dd.DragZone', v.getEl(), {
+ getDragData: function(e) {
+ let source = e.getTarget('.handle');
+ if (!source) {
+ return undefined;
+ }
+ let sourceId = source.parentNode.id;
+ let cmp = Ext.getCmp(sourceId);
+ let ddel = document.createElement('div');
+ ddel.classList.add('proxmox-tags-full');
+ ddel.innerHTML = Proxmox.Utils.getTagElement(cmp.tag, PVE.Utils.tagOverrides);
+ let repairXY = Ext.fly(source).getXY();
+ cmp.setDisabled(true);
+ ddel.id = Ext.id();
+ return {
+ ddel,
+ repairXY,
+ sourceId,
+ };
+ },
+ onMouseUp: function(target, e, id) {
+ let cmp = Ext.getCmp(this.dragData.sourceId);
+ if (cmp && !cmp.isDestroyed) {
+ cmp.setDisabled(false);
+ }
+ },
+ getRepairXY: function() {
+ return this.dragData.repairXY;
+ },
+ beforeInvalidDrop: function(target, e, id) {
+ let cmp = Ext.getCmp(this.dragData.sourceId);
+ if (cmp && !cmp.isDestroyed) {
+ cmp.setDisabled(false);
+ }
+ },
+ });
+ view.dropzone = Ext.create('Ext.dd.DropZone', v.getEl(), {
+ getTargetFromEvent: function(e) {
+ return e.getTarget('.proxmox-tag-dark,.proxmox-tag-light');
+ },
+ getIndicator: function() {
+ if (!view.indicator) {
+ view.indicator = Ext.create('Ext.Component', {
+ floating: true,
+ html: '<i class="fa fa-long-arrow-up"></i>',
+ hidden: true,
+ shadow: false,
+ });
+ }
+ return view.indicator;
+ },
+ onContainerOver: function() {
+ this.getIndicator().setVisible(false);
+ },
+ notifyOut: function() {
+ this.getIndicator().setVisible(false);
+ },
+ onNodeOver: function(target, dd, e, data) {
+ let indicator = this.getIndicator();
+ indicator.setVisible(true);
+ indicator.alignTo(Ext.getCmp(target.id), 't50-bl', [-1, -2]);
+ return this.dropAllowed;
+ },
+ onNodeDrop: function(target, dd, e, data) {
+ this.getIndicator().setVisible(false);
+ let sourceCmp = Ext.getCmp(data.sourceId);
+ if (!sourceCmp) {
+ return;
+ }
+ sourceCmp.setDisabled(false);
+ let targetCmp = Ext.getCmp(target.id);
+ view.remove(sourceCmp, { destroy: false });
+ view.insert(view.items.indexOf(targetCmp), sourceCmp);
+ },
+ });
+ },
+
+ forEachTag: function(func) {
+ let me = this;
+ let view = me.getView();
+ view.items.each((field) => {
+ if (field.reference === 'addTagBtn') {
+ return false;
+ }
+ if (field.getXType() === 'pmxTag') {
+ func(field);
+ }
+ return true;
+ });
+ },
+
+ toggleEdit: function(cancel) {
+ let me = this;
+ let vm = me.getViewModel();
+ let editMode = !vm.get('editMode');
+ vm.set('editMode', editMode);
+
+ // get a current tag list for editing
+ if (editMode) {
+ PVE.Utils.updateUIOptions();
+ }
+
+ me.forEachTag((tag) => {
+ tag.setMode(editMode ? 'editable' : 'normal');
+ });
+
+ if (!vm.get('editMode')) {
+ let tags = [];
+ if (cancel) {
+ me.loadTags(me.oldTags, true);
+ } else {
+ me.forEachTag((cmp) => {
+ if (cmp.isVisible() && cmp.tag) {
+ tags.push(cmp.tag);
+ }
+ });
+ tags = tags.join(',');
+ if (me.oldTags !== tags) {
+ me.oldTags = tags;
+ me.getView().fireEvent('change', tags);
+ }
+ }
+ }
+ me.getView().updateLayout();
+ },
+
+ addTag: function(tag) {
+ let me = this;
+ let view = me.getView();
+ let vm = me.getViewModel();
+ let index = view.items.indexOf(me.lookup('addTagBtn'));
+ view.insert(index, {
+ xtype: 'pmxTag',
+ tag,
+ mode: vm.get('editMode') ? 'editable' : 'normal',
+ listeners: {
+ change: (field, newTag) => {
+ if (newTag === '') {
+ view.remove(field);
+ vm.set('tagCount', vm.get('tagCount') - 1);
+ }
+ },
+ },
+ });
+
+ vm.set('tagCount', vm.get('tagCount') + 1);
+ },
+
+ addTagClick: function(event) {
+ let me = this;
+ if (event.target.tagName === 'SPAN') {
+ me.lookup('addTagBtn').tagEl().innerHTML = '';
+ me.lookup('addTagBtn').updateLayout();
+ }
+ },
+
+ addTagMouseDown: function(event) {
+ let me = this;
+ if (event.target.tagName === 'I') {
+ let tag = me.lookup('addTagBtn').tagEl().innerHTML;
+ if (tag !== '') {
+ me.addTag(tag, true);
+ }
+ }
+ },
+
+ addTagChange: function(field, tag) {
+ let me = this;
+ if (tag !== '') {
+ me.addTag(tag, true);
+ }
+ field.tag = '';
+ },
+
+ cancelClick: function() {
+ this.toggleEdit(true);
+ },
+
+ editClick: function() {
+ this.toggleEdit(false);
+ },
+
+ init: function(view) {
+ let me = this;
+ if (view.tags) {
+ me.loadTags(view.tags);
+ }
+
+ me.mon(Ext.GlobalEvents, 'loadedUiOptions', () => {
+ me.loadTags(me.oldTags, true); // refresh tag colors
+ });
+ },
+ },
+
+ viewModel: {
+ data: {
+ tagCount: 0,
+ editMode: false,
+ },
+
+ formulas: {
+ hideNoTags: function(get) {
+ return get('editMode') || get('tagCount') !== 0;
+ },
+ editBtnHtml: function(get) {
+ let cls = get('editMode') ? 'check' : 'pencil';
+ let qtip = get('editMode') ? gettext('Apply Changes') : gettext('Edit Tags');
+ return `<i data-qtip="${qtip}" class="fa fa-${cls}"></i>`;
+ },
+ },
+ },
+
+ loadTags: function() {
+ return this.getController().loadTags(...arguments);
+ },
+
+ items: [
+ {
+ xtype: 'box',
+ bind: {
+ hidden: '{hideNoTags}',
+ },
+ html: gettext('No Tags'),
+ },
+ {
+ xtype: 'pmxTag',
+ reference: 'addTagBtn',
+ cls: 'pve-add-tag',
+ mode: 'editable',
+ tag: '',
+ tpl: `<span>${gettext('Add Tag')}</span><i class="action fa fa-plus-square"></i>`,
+ bind: {
+ hidden: '{!editMode}',
+ },
+ hidden: true,
+ onMouseDown: Ext.emptyFn, // prevent default behaviour
+ listeners: {
+ click: {
+ element: 'el',
+ fn: 'addTagClick',
+ },
+ mousedown: {
+ element: 'el',
+ fn: 'addTagMouseDown',
+ },
+ change: 'addTagChange',
+ },
+ },
+ {
+ xtype: 'box',
+ html: `<i data-qtip="${gettext('Cancel')}" class="fa fa-times"></i>`,
+ cls: 'pve-tag-inline-button',
+ hidden: true,
+ bind: {
+ hidden: '{!editMode}',
+ },
+ listeners: {
+ click: 'cancelClick',
+ element: 'el',
+ },
+ },
+ {
+ xtype: 'box',
+ cls: 'pve-tag-inline-button',
+ bind: {
+ html: '{editBtnHtml}',
+ },
+ listeners: {
+ click: 'editClick',
+ element: 'el',
+ },
+ },
+ ],
+
+ listeners: {
+ render: 'onRender',
+ },
+
+ destroy: function() {
+ let me = this;
+ Ext.destroy(me.dragzone);
+ Ext.destroy(me.dropzone);
+ Ext.destroy(me.indicator);
+ me.callParent();
+ },
+});
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js Dominik Csapak
@ 2022-11-16 15:00 ` Thomas Lamprecht
2022-11-16 15:02 ` Dominik Csapak
0 siblings, 1 reply; 44+ messages in thread
From: Thomas Lamprecht @ 2022-11-16 15:00 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
subject -> s!form/TagEdit.js!PVE.panel.TagEditContainer component!
Am 15/11/2022 um 14:02 schrieb Dominik Csapak:
> +Ext.define('PVE.panel.TagEditContainer', {
> + extend: 'Ext.container.Container',
> + alias: 'widget.pveTagEditContainer',
nit: slightly odd that this is pveABC while in the previous patch you used
pmxABC, I guess the former lifted in widget-toolkit initially ^^
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js
2022-11-16 15:00 ` Thomas Lamprecht
@ 2022-11-16 15:02 ` Dominik Csapak
0 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-16 15:02 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
On 11/16/22 16:00, Thomas Lamprecht wrote:
> subject -> s!form/TagEdit.js!PVE.panel.TagEditContainer component!
>
> Am 15/11/2022 um 14:02 schrieb Dominik Csapak:
>> +Ext.define('PVE.panel.TagEditContainer', {
>> + extend: 'Ext.container.Container',
>> + alias: 'widget.pveTagEditContainer',
>
> nit: slightly odd that this is pveABC while in the previous patch you used
> pmxABC, I guess the former lifted in widget-toolkit initially ^^
>
>
you're right, i'll change the pmxTag to pveTag since it lives in pve currently anyway...
^ permalink raw reply [flat|nested] 44+ messages in thread
* [pve-devel] [PATCH manager v10 10/13] ui: {lxc, qemu}/Config: show Tags and make them editable
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (18 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 11/13] ui: tree/ResourceTree: show Tags in tree Dominik Csapak
` (2 subsequent siblings)
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
add the tags in the status line, and add a button for adding new ones
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/lxc/Config.js | 32 ++++++++++++++++++++++++++++++--
www/manager6/qemu/Config.js | 31 +++++++++++++++++++++++++++++--
2 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 93f385db4..9b3017add 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -4,6 +4,8 @@ Ext.define('PVE.lxc.Config', {
onlineHelp: 'chapter_pct',
+ userCls: 'proxmox-tags-full',
+
initComponent: function() {
var me = this;
var vm = me.pveSelNode.data;
@@ -182,12 +184,33 @@ Ext.define('PVE.lxc.Config', {
],
});
+ let tagsContainer = Ext.create('PVE.panel.TagEditContainer', {
+ tags: vm.tags,
+ listeners: {
+ change: function(tags) {
+ Proxmox.Utils.API2Request({
+ url: base_url + '/config',
+ method: 'PUT',
+ params: {
+ tags,
+ },
+ success: function() {
+ me.statusStore.load();
+ },
+ failure: function(response) {
+ Ext.Msg.alert('Error', response.htmlStatus);
+ me.statusStore.load();
+ },
+ });
+ },
+ },
+ });
Ext.apply(me, {
title: Ext.String.format(gettext("Container {0} on node '{1}'"), vm.text, nodename),
hstateid: 'lxctab',
tbarSpacing: false,
- tbar: [statusTxt, '->', startBtn, shutdownBtn, migrateBtn, consoleBtn, moreBtn],
+ tbar: [statusTxt, tagsContainer, '->', startBtn, shutdownBtn, migrateBtn, consoleBtn, moreBtn],
defaults: { statusStore: me.statusStore },
items: [
{
@@ -344,10 +367,12 @@ Ext.define('PVE.lxc.Config', {
me.mon(me.statusStore, 'load', function(s, records, success) {
var status;
var lock;
+ var rec;
+
if (!success) {
status = 'unknown';
} else {
- var rec = s.data.get('status');
+ rec = s.data.get('status');
status = rec ? rec.data.value : 'unknown';
rec = s.data.get('template');
template = rec ? rec.data.value : false;
@@ -357,6 +382,9 @@ Ext.define('PVE.lxc.Config', {
statusTxt.update({ lock: lock });
+ rec = s.data.get('tags');
+ tagsContainer.loadTags(rec?.data?.value);
+
startBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status === 'running' || template);
shutdownBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status !== 'running');
me.down('#removeBtn').setDisabled(!caps.vms['VM.Allocate'] || status !== 'stopped');
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 9fe933dfc..2cd6d8567 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -3,6 +3,7 @@ Ext.define('PVE.qemu.Config', {
alias: 'widget.PVE.qemu.Config',
onlineHelp: 'chapter_virtual_machines',
+ userCls: 'proxmox-tags-full',
initComponent: function() {
var me = this;
@@ -219,11 +220,33 @@ Ext.define('PVE.qemu.Config', {
],
});
+ let tagsContainer = Ext.create('PVE.panel.TagEditContainer', {
+ tags: vm.tags,
+ listeners: {
+ change: function(tags) {
+ Proxmox.Utils.API2Request({
+ url: base_url + '/config',
+ method: 'PUT',
+ params: {
+ tags,
+ },
+ success: function() {
+ me.statusStore.load();
+ },
+ failure: function(response) {
+ Ext.Msg.alert('Error', response.htmlStatus);
+ me.statusStore.load();
+ },
+ });
+ },
+ },
+ });
+
Ext.apply(me, {
title: Ext.String.format(gettext("Virtual Machine {0} on node '{1}'"), vm.text, nodename),
hstateid: 'kvmtab',
tbarSpacing: false,
- tbar: [statusTxt, '->', resumeBtn, startBtn, shutdownBtn, migrateBtn, consoleBtn, moreBtn],
+ tbar: [statusTxt, tagsContainer, '->', resumeBtn, startBtn, shutdownBtn, migrateBtn, consoleBtn, moreBtn],
defaults: { statusStore: me.statusStore },
items: [
{
@@ -382,11 +405,12 @@ Ext.define('PVE.qemu.Config', {
var spice = false;
var xtermjs = false;
var lock;
+ var rec;
if (!success) {
status = qmpstatus = 'unknown';
} else {
- var rec = s.data.get('status');
+ rec = s.data.get('status');
status = rec ? rec.data.value : 'unknown';
rec = s.data.get('qmpstatus');
qmpstatus = rec ? rec.data.value : 'unknown';
@@ -399,6 +423,9 @@ Ext.define('PVE.qemu.Config', {
xtermjs = !!s.data.get('serial');
}
+ rec = s.data.get('tags');
+ tagsContainer.loadTags(rec?.data?.value);
+
if (template) {
return;
}
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 11/13] ui: tree/ResourceTree: show Tags in tree
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (19 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 10/13] ui: {lxc, qemu}/Config: show Tags and make them editable Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 12/13] ui: add tags to ResourceGrid and GlobalSearchField Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 13/13] ui: implement tag ordering from datacenter.cfg Dominik Csapak
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
and update the treenodes when the tags change.
since we change the vm node text (which we pass through to the config
panel), we have to change how we generate the text there slightly
(otherwise that would include the rendered tags)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/data/ResourceStore.js | 6 ++++++
www/manager6/lxc/Config.js | 4 +++-
www/manager6/qemu/Config.js | 4 +++-
www/manager6/tree/ResourceTree.js | 10 +++++++++-
4 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/www/manager6/data/ResourceStore.js b/www/manager6/data/ResourceStore.js
index c7b723060..b18f7dd8d 100644
--- a/www/manager6/data/ResourceStore.js
+++ b/www/manager6/data/ResourceStore.js
@@ -293,6 +293,12 @@ Ext.define('PVE.data.ResourceStore', {
sortable: true,
width: 100,
},
+ tags: {
+ header: gettext('Tags'),
+ type: 'string',
+ hidden: true,
+ sortable: true,
+ },
};
let fields = [];
diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 9b3017add..f33390513 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -206,8 +206,10 @@ Ext.define('PVE.lxc.Config', {
},
});
+ let vm_text = `${vm.vmid} (${vm.name})`;
+
Ext.apply(me, {
- title: Ext.String.format(gettext("Container {0} on node '{1}'"), vm.text, nodename),
+ title: Ext.String.format(gettext("Container {0} on node '{1}'"), vm_text, nodename),
hstateid: 'lxctab',
tbarSpacing: false,
tbar: [statusTxt, tagsContainer, '->', startBtn, shutdownBtn, migrateBtn, consoleBtn, moreBtn],
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 2cd6d8567..5c8fa620d 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -242,8 +242,10 @@ Ext.define('PVE.qemu.Config', {
},
});
+ let vm_text = `${vm.vmid} (${vm.name})`;
+
Ext.apply(me, {
- title: Ext.String.format(gettext("Virtual Machine {0} on node '{1}'"), vm.text, nodename),
+ title: Ext.String.format(gettext("Virtual Machine {0} on node '{1}'"), vm_text, nodename),
hstateid: 'kvmtab',
tbarSpacing: false,
tbar: [statusTxt, tagsContainer, '->', resumeBtn, startBtn, shutdownBtn, migrateBtn, consoleBtn, moreBtn],
diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index be90d4f7a..5c92d4128 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -5,6 +5,8 @@ Ext.define('PVE.tree.ResourceTree', {
extend: 'Ext.tree.TreePanel',
alias: ['widget.pveResourceTree'],
+ userCls: 'proxmox-tags-circle',
+
statics: {
typeDefaults: {
node: {
@@ -114,6 +116,8 @@ Ext.define('PVE.tree.ResourceTree', {
}
}
+ info.text += PVE.Utils.renderTags(info.tags, PVE.Utils.tagOverrides);
+
info.text = status + info.text;
},
@@ -226,6 +230,10 @@ Ext.define('PVE.tree.ResourceTree', {
let stateid = 'rid';
+ const changedFields = [
+ 'text', 'running', 'template', 'status', 'qmpstatus', 'hastate', 'lock', 'tags',
+ ];
+
let updateTree = function() {
store.suspendEvents();
@@ -261,7 +269,7 @@ Ext.define('PVE.tree.ResourceTree', {
}
// tree item has been updated
- for (const field of ['text', 'running', 'template', 'status', 'qmpstatus', 'hastate', 'lock']) {
+ for (const field of changedFields) {
if (item.data[field] !== olditem.data[field]) {
changed = true;
break;
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 12/13] ui: add tags to ResourceGrid and GlobalSearchField
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (20 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 11/13] ui: tree/ResourceTree: show Tags in tree Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 13/13] ui: implement tag ordering from datacenter.cfg Dominik Csapak
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
also allows to search for tags in the GlobalSearchField where each tag is
treated like a seperate field, so it weighs more if the user searches for
the exact string of a single tag
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
ui: ResourceGrid: render tags
with the 'full' styling
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/data/ResourceStore.js | 1 +
www/manager6/form/GlobalSearchField.js | 20 +++++++++++++++-----
www/manager6/grid/ResourceGrid.js | 1 +
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/www/manager6/data/ResourceStore.js b/www/manager6/data/ResourceStore.js
index b18f7dd8d..ed1f4699e 100644
--- a/www/manager6/data/ResourceStore.js
+++ b/www/manager6/data/ResourceStore.js
@@ -295,6 +295,7 @@ Ext.define('PVE.data.ResourceStore', {
},
tags: {
header: gettext('Tags'),
+ renderer: (value) => PVE.Utils.renderTags(value, PVE.Utils.tagOverrides),
type: 'string',
hidden: true,
sortable: true,
diff --git a/www/manager6/form/GlobalSearchField.js b/www/manager6/form/GlobalSearchField.js
index 267a480d7..8e815d4f5 100644
--- a/www/manager6/form/GlobalSearchField.js
+++ b/www/manager6/form/GlobalSearchField.js
@@ -15,6 +15,7 @@ Ext.define('PVE.form.GlobalSearchField', {
grid: {
xtype: 'gridpanel',
+ userCls: 'proxmox-tags-full',
focusOnToFront: false,
floating: true,
emptyText: Proxmox.Utils.noneText,
@@ -23,7 +24,7 @@ Ext.define('PVE.form.GlobalSearchField', {
scrollable: {
xtype: 'scroller',
y: true,
- x: false,
+ x: true,
},
store: {
model: 'PVEResources',
@@ -78,6 +79,11 @@ Ext.define('PVE.form.GlobalSearchField', {
text: gettext('Description'),
flex: 1,
dataIndex: 'text',
+ renderer: function(value, mD, rec) {
+ let overrides = PVE.Utils.tagOverrides;
+ let tags = PVE.Utils.renderTags(rec.data.tags, overrides);
+ return `${value}${tags}`;
+ },
},
{
text: gettext('Node'),
@@ -104,16 +110,20 @@ Ext.define('PVE.form.GlobalSearchField', {
'storage': ['type', 'pool', 'node', 'storage'],
'default': ['name', 'type', 'node', 'pool', 'vmid'],
};
- let fieldArr = fieldMap[item.data.type] || fieldMap.default;
+ let fields = fieldMap[item.data.type] || fieldMap.default;
+ let fieldArr = fields.map(field => item.data[field]?.toString().toLowerCase());
+ if (item.data.tags) {
+ let tags = item.data.tags.split(/[;, ]/);
+ fieldArr.push(...tags);
+ }
let filterWords = me.filterVal.split(/\s+/);
// all text is case insensitive and each split-out word is searched for separately.
// a row gets 1 point for every partial match, and and additional point for every exact match
let match = 0;
- for (let field of fieldArr) {
- let fieldValue = item.data[field]?.toString().toLowerCase();
- if (fieldValue === undefined) {
+ for (let fieldValue of fieldArr) {
+ if (fieldValue === undefined || fieldValue === "") {
continue;
}
for (let filterWord of filterWords) {
diff --git a/www/manager6/grid/ResourceGrid.js b/www/manager6/grid/ResourceGrid.js
index 29906a376..9376bcc26 100644
--- a/www/manager6/grid/ResourceGrid.js
+++ b/www/manager6/grid/ResourceGrid.js
@@ -7,6 +7,7 @@ Ext.define('PVE.grid.ResourceGrid', {
property: 'type',
direction: 'ASC',
},
+ userCls: 'proxmox-tags-full',
initComponent: function() {
let me = this;
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [pve-devel] [PATCH manager v10 13/13] ui: implement tag ordering from datacenter.cfg
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
` (21 preceding siblings ...)
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 12/13] ui: add tags to ResourceGrid and GlobalSearchField Dominik Csapak
@ 2022-11-15 13:02 ` Dominik Csapak
22 siblings, 0 replies; 44+ messages in thread
From: Dominik Csapak @ 2022-11-15 13:02 UTC (permalink / raw)
To: pve-devel
ui-options/datacenter.cfg return an 'ordering' option. parse that and
use it to order the tags when viewing.
when having that enabled, drag & drop when editing is disabled and the
tags will be inserted at the right place. when saving, the sorted
order will be written into the config
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v10
www/css/ext6-pve.css | 5 +++++
www/manager6/Utils.js | 7 +++++++
www/manager6/dc/OptionView.js | 17 +++++++++++++++++
www/manager6/form/TagEdit.js | 13 ++++++++++++-
4 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/www/css/ext6-pve.css b/www/css/ext6-pve.css
index 4fc83a878..b8c713c48 100644
--- a/www/css/ext6-pve.css
+++ b/www/css/ext6-pve.css
@@ -668,6 +668,11 @@ table.osds td:first-of-type {
cursor: grab;
}
+.hide-handles .pve-edit-tag > i.handle {
+ display: none;
+ padding-right: 0px;
+}
+
.pve-edit-tag > i.action,
.pve-add-tag > i.action {
padding-left: 5px;
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 2e819ce84..d34ac53ac 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1931,6 +1931,9 @@ Ext.define('PVE.Utils', {
let text = '';
if (tagstext) {
let tags = (tagstext.split(/[,; ]/) || []).filter(t => !!t);
+ if (PVE.Utils.shouldSortTags()) {
+ tags = tags.sort();
+ }
text += ' ';
tags.forEach((tag) => {
text += Proxmox.Utils.getTagElement(tag, overrides);
@@ -1939,6 +1942,10 @@ Ext.define('PVE.Utils', {
return text;
},
+ shouldSortTags: function() {
+ return PVE.UIOptions?.['tag-style']?.ordering === 'alphabetical';
+ },
+
tagCharRegex: /^[a-z0-9+_.-]$/i,
},
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index ef2989d6c..65d057036 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -323,6 +323,7 @@ Ext.define('PVE.dc.OptionView', {
let shape = value.shape;
let shapeText = PVE.Utils.tagTreeStyles[shape] ?? Proxmox.Utils.defaultText;
let txt = Ext.String.format(gettext("Tree Shape: {0}"), shapeText);
+ txt += `, ${Ext.String.format(gettext("Ordering: {0}"), value.ordering ?? 'config')}`;
if (Object.keys(colors).length > 0) {
txt += ', ';
}
@@ -361,6 +362,9 @@ Ext.define('PVE.dc.OptionView', {
if (values.shape) {
style.shape = values.shape;
}
+ if (values.ordering) {
+ style.ordering = values.ordering;
+ }
let value = PVE.Parser.printPropertyString(style);
if (value === '') {
return {
@@ -380,6 +384,19 @@ Ext.define('PVE.dc.OptionView', {
defaultValue: '__default__',
deleteEmpty: true,
},
+ {
+ name: 'ordering',
+ xtype: 'proxmoxKVComboBox',
+ fieldLabel: gettext('Ordering'),
+ comboItems: [
+ ['__default__', `${Proxmox.Utils.defaultText} (config)`],
+ ['config', 'config'],
+ ['alphabetical', 'alphabetical'],
+ ],
+ defaultValue: '__default__',
+ value: '__default__',
+ deleteEmpty: true,
+ },
{
xtype: 'displayfield',
fieldLabel: gettext('Color Overrides'),
diff --git a/www/manager6/form/TagEdit.js b/www/manager6/form/TagEdit.js
index 7200e3c09..7219845ae 100644
--- a/www/manager6/form/TagEdit.js
+++ b/www/manager6/form/TagEdit.js
@@ -37,6 +37,8 @@ Ext.define('PVE.panel.TagEditContainer', {
onRender: function(v) {
let me = this;
let view = me.getView();
+ view.toggleCls('hide-handles', PVE.Utils.shouldSortTags());
+
view.dragzone = Ext.create('Ext.dd.DragZone', v.getEl(), {
getDragData: function(e) {
let source = e.getTarget('.handle');
@@ -168,6 +170,14 @@ Ext.define('PVE.panel.TagEditContainer', {
let view = me.getView();
let vm = me.getViewModel();
let index = view.items.indexOf(me.lookup('addTagBtn'));
+ if (PVE.Utils.shouldSortTags()) {
+ index = view.items.findIndexBy(tagField => {
+ if (tagField.reference === 'addTagBtn') {
+ return true;
+ }
+ return tagField.tag >= tag;
+ }, 1);
+ }
view.insert(index, {
xtype: 'pmxTag',
tag,
@@ -226,7 +236,8 @@ Ext.define('PVE.panel.TagEditContainer', {
}
me.mon(Ext.GlobalEvents, 'loadedUiOptions', () => {
- me.loadTags(me.oldTags, true); // refresh tag colors
+ view.toggleCls('hide-handles', PVE.Utils.shouldSortTags());
+ me.loadTags(me.oldTags, true); // refresh tag colors and order
});
},
},
--
2.30.2
^ permalink raw reply [flat|nested] 44+ messages in thread