* [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-03 8:47 ` Thomas Lamprecht
2020-12-03 15:52 ` [pve-devel] applied: " Thomas Lamprecht
2020-12-02 9:21 ` [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools Dominik Csapak
` (9 subsequent siblings)
10 siblings, 2 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
moved and generalized from pve-storage, since we'll need it
in more places
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 4b445ea..bda236a 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -48,6 +48,7 @@ template_replace
safe_print
trim
extract_param
+extract_sensitive_params
file_copy
get_host_arch
O_PATH
@@ -807,6 +808,29 @@ sub extract_param {
return $res;
}
+sub extract_sensitive_params :prototype($$$) {
+ my ($param, $sensitive_list, $delete_list) = @_;
+
+ my $sensitive;
+
+ my %delete = map { $_ => 1 } ($delete_list || [])->@*;
+
+ # always extract sensitive keys, so they don't get written to the www-data readable scfg
+ for my $opt (@$sensitive_list) {
+ # First handle deletions as explicitly setting `undef`, afterwards new values may override
+ # it.
+ if (exists($delete{$opt})) {
+ $sensitive->{$opt} = undef;
+ }
+
+ if (defined(my $value = extract_param($param, $opt))) {
+ $sensitive->{$opt} = $value;
+ }
+ }
+
+ return $sensitive;
+}
+
# Note: we use this to wait until vncterm/spiceterm is ready
sub wait_for_vnc_port {
my ($port, $family, $timeout) = @_;
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
2020-12-02 9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
@ 2020-12-03 8:47 ` Thomas Lamprecht
2020-12-03 9:16 ` Wolfgang Bumiller
2020-12-03 15:52 ` [pve-devel] applied: " Thomas Lamprecht
1 sibling, 1 reply; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 8:47 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak; +Cc: Wolfgang Bumiller
On 02.12.20 10:21, Dominik Csapak wrote:
> moved and generalized from pve-storage, since we'll need it
> in more places
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 4b445ea..bda236a 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -48,6 +48,7 @@ template_replace
> safe_print
> trim
> extract_param
> +extract_sensitive_params
> file_copy
> get_host_arch
> O_PATH
> @@ -807,6 +808,29 @@ sub extract_param {
> return $res;
> }
>
can we have some short comment about what this does and when/why it can be useful here
> +sub extract_sensitive_params :prototype($$$) {
> + my ($param, $sensitive_list, $delete_list) = @_;
> +
> + my $sensitive;
I know auto vivification and such things exist, but I'd feel more comfortable
to set above explicitly to and empty hash {} .
> +
> + my %delete = map { $_ => 1 } ($delete_list || [])->@*;
> +
> + # always extract sensitive keys, so they don't get written to the www-data readable scfg
not only for scfg anymore, would drop that comment actually completely, that's rather
something for a method comment (see above)
> + for my $opt (@$sensitive_list) {
> + # First handle deletions as explicitly setting `undef`, afterwards new values may override
> + # it.
I know this is just copied, but there's no actual reason for setting to undef vs.
using delete encoded in that comment, it's just merely describing what one sees
when reading the code anyhow..
@Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
used undef vs. delete?
> + if (exists($delete{$opt})) {
> + $sensitive->{$opt} = undef;
> + }
> +
> + if (defined(my $value = extract_param($param, $opt))) {
> + $sensitive->{$opt} = $value;
> + }
> + }
> +
> + return $sensitive;
> +}
> +
> # Note: we use this to wait until vncterm/spiceterm is ready
> sub wait_for_vnc_port {
> my ($port, $family, $timeout) = @_;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
2020-12-03 8:47 ` Thomas Lamprecht
@ 2020-12-03 9:16 ` Wolfgang Bumiller
2020-12-03 9:35 ` Thomas Lamprecht
0 siblings, 1 reply; 21+ messages in thread
From: Wolfgang Bumiller @ 2020-12-03 9:16 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Dominik Csapak
> On 12/03/2020 9:47 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>
>
> On 02.12.20 10:21, Dominik Csapak wrote:
> > moved and generalized from pve-storage, since we'll need it
> > in more places
> >
> > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > ---
> > src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> > index 4b445ea..bda236a 100644
> > --- a/src/PVE/Tools.pm
> > +++ b/src/PVE/Tools.pm
> > @@ -48,6 +48,7 @@ template_replace
> > safe_print
> > trim
> > extract_param
> > +extract_sensitive_params
> > file_copy
> > get_host_arch
> > O_PATH
> > @@ -807,6 +808,29 @@ sub extract_param {
> > return $res;
> > }
> >
>
> can we have some short comment about what this does and when/why it can be useful here
>
> > +sub extract_sensitive_params :prototype($$$) {
> > + my ($param, $sensitive_list, $delete_list) = @_;
> > +
> > + my $sensitive;
>
> I know auto vivification and such things exist, but I'd feel more comfortable
> to set above explicitly to and empty hash {} .
>
> > +
> > + my %delete = map { $_ => 1 } ($delete_list || [])->@*;
> > +
> > + # always extract sensitive keys, so they don't get written to the www-data readable scfg
>
> not only for scfg anymore, would drop that comment actually completely, that's rather
> something for a method comment (see above)
>
> > + for my $opt (@$sensitive_list) {
> > + # First handle deletions as explicitly setting `undef`, afterwards new values may override
> > + # it.
>
> I know this is just copied, but there's no actual reason for setting to undef vs.
> using delete encoded in that comment, it's just merely describing what one sees
> when reading the code anyhow..
>
> @Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
> used undef vs. delete?
The update hooks in pve-storage don't get the deletion-list passed on as parameter,
so I translated into putting `undef` into the parameter list.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
2020-12-03 9:16 ` Wolfgang Bumiller
@ 2020-12-03 9:35 ` Thomas Lamprecht
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 9:35 UTC (permalink / raw)
To: Wolfgang Bumiller, Proxmox VE development discussion, Dominik Csapak
On 03.12.20 10:16, Wolfgang Bumiller wrote:
>> On 12/03/2020 9:47 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>> On 02.12.20 10:21, Dominik Csapak wrote:
>>> + for my $opt (@$sensitive_list) {
>>> + # First handle deletions as explicitly setting `undef`, afterwards new values may override
>>> + # it.
>>
>> I know this is just copied, but there's no actual reason for setting to undef vs.
>> using delete encoded in that comment, it's just merely describing what one sees
>> when reading the code anyhow..
>>
>> @Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
>> used undef vs. delete?
>
> The update hooks in pve-storage don't get the deletion-list passed on as parameter,
> so I translated into putting `undef` into the parameter list.
>
OK, then that would be a much better comment here as it gives an
actual reason, something like
# delete by setting to undef so that add/update hooks can know about it
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] applied: [PATCH common 1/1] tools: add extract_sensitive_params
2020-12-02 9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
2020-12-03 8:47 ` Thomas Lamprecht
@ 2020-12-03 15:52 ` Thomas Lamprecht
1 sibling, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 15:52 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
On 02.12.20 10:21, Dominik Csapak wrote:
> moved and generalized from pve-storage, since we'll need it
> in more places
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
>
applied, with some followups for the stuff commented, thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2021-01-28 16:31 ` [pve-devel] applied: " Thomas Lamprecht
2020-12-02 9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
we have a more general version there
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2/Storage/Config.pm | 29 ++++-------------------------
1 file changed, 4 insertions(+), 25 deletions(-)
diff --git a/PVE/API2/Storage/Config.pm b/PVE/API2/Storage/Config.pm
index 6e23427..00abd13 100755
--- a/PVE/API2/Storage/Config.pm
+++ b/PVE/API2/Storage/Config.pm
@@ -4,7 +4,7 @@ use strict;
use warnings;
use PVE::SafeSyslog;
-use PVE::Tools qw(extract_param);
+use PVE::Tools qw(extract_param extract_sensitive_params);
use PVE::Cluster qw(cfs_read_file cfs_write_file);
use PVE::Storage;
use PVE::Storage::Plugin;
@@ -112,28 +112,7 @@ __PACKAGE__->register_method ({
return &$api_storage_config($cfg, $param->{storage});
}});
-my sub extract_sensitive_params :prototype($$) {
- my ($param, $delete_list) = @_;
-
- my $sensitive;
-
- my %delete = map { $_ => 1 } ($delete_list || [])->@*;
-
- # always extract pw and keys, so they don't get written to the www-data readable scfg
- for my $opt (qw(password encryption-key)) {
- # First handle deletions as explicitly setting `undef`, afterwards new values may override
- # it.
- if (exists($delete{$opt})) {
- $sensitive->{$opt} = undef;
- }
-
- if (defined(my $value = extract_param($param, $opt))) {
- $sensitive->{$opt} = $value;
- }
- }
-
- return $sensitive;
-}
+my $sensitive_params = [qw(password encryption-key)];
__PACKAGE__->register_method ({
name => 'create',
@@ -182,7 +161,7 @@ __PACKAGE__->register_method ({
# fix me in section config create never need an empty entity.
delete $param->{nodes} if !$param->{nodes};
- my $sensitive = extract_sensitive_params($param, []);
+ my $sensitive = extract_sensitive_params($param, $sensitive_params, []);
my $plugin = PVE::Storage::Plugin->lookup($type);
my $opts = $plugin->check_config($storeid, $param, 1, 1);
@@ -282,7 +261,7 @@ __PACKAGE__->register_method ({
my $scfg = PVE::Storage::storage_config($cfg, $storeid);
$type = $scfg->{type};
- my $sensitive = extract_sensitive_params($param, $delete);
+ my $sensitive = extract_sensitive_params($param, $sensitive_params, $delete);
my $plugin = PVE::Storage::Plugin->lookup($type);
my $opts = $plugin->check_config($storeid, $param, 0, 1);
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-03 9:05 ` Thomas Lamprecht
2020-12-02 9:21 ` [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection Dominik Csapak
` (7 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
like we do in other apis of section configs (e.g. storage)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2/Cluster/MetricServer.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
index 9a14985e..ec3c7b75 100644
--- a/PVE/API2/Cluster/MetricServer.pm
+++ b/PVE/API2/Cluster/MetricServer.pm
@@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
my $d = $options->{$k} || die "no such option '$k'\n";
die "unable to delete required option '$k'\n" if !$d->{optional};
die "unable to delete fixed option '$k'\n" if $d->{fixed};
+ die "cannot set and delete property '$k' at the same time!\n"
+ if defined($opts->{$k});
delete $data->{$k};
}
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
2020-12-02 9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
@ 2020-12-03 9:05 ` Thomas Lamprecht
2020-12-04 11:30 ` Dominik Csapak
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 9:05 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
On 02.12.20 10:21, Dominik Csapak wrote:
> like we do in other apis of section configs (e.g. storage)
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> PVE/API2/Cluster/MetricServer.pm | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
> index 9a14985e..ec3c7b75 100644
> --- a/PVE/API2/Cluster/MetricServer.pm
> +++ b/PVE/API2/Cluster/MetricServer.pm
> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
> my $d = $options->{$k} || die "no such option '$k'\n";
> die "unable to delete required option '$k'\n" if !$d->{optional};
> die "unable to delete fixed option '$k'\n" if $d->{fixed};
> + die "cannot set and delete property '$k' at the same time!\n"
> + if defined($opts->{$k});
>
> delete $data->{$k};
> }
>
That counts as API change, strictly speaking.. For container and VMs we order
deletions before setting the value, and the one from container is the last
one which got some actual thoughts and discussion going on, IIRC, albeit not
to sure if about that exact behavior (as it was probably pre-existing).
It'd be good to at least decide for one behavior and try making that universal,
as else this is confusing..
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
2020-12-03 9:05 ` Thomas Lamprecht
@ 2020-12-04 11:30 ` Dominik Csapak
2020-12-04 11:57 ` Thomas Lamprecht
0 siblings, 1 reply; 21+ messages in thread
From: Dominik Csapak @ 2020-12-04 11:30 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
> On 02.12.20 10:21, Dominik Csapak wrote:
>> like we do in other apis of section configs (e.g. storage)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> PVE/API2/Cluster/MetricServer.pm | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>> index 9a14985e..ec3c7b75 100644
>> --- a/PVE/API2/Cluster/MetricServer.pm
>> +++ b/PVE/API2/Cluster/MetricServer.pm
>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>> my $d = $options->{$k} || die "no such option '$k'\n";
>> die "unable to delete required option '$k'\n" if !$d->{optional};
>> die "unable to delete fixed option '$k'\n" if $d->{fixed};
>> + die "cannot set and delete property '$k' at the same time!\n"
>> + if defined($opts->{$k});
>>
>> delete $data->{$k};
>> }
>>
>
> That counts as API change, strictly speaking..
ok, so should we leave it as is for now?
> For container and VMs we order
> deletions before setting the value, and the one from container is the last
> one which got some actual thoughts and discussion going on, IIRC, albeit not
> to sure if about that exact behavior (as it was probably pre-existing).
>
> It'd be good to at least decide for one behavior and try making that universal,
> as else this is confusing..
>
yeah that makes sense (though i think the ordering is irrelevant, since
even in container you cannot set and delete at the same time)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
2020-12-04 11:30 ` Dominik Csapak
@ 2020-12-04 11:57 ` Thomas Lamprecht
2020-12-04 12:45 ` Thomas Lamprecht
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-04 11:57 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
On 04.12.20 12:30, Dominik Csapak wrote:
> On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
>> On 02.12.20 10:21, Dominik Csapak wrote:
>>> like we do in other apis of section configs (e.g. storage)
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>> PVE/API2/Cluster/MetricServer.pm | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>>> index 9a14985e..ec3c7b75 100644
>>> --- a/PVE/API2/Cluster/MetricServer.pm
>>> +++ b/PVE/API2/Cluster/MetricServer.pm
>>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>>> my $d = $options->{$k} || die "no such option '$k'\n";
>>> die "unable to delete required option '$k'\n" if !$d->{optional};
>>> die "unable to delete fixed option '$k'\n" if $d->{fixed};
>>> + die "cannot set and delete property '$k' at the same time!\n"
>>> + if defined($opts->{$k});
>>> delete $data->{$k};
>>> }
>>>
>>
>> That counts as API change, strictly speaking..
>
> ok, so should we leave it as is for now?
>
>> For container and VMs we order
>> deletions before setting the value, and the one from container is the last
>> one which got some actual thoughts and discussion going on, IIRC, albeit not
>> to sure if about that exact behavior (as it was probably pre-existing).
>>
>> It'd be good to at least decide for one behavior and try making that universal,
>> as else this is confusing..
>>
>
> yeah that makes sense (though i think the ordering is irrelevant, since
> even in container you cannot set and delete at the same time)
>
I mean yes but no. It's ordered delete first, so it actually just behaves like
a set (update).
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
2020-12-04 11:57 ` Thomas Lamprecht
@ 2020-12-04 12:45 ` Thomas Lamprecht
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-04 12:45 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
On 04.12.20 12:57, Thomas Lamprecht wrote:
> On 04.12.20 12:30, Dominik Csapak wrote:
>> On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
>>> On 02.12.20 10:21, Dominik Csapak wrote:
>>>> like we do in other apis of section configs (e.g. storage)
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>> PVE/API2/Cluster/MetricServer.pm | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>>>> index 9a14985e..ec3c7b75 100644
>>>> --- a/PVE/API2/Cluster/MetricServer.pm
>>>> +++ b/PVE/API2/Cluster/MetricServer.pm
>>>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>>>> my $d = $options->{$k} || die "no such option '$k'\n";
>>>> die "unable to delete required option '$k'\n" if !$d->{optional};
>>>> die "unable to delete fixed option '$k'\n" if $d->{fixed};
>>>> + die "cannot set and delete property '$k' at the same time!\n"
>>>> + if defined($opts->{$k});
>>>> delete $data->{$k};
>>>> }
>>>>
>>>
>>> That counts as API change, strictly speaking..
>>
>> ok, so should we leave it as is for now?
>>
>>> For container and VMs we order
>>> deletions before setting the value, and the one from container is the last
>>> one which got some actual thoughts and discussion going on, IIRC, albeit not
>>> to sure if about that exact behavior (as it was probably pre-existing).
>>>
>>> It'd be good to at least decide for one behavior and try making that universal,
>>> as else this is confusing..
>>>
>>
>> yeah that makes sense (though i think the ordering is irrelevant, since
>> even in container you cannot set and delete at the same time)
>>
>
> I mean yes but no. It's ordered delete first, so it actually just behaves like
> a set (update).
>
I only looked at the internal update method and missed the check which errors
also for CTs in the API, so ignore this.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (2 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks Dominik Csapak
` (6 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
by providing the id or cfg to have better context in those methods
we will need that for influxdb http api
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/ExtMetric.pm | 4 ++--
PVE/Status/Plugin.pm | 14 +++++++-------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/PVE/ExtMetric.pm b/PVE/ExtMetric.pm
index 448d3925..3358c359 100644
--- a/PVE/ExtMetric.pm
+++ b/PVE/ExtMetric.pm
@@ -49,7 +49,7 @@ sub transactions_start {
foreach_plug($cfg, sub {
my ($plugin, $id, $plugin_config) = @_;
- my $connection = $plugin->_connect($plugin_config);
+ my $connection = $plugin->_connect($plugin_config, $id);
push @$transactions, {
connection => $connection,
@@ -72,7 +72,7 @@ sub transactions_finish {
my $flush_err = $@;
warn "$flush_err" if $flush_err;
- $plugin->_disconnect($txn->{connection});
+ $plugin->_disconnect($txn->{connection}, $txn->{cfg});
$txn->{connection} = undef;
# avoid log spam, already got a send error; disconnect would fail too
warn "disconnect failed: $@" if $@ && !$flush_err;
diff --git a/PVE/Status/Plugin.pm b/PVE/Status/Plugin.pm
index c3e0bec1..2fa223ca 100644
--- a/PVE/Status/Plugin.pm
+++ b/PVE/Status/Plugin.pm
@@ -70,12 +70,12 @@ sub parse_section_header {
}
sub _connect {
- my ($class, $cfg) = @_;
+ my ($class, $cfg, $id) = @_;
die "please implement inside plugin";
}
sub _disconnect {
- my ($class, $connection) = @_;
+ my ($class, $connection, $cfg) = @_;
$connection->close(); # overwrite if not a simple socket
}
@@ -115,22 +115,22 @@ sub flush_data {
return if !defined($txn->{data}) || $txn->{data} eq '';
my $data = delete $txn->{data};
- eval { $class->send($txn->{connection}, $data) };
+ eval { $class->send($txn->{connection}, $data, $txn->{cfg}) };
die "metrics send error '$txn->{id}': $@" if $@;
}
sub send {
- my ($class, $connection, $data) = @_;
+ my ($class, $connection, $data, $cfg) = @_;
defined($connection->send($data))
or die "failed to send metrics: $!\n";
}
sub test_connection {
- my ($class, $cfg) = @_;
+ my ($class, $cfg, $id) = @_;
- my $conn = $class->_connect($cfg);
- $class->_disconnect($conn);
+ my $conn = $class->_connect($cfg, $id);
+ $class->_disconnect($conn, $cfg);
}
sub update_node_status {
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (3 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api Dominik Csapak
` (5 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
like we do in it for the storage section configs
we will need this to store the token for influxdbs http api
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2/Cluster/MetricServer.pm | 39 +++++++++++++++++++++++++++-----
PVE/Status/Plugin.pm | 24 ++++++++++++++++++++
2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
index ec3c7b75..a6b99d4f 100644
--- a/PVE/API2/Cluster/MetricServer.pm
+++ b/PVE/API2/Cluster/MetricServer.pm
@@ -3,7 +3,7 @@ package PVE::API2::Cluster::MetricServer;
use warnings;
use strict;
-use PVE::Tools qw(extract_param);
+use PVE::Tools qw(extract_param extract_sensitive_params);
use PVE::Exception qw(raise_perm_exc raise_param_exc);
use PVE::JSONSchema qw(get_standard_option);
use PVE::RPCEnvironment;
@@ -152,6 +152,8 @@ __PACKAGE__->register_method ({
my $plugin = PVE::Status::Plugin->lookup($type);
my $id = extract_param($param, 'id');
+ my $sensitive_params = extract_sensitive_params($param, ['token'], []);
+
PVE::Cluster::cfs_lock_file('status.cfg', undef, sub {
my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
@@ -160,10 +162,20 @@ __PACKAGE__->register_method ({
my $opts = $plugin->check_config($id, $param, 1, 1);
- $plugin->test_connection($opts);
-
$cfg->{ids}->{$id} = $opts;
+ $plugin->on_add_hook($id, $opts, $sensitive_params);
+
+ eval {
+ $plugin->test_connection($opts, $id);
+ };
+
+ if (my $err = $@) {
+ eval { $plugin->on_delete_hook($id, $opts) };
+ warn "$@\n" if $@;
+ die $err;
+ }
+
PVE::Cluster::cfs_write_file('status.cfg', $cfg);
});
die $@ if $@;
@@ -190,6 +202,12 @@ __PACKAGE__->register_method ({
my $digest = extract_param($param, 'digest');
my $delete = extract_param($param, 'delete');
+ if ($delete) {
+ $delete = [PVE::Tools::split_list($delete)];
+ }
+
+ my $sensitive_params = extract_sensitive_params($param, ['token'], $delete);
+
PVE::Cluster::cfs_lock_file('status.cfg', undef, sub {
my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
@@ -201,15 +219,13 @@ __PACKAGE__->register_method ({
my $plugin = PVE::Status::Plugin->lookup($data->{type});
my $opts = $plugin->check_config($id, $param, 0, 1);
- $plugin->test_connection($opts);
-
for my $k (keys %$opts) {
$data->{$k} = $opts->{$k};
}
if ($delete) {
my $options = $plugin->private()->{options}->{$data->{type}};
- for my $k (PVE::Tools::split_list($delete)) {
+ for my $k (@$delete) {
my $d = $options->{$k} || die "no such option '$k'\n";
die "unable to delete required option '$k'\n" if !$d->{optional};
die "unable to delete fixed option '$k'\n" if $d->{fixed};
@@ -220,6 +236,10 @@ __PACKAGE__->register_method ({
}
}
+ $plugin->on_update_hook($id, $data, $sensitive_params);
+
+ $plugin->test_connection($opts, $id);
+
PVE::Cluster::cfs_write_file('status.cfg', $cfg);
});
die $@ if $@;
@@ -253,6 +273,13 @@ __PACKAGE__->register_method ({
my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
my $id = $param->{id};
+
+ my $plugin_cfg = $cfg->{ids}->{$id};
+
+ my $plugin = PVE::Status::Plugin->lookup($plugin_cfg->{type});
+
+ $plugin->on_delete_hook($id, $plugin_cfg);
+
delete $cfg->{ids}->{$id};
PVE::Cluster::cfs_write_file('status.cfg', $cfg);
});
diff --git a/PVE/Status/Plugin.pm b/PVE/Status/Plugin.pm
index 2fa223ca..fae7a0f0 100644
--- a/PVE/Status/Plugin.pm
+++ b/PVE/Status/Plugin.pm
@@ -153,4 +153,28 @@ sub update_storage_status {
die "please implement inside plugin";
}
+sub on_add_hook {
+ my ($class, $id, $opts, $sensitive_opts) = @_;
+
+ # implement in subclass
+
+ return undef;
+}
+
+sub on_update_hook {
+ my ($class, $id, $opts, $sensitive_opts) = @_;
+
+ # implement in subclass
+
+ return undef;
+}
+
+sub on_delete_hook {
+ my ($class, $id, $opts) = @_;
+
+ # implement in subclass
+
+ return undef;
+}
+
1;
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (4 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment Dominik Csapak
` (4 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
needs an organization/bucket (previously db) and an optional token
the http client does not fit exactly in the connect/send/disconnect
scheme, so it simply creates a request in 'connect',
does the actual http connection in 'send' and nothing in 'disconnect'
max-body-size is set to 25.000.000 bytes by default (the influxdb default)
and the timeout to 1 second (same as default graphite tcp timeout)
the token (if given) gets saved in /etc/pve/priv/metricserver/$ID.pw
it is optional, because the 1.8.x compatibility api does not need
authentication (in contrast to influxdb 2.x)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/Status/InfluxDB.pm | 215 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 207 insertions(+), 8 deletions(-)
diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
index 6663579f..f16af486 100644
--- a/PVE/Status/InfluxDB.pm
+++ b/PVE/Status/InfluxDB.pm
@@ -6,6 +6,8 @@ use warnings;
use POSIX qw(isnan isinf);
use Scalar::Util 'looks_like_number';
use IO::Socket::IP;
+use LWP::UserAgent;
+use HTTP::Request;
use PVE::SafeSyslog;
@@ -24,12 +26,51 @@ sub type {
return 'influxdb';
}
+sub properties {
+ return {
+ organization => {
+ description => "The influxdb organization. Only necessary when using the http v2 api. ".
+ "Has no meaning when using v2 compatibility api.",
+ type => 'string',
+ optional => 1,
+ },
+ bucket => {
+ description => "The influxdb bucket/db. Only necessary when using the http v2 api.",
+ type => 'string',
+ optional => 1,
+ },
+ token => {
+ description => "The influxdb access token. Only necessary when using the http v2 api. ".
+ "If the v2 compatibility api is used, use 'user:password' instead.",
+ type => 'string',
+ optional => 1,
+ },
+ influxdbproto => {
+ type => 'string',
+ enum => ['udp', 'http', 'https'],
+ default => 'udp',
+ optional => 1,
+ },
+ 'max-body-size' => {
+ description => "Influxdb max-body-size. Requests are batched up to this size.",
+ type => 'integer',
+ minimum => 1,
+ default => 25_000_000,
+ }
+ };
+}
sub options {
return {
server => {},
port => {},
mtu => { optional => 1 },
disable => { optional => 1 },
+ organization => { optional => 1},
+ bucket => { optional => 1},
+ token => { optional => 1},
+ influxdbproto => { optional => 1},
+ timeout => { optional => 1},
+ 'max-body-size' => { optional => 1 },
};
}
@@ -84,21 +125,106 @@ sub update_storage_status {
build_influxdb_payload($class, $txn, $data, $ctime, $object);
}
-sub _connect {
+sub _send_batch_size {
my ($class, $cfg) = @_;
+ my $proto = $cfg->{influxdbproto} // 'udp';
+ if ($proto ne 'udp') {
+ return $cfg->{'max-body-size'} // 25_000_000;
+ }
+
+ return $class->SUPER::_send_batch_size($cfg);
+}
+
+sub send {
+ my ($class, $connection, $data, $cfg) = @_;
+
+ my $proto = $cfg->{influxdbproto} // 'udp';
+ if ($proto eq 'udp') {
+ return $class->SUPER::send($connection, $data, $cfg);
+ } elsif ($proto =~ m/^https?$/) {
+ my $ua = LWP::UserAgent->new();
+ $ua->timeout($cfg->{timeout} // 1);
+ $connection->content($data);
+ my $response = $ua->request($connection);
+
+ if (!$response->is_success) {
+ my $err = $response->status_line;
+ die "$err\n";
+ }
+ } else {
+ die "invalid protocol\n";
+ }
+
+ return;
+}
+
+sub _disconnect {
+ my ($class, $connection, $cfg) = @_;
+ my $proto = $cfg->{influxdbproto} // 'udp';
+ if ($proto eq 'udp') {
+ return $class->SUPER::_disconnect($connection, $cfg);
+ }
+
+ return;
+}
+
+sub _connect {
+ my ($class, $cfg, $id) = @_;
my $host = $cfg->{server};
my $port = $cfg->{port};
+ my $proto = $cfg->{influxdbproto} // 'udp';
+
+ if ($proto eq 'udp') {
+ my $socket = IO::Socket::IP->new(
+ PeerAddr => $host,
+ PeerPort => $port,
+ Proto => 'udp',
+ ) || die "couldn't create influxdb socket [$host]:$port - $@\n";
+
+ $socket->blocking(0);
+
+ return $socket;
+ } elsif ($proto =~ m/^https?$/) {
+ my $token = get_credentials($id);
+ my $org = $cfg->{organization} // 'proxmox';
+ my $bucket = $cfg->{bucket} // 'proxmox';
+ my $url = "${proto}://${host}:${port}/api/v2/write?org=${org}&bucket=${bucket}";
+
+ my $req = HTTP::Request->new(POST => $url);
+ if (defined($token)) {
+ $req->header( "Authorization", "Token $token");
+ }
- my $socket = IO::Socket::IP->new(
- PeerAddr => $host,
- PeerPort => $port,
- Proto => 'udp',
- ) || die "couldn't create influxdb socket [$host]:$port - $@\n";
+ return $req;
+ }
+
+ die "cannot connect to influxdb: invalid protocol\n";
+}
- $socket->blocking(0);
+sub test_connection {
+ my ($class, $cfg, $id) = @_;
+
+ my $proto = $cfg->{influxdbproto} // 'udp';
+ if ($proto eq 'udp') {
+ return $class->SUPER::test_connection($cfg, $id);
+ } elsif ($proto =~ m/^https?$/) {
+ my $host = $cfg->{server};
+ my $port = $cfg->{port};
+ my $url = "${proto}://${host}:${port}/health";
+ my $ua = LWP::UserAgent->new();
+ $ua->timeout($cfg->{timeout} // 1);
+ my $response = $ua->get($url);
+
+ if (!$response->is_success) {
+ my $err = $response->status_line;
+ die "$err\n";
+ }
+ } else {
+ die "invalid protocol\n";
+ }
- return $socket;
+ return;
}
sub build_influxdb_payload {
@@ -177,4 +303,77 @@ sub prepare_value {
return $value;
}
+my $priv_dir = "/etc/pve/priv/metricserver";
+
+sub cred_file_name {
+ my ($id) = @_;
+ return "${priv_dir}/${id}.pw";
+}
+
+sub delete_credentials {
+ my ($id) = @_;
+
+ if (my $cred_file = cred_file_name($id)) {
+ unlink($cred_file)
+ or warn "removing influxdb credentials file '$cred_file' failed: $!\n";
+ }
+
+ return;
+}
+
+sub set_credentials {
+ my ($id, $token) = @_;
+
+ my $cred_file = cred_file_name($id);
+
+ mkdir $priv_dir;
+
+ PVE::Tools::file_set_contents($cred_file, "$token");
+}
+
+sub get_credentials {
+ my ($id) = @_;
+
+ my $cred_file = cred_file_name($id);
+
+ return PVE::Tools::file_get_contents($cred_file);
+}
+
+sub on_add_hook {
+ my ($class, $id, $opts, $sensitive_opts) = @_;
+
+ my $token = $sensitive_opts->{token};
+
+ if (defined($token)) {
+ set_credentials($id, $token);
+ } else {
+ delete_credenetials($id);
+ }
+
+ return undef;
+}
+
+sub on_update_hook {
+ my ($class, $id, $opts, $sensitive_opts) = @_;
+ return if !exists($sensitive_opts->{token});
+
+ my $token = $sensitive_opts->{token};
+ if (defined($token)) {
+ set_credentials($id, $token);
+ } else {
+ delete_credenetials($id);
+ }
+
+ return undef;
+}
+
+sub on_delete_hook {
+ my ($class, $id, $opts) = @_;
+
+ delete_credentials($id);
+
+ return undef;
+}
+
+
1;
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (5 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api Dominik Csapak
` (3 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
we already have that information in the reference docs, no need to
have it here as well
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/Status/InfluxDB.pm | 7 -------
1 file changed, 7 deletions(-)
diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
index f16af486..9ecce235 100644
--- a/PVE/Status/InfluxDB.pm
+++ b/PVE/Status/InfluxDB.pm
@@ -13,13 +13,6 @@ use PVE::SafeSyslog;
use PVE::Status::Plugin;
-# example config (/etc/pve/status.cfg)
-#influxdb:
-# server test
-# port 8089
-# disable 0
-#
-
use base('PVE::Status::Plugin');
sub type {
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (6 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows Dominik Csapak
` (2 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
and en/disable them accordingly to the selected mode
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/dc/MetricServerView.js | 101 ++++++++++++++++++++++++++--
1 file changed, 96 insertions(+), 5 deletions(-)
diff --git a/www/manager6/dc/MetricServerView.js b/www/manager6/dc/MetricServerView.js
index e30ea14e..dfec8c03 100644
--- a/www/manager6/dc/MetricServerView.js
+++ b/www/manager6/dc/MetricServerView.js
@@ -174,6 +174,13 @@ Ext.define('PVE.dc.InfluxDBEdit', {
subject: 'InfluxDB',
+ cbindData: function() {
+ let me = this;
+ me.callParent();
+ me.tokenEmptyText = me.isCreate ? '' : gettext('unchanged');
+ return {};
+ },
+
items: [
{
xtype: 'inputpanel',
@@ -209,6 +216,41 @@ Ext.define('PVE.dc.InfluxDBEdit', {
fieldLabel: gettext('Server'),
allowBlank: false,
},
+ {
+ xtype: 'proxmoxintegerfield',
+ name: 'port',
+ fieldLabel: gettext('Port'),
+ value: 8089,
+ minValue: 1,
+ maximum: 65536,
+ allowBlank: false,
+ },
+ {
+ xtype: 'proxmoxKVComboBox',
+ name: 'influxdbproto',
+ fieldLabel: gettext('Protocol'),
+ value: '__default__',
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ comboItems: [
+ ['__default__', 'UDP'],
+ ['http', 'HTTP'],
+ ['https', 'HTTPS'],
+ ],
+ listeners: {
+ change: function(field, value) {
+ let me = this;
+ let isUdp = value !== 'http' && value !== 'https';
+ me.up('inputpanel').down('field[name=organization]').setDisabled(isUdp);
+ me.up('inputpanel').down('field[name=bucket]').setDisabled(isUdp);
+ me.up('inputpanel').down('field[name=token]').setDisabled(isUdp);
+ me.up('inputpanel').down('field[name=mtu]').setDisabled(!isUdp);
+ me.up('inputpanel').down('field[name=timeout]').setDisabled(isUdp);
+ me.up('inputpanel').down('field[name=max-body-size]').setDisabled(isUdp);
+ },
+ },
+ },
],
column2: [
@@ -220,18 +262,67 @@ Ext.define('PVE.dc.InfluxDBEdit', {
uncheckedValue: 0,
checked: true,
},
+ {
+ xtype: 'proxmoxtextfield',
+ name: 'organization',
+ fieldLabel: gettext('Organization'),
+ emptyText: 'proxmox',
+ disabled: true,
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
+ {
+ xtype: 'proxmoxtextfield',
+ name: 'bucket',
+ fieldLabel: gettext('Bucket'),
+ emptyText: 'proxmox',
+ disabled: true,
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
+ {
+ xtype: 'proxmoxtextfield',
+ name: 'token',
+ fieldLabel: gettext('Token'),
+ disabled: true,
+ allowBlank: true,
+ deleteEmpty: false,
+ submitEmpty: false,
+ cbind: {
+ disabled: '{!isCreate}',
+ emptyText: '{tokenEmptyText}',
+ },
+ },
+ ],
+
+ advancedColumn1: [
{
xtype: 'proxmoxintegerfield',
- name: 'port',
- fieldLabel: gettext('Port'),
- value: 8089,
+ name: 'timeout',
+ fieldLabel: gettext('Timeout (s)'),
+ disabled: true,
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
minValue: 1,
- maximum: 65536,
- allowBlank: false,
+ emptyText: 1,
},
],
advancedColumn2: [
+ {
+ xtype: 'proxmoxintegerfield',
+ name: 'max-body-size',
+ fieldLabel: gettext('Batch Size (b)'),
+ minValue: 1,
+ emptyText: '25000000',
+ submitEmpty: false,
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
{
xtype: 'proxmoxintegerfield',
name: 'mtu',
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (7 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2020-12-02 9:21 ` [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api Dominik Csapak
2021-01-28 16:36 ` [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Thomas Lamprecht
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/dc/MetricServerView.js | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/www/manager6/dc/MetricServerView.js b/www/manager6/dc/MetricServerView.js
index dfec8c03..edb40cc5 100644
--- a/www/manager6/dc/MetricServerView.js
+++ b/www/manager6/dc/MetricServerView.js
@@ -172,6 +172,8 @@ Ext.define('PVE.dc.InfluxDBEdit', {
extend: 'PVE.dc.MetricServerBaseEdit',
mixins: ['Proxmox.Mixin.CBind'],
+ onlineHelp: 'metric_server_influxdb',
+
subject: 'InfluxDB',
cbindData: function() {
@@ -343,6 +345,8 @@ Ext.define('PVE.dc.GraphiteEdit', {
extend: 'PVE.dc.MetricServerBaseEdit',
mixins: ['Proxmox.Mixin.CBind'],
+ onlineHelp: 'metric_server_graphite',
+
subject: 'Graphite',
items: [
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (8 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows Dominik Csapak
@ 2020-12-02 9:21 ` Dominik Csapak
2021-01-28 16:36 ` [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Thomas Lamprecht
10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02 9:21 UTC (permalink / raw)
To: pve-devel
and mention that 1.8.x has a compatible api (and their differences)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
pve-external-metric-server.adoc | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/pve-external-metric-server.adoc b/pve-external-metric-server.adoc
index 783e72c..eace999 100644
--- a/pve-external-metric-server.adoc
+++ b/pve-external-metric-server.adoc
@@ -56,3 +56,21 @@ Here is an example configuration for influxdb (on your influxdb server):
With this configuration, your server listens on all IP addresses on port 8089,
and writes the data in the *proxmox* database
+
+Alternatively, the plugin can be configured to use the http(s) API of InfluxDB 2.x.
+InfluxDB 1.8.x does contain a forwards compatible API endpoint for this v2 API.
+
+To use it, set 'influxdbproto' to 'http' or 'https' (depending on your configuration).
+By default, {pve} uses the organization 'proxmox' and the bucket/db 'proxmox'
+(They can be set with the configuration 'organization' and 'bucket' respectively).
+
+Since InfluxDB's v2 API is only available with authentication, you have
+to generate a token that can write into the correct bucket and set it.
+
+In the v2 compatible API of 1.8.x, you can use 'user:password' as token
+(if required), and can omit the 'organization' since that has no meaning in InfluxDB 1.x.
+
+You can also set the HTTP Timeout (default is 1s) with the 'timeout' setting,
+as well as the maximum batch size (default 25000000 bytes) with the
+'max-body-size' setting (this corresponds to the InfluxDB setting with the
+same name).
--
2.20.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin
2020-12-02 9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
` (9 preceding siblings ...)
2020-12-02 9:21 ` [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api Dominik Csapak
@ 2021-01-28 16:36 ` Thomas Lamprecht
10 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2021-01-28 16:36 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
On 02.12.20 10:21, Dominik Csapak wrote:
> this series adds the possibility to write influxdb data via their
> v2 api. For influxdb 1.x there is a forwards compatible api since 1.8.0
>
> i am not so sure about how i integrated the http api part into the
> existing influxdb plugin, another alternative would be to have
> a new InfluxDB2 Plugin that does only http(s).
> this way i could omit the permanent 'cfg->proto' check, but i am not
> sure if we want to have a new plugin for this..
>
> manager/storage patches depend on common, but the storage patch is not needed for
> it to work
>
> pve-common:
>
> Dominik Csapak (1):
> tools: add extract_sensitive_params
>
> src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> pve-storage:
>
> Dominik Csapak (1):
> api: storage/config: use extract_sensitive_params from tools
>
> PVE/API2/Storage/Config.pm | 29 ++++-------------------------
> 1 file changed, 4 insertions(+), 25 deletions(-)
>
> pve-manager:
>
> Dominik Csapak (7):
> api: cluster/metricserver: prevent simultaneosly setting and deleting
> of property
> status/plugin: extend send/_connect/_disconnect/test_connection
> status/plugin: extend with add/update/delete hooks
> status/influxdb: implement influxdb 2.x http api
> status/influxdb: remove unnecessary comment
> ui: add necessary fields for influxdb http api
> ui: dc/MetricServerView: add onlineHelp to edit windows
>
> PVE/API2/Cluster/MetricServer.pm | 41 ++++-
> PVE/ExtMetric.pm | 4 +-
> PVE/Status/InfluxDB.pm | 222 ++++++++++++++++++++++++++--
> PVE/Status/Plugin.pm | 38 ++++-
> www/manager6/dc/MetricServerView.js | 105 ++++++++++++-
> 5 files changed, 375 insertions(+), 35 deletions(-)
>
> pve-docs:
>
> Dominik Csapak (1):
> external metrics server: extend docs to explain http api
>
> pve-external-metric-server.adoc | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
applied remaining parts of the series, thanks!
FYI: Had to resolve a small merge conflict due to context change in the
"status/plugin: extend send/_connect/_disconnect/test_connection"
commit
^ permalink raw reply [flat|nested] 21+ messages in thread