[pve-devel] [Patch V2 manager 4/8] Add the config parser and convert the old config to a new one.

Fabian Grünbichler f.gruenbichler at proxmox.com
Wed Apr 1 15:26:09 CEST 2020


On March 31, 2020 12:08 pm, Wolfgang Link wrote:
> Signed-off-by: Wolfgang Link <w.link at 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 at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 




More information about the pve-devel mailing list