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

Add hostname configuration option to choose the hostname/IP to listen on #16

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

stdweird
Copy link
Member

Fixes #15

@stdweird stdweird added this to the 16.10 milestone Oct 11, 2016
@stdweird
Copy link
Member Author

restest this please

@@ -239,6 +247,7 @@ cdp-listend [OPTIONS]
--fetch_smear=SECONDS fetch run smearing time
--nch_smear=SECONDS nch run smearing time\n";
--facility syslog facility
--hostname=fqdn hostname/IP to listen on (use 0.0.0.0 to listen on all IPs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Listening on 0.0.0.0 is potentially a security risk. I think it was a feature of the old code that it was so restrictive so it was not possible to accidentally start listening on external interfaces. I don't like encoding configuration policy in code (that's what PAN is for!) but I feel reticent about this addition to the help text.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ned21 is it sufficient if i remove the 0.0.0.0 from the help text?

the old code would only listen on whatever hostname is set to, external/public or not. only localhost is not allowed (altough that is also not really valid anymore, people could listen on localhost and have some routing/kernel rules in places that pick up the packets from other interfaces.)

wrt security, in order to solve #13 properly, i'll add optional support for encrypting messages, similar to what ccm-fetch uses with TRUST. that way you could listen on 0.0.0.0; but that's for later.

@stdweird
Copy link
Member Author

@ned21 i removed 0.0.0.0 from the help text

@ned21
Copy link
Contributor

ned21 commented Oct 12, 2016

LGTM. If @samary can confirm the rpm works as expected its good to merge

@samary
Copy link

samary commented Oct 12, 2016

@stdweird, @ned21 : I tried and is seems to work.
Although I couldn't use the notify ant task (seems to use the hostname in the profile to notify, not sure how notify finds it).
I tested the notification using nc -u $hostname_private 7777 passing ccm TIMESTAMP as message.
We should maybe add a new attribute in the profile tree to specify the hostname to use for the notify ant task (could also be used to configure cdp-listend as well). I will try to find a way in quattor/configuration-modules-core#944.

@stdweird
Copy link
Member Author

@samary you cannot do this via the ant task, but if you add routes on the deploy host like

route add -host pubic.host gw private.host

(e.g. via quattor 😄 ) it should do the trick for now.

samary pushed a commit to quattor/configuration-modules-core that referenced this pull request Oct 12, 2016
A new "hostname" option has been introduced by quattor/cdp-listend#16 to specify the interface to bind.
@samary
Copy link

samary commented Oct 12, 2016

@stdweird : I don't know how the ant task is finding the hostname to use, any idea ? Is there a way to change the ant task to read another part in the profile (eg. /software/components/cdp/hostname) to have a 1 to 1 configuration (for coherence purpose) ?

@stdweird
Copy link
Member Author

@samary it doesn't. there's nothing the in the profile it can use for that.

@samary
Copy link

samary commented Oct 12, 2016

@stdweird : Ok, so how notify finds the hostname to use ?

@stdweird
Copy link
Member Author

@samary i'd guess the profile name itself is used?

@stdweird
Copy link
Member Author

@samary but you could make it use /software/components/cdp/hostname if it existed, but then the ant task has to consume/parse the profiles.

@samary
Copy link

samary commented Oct 12, 2016

@stdweird : I tried with the profile name but it not used by notify. Will ask to the mailing list. (Making ant reading the profile is way beyond my current understanding of it).

samary pushed a commit to quattor/configuration-modules-core that referenced this pull request Oct 12, 2016
A new "hostname" option has been introduced by quattor/cdp-listend#16 to specify the interface to bind.
@jrha
Copy link
Member

jrha commented Oct 12, 2016

Looks like the profile name from profiles-info.xml is what is used.

@samary
Copy link

samary commented Oct 12, 2016

@jrha : I'm not sure about that. I tried to change by hand the hostname in profiles-info.xml but still notifying on the wrong hostname.
It seems this has deeper implication than we thought : AFAIK, Hostname (therefore FULL_HOSTNAME) is set via object template profile_hostname; and used by all the quattor chain (profile name, hostname, notify, ...).
Not being able to specify another hostname to notify make this change a bit useless for us (except by using the routing hack @stdweird proposed but it is not consistent enough for us).
I'll open a separate issue to track our specific requirement.

@stdweird
Copy link
Member Author

@samary what might also work is add the public hostnames with the private ips in /etc/hosts
it's less hacky then the direct route

@jrha
Copy link
Member

jrha commented Oct 19, 2016

LGTM and fixes #15, so can it be merged?
The issue with notifications should probably be tracked on the scdb-ant-utils repository.

@ned21
Copy link
Contributor

ned21 commented Oct 19, 2016

Except it does not really fix the reason behind #15. Given it's not usable, do we really want to clutter up our code base with unused code? Or is it a nice to have feature that's worth keeping even if no one is using it? We've deleted a lot of unused code from Quattor over the years... IMHO it seems a shame to add new unused code.

@jrha
Copy link
Member

jrha commented Oct 19, 2016

It seems like a basic feature that would be nice to have to me, but I see your point.

@stdweird
Copy link
Member Author

@ned21 what is missing to make it usable? it's "only" the notifcation mechanism in the ant task, or is there something else i missed?

@ned21
Copy link
Contributor

ned21 commented Oct 19, 2016

I was going off this quote from @samary and did not dive into the details:

Not being able to specify another hostname to notify make this change a bit useless for us

So we might fix something else and then discover another problem further down the stack, etc etc.

@jrha
Copy link
Member

jrha commented Oct 19, 2016

It's a good point, I'll bump this and think about a top-level issue for the problem.

@jrha jrha modified the milestones: 16.12, 16.10 Oct 19, 2016
Copy link
Member

@jrha jrha left a comment

Choose a reason for hiding this comment

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

LGTM, but possibly not currently useful

@jrha jrha merged commit 1bee81e into quattor:master Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants