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

Remove default value of 0 for $port and allow $port to be unset #81

Merged
merged 1 commit into from
Jul 11, 2020

Conversation

chrekh
Copy link
Contributor

@chrekh chrekh commented Jul 10, 2020

Also remove the requirement for $port to be set if $qeryhost is used. If there is no
'allow' in chrony.conf the server doesn't allow client access, no need to also set port to
0. And if there is 'allow' in chrony.conf the default port to listen on is the expected
123.

From chrony.conf(5)

port port
This option allows the UDP port on which the server understands NTP requests to be
specified. For normal servers this option should not be required (the default is 123,
the standard NTP port).

Comment on lines -228 to -230
if $queryhosts != [] and $port == 0 {
fail("Setting \$queryhosts has no effect unless also setting \$port which defaults to 0 in ${module_name}, refusing that.")
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be kept in but with and !$port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The point of this change is that I want to accept client access without needing to specify the port, which then is the default ntp-port 123. But as you said, this needs to be reviewed by someone more with chrony knowledge.

I also noticed that the unit-test failed. I need to also change the test. Unit-testing is a great thing! :)

@ekohl
Copy link
Member

ekohl commented Jul 10, 2020

It'd be nice if anyone with chrony knowledge could review this instead of me.

Also remove the requirement for $port to be set if $qeryhost is used. If there is no
'allow' in chrony.conf the server doesn't allow client access, no need to also set port to
0. And if there is 'allow' in chrony.conf the default port to listen on is the expected
123.

From chrony.conf(5)

   port port
       This option allows the UDP port on which the server understands NTP requests to be
       specified. For normal servers this option should not be required (the default is 123,
       the standard NTP port).
@alexjfisher
Copy link
Member

I think this is ok. The template's use of if $port { didn't make any sense if $port always had a value.

@igalic
Copy link
Contributor

igalic commented Jul 10, 2020

I think this is ok. The template's use of if $port { didn't make any sense if $port always had a value.

indeed. 0 in ruby is "truthy":

[2] pry(main)> if 0 then :true else :false end
=> :true

0 as a port in TCP/UCP bind() call however, means: "Dear Operating System, give me any free port, please"

@alexjfisher alexjfisher merged commit e2cfc46 into voxpupuli:master Jul 11, 2020
@chrekh chrekh deleted the port branch July 12, 2020 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants