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.

Sign up for GitHub

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

Jump to bottom

Allow 4 and 6 suffix inside proto to limit ip4 or ip6 connection only. #327

Merged
merged 1 commit into from Feb 3, 2019
Merged

Allow 4 and 6 suffix inside proto to limit ip4 or ip6 connection only. #327

merged 1 commit into from Feb 3, 2019

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Feb 2, 2019

Pull Request (PR) description

Since 8.0.0 it's not possible to choose tcp4 or tcp6 as proto for an openvpn server.

This Pull Request (PR) fixes the following issues

N/A

bastelfreak
bastelfreak reviewed Feb 2, 2019
View reviewed changes
manifests/server.pp Outdated Show resolved Hide resolved
@bastelfreak bastelfreak added bug Something isn't working needs-feedback Further information is requested labels Feb 2, 2019
@jkroepke jkroepke changed the title Allow tcp4 and tcp6 as proto Allow 4 and 6 as suffix inside proto to limit ip4 or ip6 connection only. Feb 2, 2019
@jkroepke jkroepke changed the title Allow 4 and 6 as suffix inside proto to limit ip4 or ip6 connection only. Allow 4 and 6 suffix inside proto to limit ip4 or ip6 connection only. Feb 2, 2019
@bastelfreak bastelfreak removed the needs-feedback Further information is requested label Feb 2, 2019
@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 2, 2019

Let me add some tests before merge.

bastelfreak
bastelfreak reviewed Feb 2, 2019
View reviewed changes
templates/server.erb Outdated Show resolved Hide resolved
@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 3, 2019 •
edited

Tests added,

Travis failure should not related here.

ghoneycutt
ghoneycutt reviewed Feb 3, 2019
View reviewed changes
spec/defines/openvpn_server_spec.rb
@@ -110,6 +110,66 @@
it { is_expected.to contain_file('/etc/openvpn/test_server.conf').with_content(%r{^rcvbuf\s+393215$}) }
end

context 'when using udp4' do
Copy link
Member

Choose a reason for hiding this comment

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

There should be a context that shows that an error is thrown if an invalid entry for proto is given.

ghoneycutt
ghoneycutt reviewed Feb 3, 2019
View reviewed changes
manifests/server.pp Outdated Show resolved Hide resolved
@ghoneycutt
Copy link
Member

Travis failure looks like a timeout. Restarted the job.

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 3, 2019

Implement changes as requested.

ghoneycutt
ghoneycutt reviewed Feb 3, 2019
View reviewed changes
spec/defines/openvpn_server_spec.rb Outdated
context 'when using tcp6' do
let(:params) do
{
'country' => 'CO',
Copy link
Member

Choose a reason for hiding this comment

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

this section has four spaces instead of two and is throwing style errors at https://travis-ci.org/voxpupuli/puppet-openvpn/jobs/488140662

ghoneycutt
ghoneycutt reviewed Feb 3, 2019
View reviewed changes
spec/defines/openvpn_server_spec.rb Outdated
context 'when using invalid value for proto' do
let(:params) do
{
'country' => 'CO',
Copy link
Member

Choose a reason for hiding this comment

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

same style issue here

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 3, 2019

Another question. Should tcp-server, tcp4-server and tcp6-server also allowed? From OpenVPN side, values like tcp*-server are the correct values not just tcp.

I know that the module append -server automatically but it looks like a backward compatibility feature.

From man page, only udp[46]? and tcp[46]?-server are allowed.

@ghoneycutt
Copy link
Member

I don't follow you as the -server part is already added as you mentioned.

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 3, 2019

  • From module site proto => 'tcp-server' is invalid while it's valid in openvpn.
  • From module site proto => 'tcp' is valid but invalid (or deprecated) in openvpn.

I miss the consistency between openvpn and module parameters.

@ghoneycutt
Copy link
Member

We should probably not append -server at all, though that would need a major version bump.

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 3, 2019

I would prefer to implement -server as valid parameter and raise a deprecation warning for the old behavior. But this should be an another PR. ok.

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 3, 2019

beaker still timeouts inside travis.

@ghoneycutt
Copy link
Member

Thanks @jkroepke !

@ghoneycutt ghoneycutt merged commit d6b7ad0 into voxpupuli:master Feb 3, 2019
@ghoneycutt
Copy link
Member

Released in v8.1.0

@jkroepke jkroepke deleted the tcp6 branch February 3, 2019 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghoneycutt ghoneycutt ghoneycutt left review comments

@bastelfreak bastelfreak bastelfreak left review comments

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jkroepke @ghoneycutt @bastelfreak