On March 31, 2020 12:08 pm, Wolfgang Link wrote:
> Signed-off-by: Wolfgang Link <w.l...@proxmox.com>
> ---
> PVE/API2/ACME.pm | 16 +++++------
> PVE/NodeConfig.pm | 67 +++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 73 insertions(+), 10 deletions(-)
>
> diff --git a/PVE/API2/ACME.pm b/PVE/API2/ACME.pm
> index 8bd6a924..ba25d153 100644
> --- a/PVE/API2/ACME.pm
> +++ b/PVE/API2/ACME.pm
> @@ -46,9 +46,9 @@ __PACKAGE__->register_method ({
> }});
>
> my $order_certificate = sub {
> - my ($acme, $domains) = @_;
> + my ($acme, $acme_node_config) = @_;
> print "Placing ACME order\n";
> - my ($order_url, $order) = $acme->new_order($domains);
> + my ($order_url, $order) = $acme->new_order($acme_node_config->{domains});
so here we just need the domains
> print "Order URL: $order_url\n";
> for my $auth_url (@{$order->{authorizations}}) {
> print "\nGetting authorization details from '$auth_url'\n";
> @@ -57,7 +57,7 @@ my $order_certificate = sub {
> print "... already validated!\n";
> } else {
> print "... pending!\n";
> - my $validation = PVE::ACME::setup($acme, $auth);
> + my $validation = PVE::ACME::setup($acme, $auth, $acme_node_config);
and here we actually want to pass in the plugin ID, domain and possibly
alias for one domain/auth.
so this could be vastly simplified, if we just pass in a hash
{
$domain1 => {
plugin => 'mydnspluginid',
alias => 'myalias.com',
},
$domain2 => {
plugin => 'default',
}
}
to $order_certificate, which can then pass
keys %$hash (when it needs the list of domains)
$hash (when it needs the associated plugin info for PVE::ACME::Setup,
although IMHO that could as well be inlined here altogether)
>
> print "Triggering validation\n";
> eval {
> @@ -166,7 +166,7 @@ __PACKAGE__->register_method ({
> my $node_config = PVE::NodeConfig::load_config($node);
> raise("ACME settings in node configuration are missing!", 400)
> if !$node_config || !$node_config->{acme};
> - my $acme_node_config =
> PVE::NodeConfig::parse_acme($node_config->{acme});
> + my $acme_node_config = PVE::NodeConfig::parse_acme($node_config);
> raise("ACME domain list in node configuration is missing!", 400)
> if !$acme_node_config;
>
> @@ -186,7 +186,7 @@ __PACKAGE__->register_method ({
> print "Loading ACME account details\n";
> $acme->load();
>
> - my ($cert, $key) = $order_certificate->($acme,
> $acme_node_config->{domains});
> + my ($cert, $key) = $order_certificate->($acme, $acme_node_config);
>
> my $code = sub {
> print "Setting pveproxy certificate and key\n";
> @@ -240,7 +240,7 @@ __PACKAGE__->register_method ({
> my $node_config = PVE::NodeConfig::load_config($node);
> raise("ACME settings in node configuration are missing!", 400)
> if !$node_config || !$node_config->{acme};
> - my $acme_node_config =
> PVE::NodeConfig::parse_acme($node_config->{acme});
> + my $acme_node_config = PVE::NodeConfig::parse_acme($node_config);
> raise("ACME domain list in node configuration is missing!", 400)
> if !$acme_node_config;
>
> @@ -262,7 +262,7 @@ __PACKAGE__->register_method ({
> print "Loading ACME account details\n";
> $acme->load();
>
> - my ($cert, $key) = $order_certificate->($acme,
> $acme_node_config->{domains});
> + my ($cert, $key) = $order_certificate->($acme, $acme_node_config);
>
> my $code = sub {
> print "Setting pveproxy certificate and key\n";
> @@ -306,7 +306,7 @@ __PACKAGE__->register_method ({
> my $node_config = PVE::NodeConfig::load_config($node);
> raise("ACME settings in node configuration are missing!", 400)
> if !$node_config || !$node_config->{acme};
> - my $acme_node_config =
> PVE::NodeConfig::parse_acme($node_config->{acme});
> + my $acme_node_config = PVE::NodeConfig::parse_acme($node_config);
I am not so happy about this, usually when we have a 'parse_foo' method
and a 'foo' config key, those are a 1:1 match for parsing the property
string + some extra stuff. in this case, it parses multiple related
options from a hash, and converts them. maybe we could rename it to
"get_acme_config" or something similar to make it a bit more obvious
that this is more than just our regular parser? see below for some more
ideas..
> raise("ACME domain list in node configuration is missing!", 400)
> if !$acme_node_config;
>
> diff --git a/PVE/NodeConfig.pm b/PVE/NodeConfig.pm
> index 6a75ee32..78827ded 100644
> --- a/PVE/NodeConfig.pm
> +++ b/PVE/NodeConfig.pm
> @@ -8,6 +8,7 @@ use Storable qw(dclone);
> use PVE::CertHelpers;
> use PVE::JSONSchema qw(get_standard_option);
> use PVE::Tools qw(file_get_contents file_set_contents lock_file);
> +use PVE::INotify;
>
> # regitster up to 20 domain names
> my $MAXDOMAINS = 20;
> @@ -115,6 +116,7 @@ $acmedesc->{domains} = {
> format => 'pve-acme-domain-list',
> optional => 1,
> };
> +$acmedesc->{domain}->{optional} = 1;
> PVE::JSONSchema::register_format('pve-acme-node-conf', $acmedesc);
>
> $confdesc->{acme} = {
> @@ -219,18 +221,79 @@ sub write_node_config {
> return $raw;
> }
>
> +my $convert_domains = sub {
since this does a read-modify-write cycle, it needs to be called under
lock_config!
> + my ($node, $conf) = @_;
> +
> + $conf = load_config($node);
> +
> + my $acme = PVE::JSONSchema::parse_property_string($acmedesc,
> $conf->{acme});
> + my $domainstring = delete $acme->{domains};
> + die "No Domains to convert found.\n" if !defined($domainstring);
> + # Extract domain list
> + my @domains = ( PVE::Tools::split_list($domainstring) );
> +
> + $acme->{domain} = $domains[0];
> + $conf->{acme} = PVE::JSONSchema::print_property_string($acme, $acmedesc);
> +
> + for my $i (1..$MAXDOMAINS) {
> + last if !defined $domains[$i];
> + my $domain_rec = {};
> +
> + $domain_rec->{domain} = $domains[$i];
> + $domain_rec->{plugin} = $acme->{plugin}
> + if defined $acme->{plugin};
> + $domain_rec->{alias} = $acme->{alias}
> + if defined $acme->{alias};
> + $conf->{"acme_additional_domain$i"} =
> + PVE::JSONSchema::print_property_string($domain_rec,
> + $acme_additional_desc);
so now this has overwritten a potentially existing acme_addition_domainX
entry (nothing tells me that the old and new are mutually exclusive?)
> + }
> +
> + write_config ($node, $conf);
> + $conf->{domains} = \@domains;
> +
> + return $conf;
> +};
could we avoid this if we just say the acme key remains mostly as is:
account => optional, defaults to default
domains => make it optional, always uses always-available 'default'
standalone plugin if defined
and the new acme_domainX:
domain
plugin
alias
whenever we want just the account, we call 'parse_acme'. whenever we
want the full picture, we call a new helper 'get_acme_domains' that
parses acme and all defined acme_domainX keys and returns the hash I
described earlier. we probably want checks that domains are only defined
once anyway, so in write_config we should ensure this.
this would avoid conversion at all, and just the GUI would need to
display this in a meaningful way (and probably annotate the domains with
whether they come from an index or from the main key).
from 6.x -> 7.x we could still deprecate domains in acme if we want and
do a one-time postinst upgrade or something
> +
> sub parse_acme {
> my ($data, $noerr) = @_;
>
> $data //= '';
this is now wrong, since the code below expects this to be a hash.
>
> - my $res = eval { PVE::JSONSchema::parse_property_string($acmedesc,
> $data); };
> + my $tmp_res = eval {
> + PVE::JSONSchema::parse_property_string($acmedesc, $data->{acme});
> + };
> if ($@) {
> return undef if $noerr;
> die $@;
> }
>
> - $res->{domains} = [ PVE::Tools::split_list($res->{domains}) ];
> + if (defined($tmp_res->{domains})) {
> + my $node = PVE::INotify::nodename();
this is dangerous - parsing! another node's config should not lead to
re-writing my own config
> + my $conf_file = config_file($node);
unused variable
> + $data = &$convert_domains($node, $data);
> + }
> + $tmp_res = PVE::JSONSchema::parse_property_string($acmedesc,
> $data->{acme});
> + my $res = { "0" => $tmp_res,
> + "domains" => [ $tmp_res->{domain} ],
> + };
> + $res->{account} = $tmp_res->{account};
> + delete $res->{0}->{account};
> +
> + for my $i (1..$MAXDOMAINS) {
now I see where the 1 comes from ;)
> + my $domain_rec = $data->{"acme_additional_domain$i"};
> + last if !defined($domain_rec);
this is wrong - all the acme_additional_domainN keys are independent, so
we can't stop parsing them on the first hole in the indices??
> + $tmp_res = eval {
> + PVE::JSONSchema::parse_property_string($acme_additional_desc,
> + $domain_rec);
> + };
> + if ($@) {
> + return undef if $noerr;
> + die $@;
> + }
> + $res->{$i} = $tmp_res;
> + push @{$res->{domains}}, $tmp_res->{domain};
> + }
>
> return $res;
> }
> --
> 2.20.1
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
