Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple values in TLSAccept #947

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

wolfaba
Copy link
Contributor

@wolfaba wolfaba commented May 17, 2024

Pull Request (PR) description

Option TLSAccept in zabbix_agentd.conf accepts list of values separated with comma. But the module define tlsaccept as Enum of those three values. The string as comma separated list of values is not allowed anymore. Therefore the tlsaccept should be array of that enum and in the template the array should be joined with comma.

BTW: the same option TLSAccept is in zabbix_proxy.conf, but in the module the tlsaccept in proxy.pp is still simple string value.

Thank you.

@wolfaba
Copy link
Contributor Author

wolfaba commented May 17, 2024

I am sorry for so many commits. I am neither ruby nor puppet developer 😞 If you understand, what I mean, could you please fix the "if"? I need join array values or just insert single string (Enum) value. Thank you.

@@ -310,7 +310,7 @@ LoadModulePath=<%= @loadmodulepath %>
# Mandatory: yes, if TLS certificate or PSK parameters are defined (even for 'unencrypted' connection)
# Default:
# TLSAccept=unencrypted
<% if @tlsaccept %>TLSAccept=<%= @tlsaccept %><% end %>
<% if @tlsaccept and ( @tlsaccept.instance_of?(Array) or @tlsaccept.instance_of?(String) ) %>TLSAccept=<% if @tlsaccept.instance_of?(Array) %><%= scope.call_function('join', [@tlsaccept, ',']) %><% elsif @tlsaccept.instance_of?(String) %><%= @tlsaccept %><% end %><% end %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this (not tested)?

Suggested change
<% if @tlsaccept and ( @tlsaccept.instance_of?(Array) or @tlsaccept.instance_of?(String) ) %>TLSAccept=<% if @tlsaccept.instance_of?(Array) %><%= scope.call_function('join', [@tlsaccept, ',']) %><% elsif @tlsaccept.instance_of?(String) %><%= @tlsaccept %><% end %><% end %>
<% if @tlsaccept %>TLSAccept=<%= [@tlsaccept].flatten.join(',') %><% end %>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smortex of course it works, thank you very much, it's simple and nice 👍

Copy link
Contributor

@Valantin Valantin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Robert, can you add the same datatype to the tlsaccept on the proxy?

@wolfaba
Copy link
Contributor Author

wolfaba commented May 27, 2024

Hi Robert, can you add the same datatype to the tlsaccept on the proxy?

Hello @Valantin, I hope it's correct.

@Valantin
Copy link
Contributor

ok, now I can you add spec test for the classes?

for agent exist
it { is_expected.to contain_file('/etc/zabbix/zabbix_agentd.conf').with_content %r{^TLSAccept=cert$} }
You can try to add two test, using tlsaccept => ['cert'] and one using a two element array.
the first one is for check it work in the some way with string and array with one element and the second for check it costruct correctly the string with an array of two or more.
For proxy not exist the spec, probably you can reuse the one from the agent

 context 'configuration file with full options' do
    if facts[:kernel] == 'Linux'
      let :params { tlsaccept: ['cert'] }
      it { is_expected.to contain_file('/etc/zabbix/zabbix_agentd.conf').with_content %r{^TLSAccept=cert$} }
    end
 end

@Valantin Valantin changed the title multiple values in tlsaccept are joined with comma to TLSAccept in zabbix_agentd.conf Support multiple values in TLSAccept May 27, 2024
@Valantin Valantin self-requested a review May 27, 2024 15:11
@Valantin
Copy link
Contributor

@wolfaba can you squash in one commit with good message?

@wolfaba
Copy link
Contributor Author

wolfaba commented May 27, 2024

@Valantin is it better now? I hope I haven't broke it 😬

@Valantin Valantin merged commit edf9e52 into voxpupuli:master May 27, 2024
62 checks passed
@Valantin Valantin removed the enhancement New feature or request label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants