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

Changed IPs to Hostnames where possible, and removed host.present tes… #369

Closed
wants to merge 2 commits into from

Conversation

@setupminimal
Copy link
Contributor

setupminimal commented May 10, 2016

…ts from common/init.sls

This theoretically solves Issue #288. It certainly prevents /etc/hosts from being messed with, and the recommended testing in Vagrant returns all green. I'm still not entirely sure that I didn't miss something, just because I'm unfamiliar with saltfs's code, so I would appreciate some feedback.


This change is Reviewable

@aneeshusa
Copy link
Member

aneeshusa commented May 10, 2016

Thanks for the PR! We actually have moved off Linode, and are using DNS instead of the hosts file for name resolution, so the hosts dictionary in the map.jinja file should be removed entirely to make sure everything is going through DNS.

@setupminimal
Copy link
Contributor Author

setupminimal commented May 10, 2016

Fixed. Thank you.

@aneeshusa
Copy link
Member

aneeshusa commented May 10, 2016

LGTM, please squash these commits together.

@aneeshusa
Copy link
Member

aneeshusa commented May 10, 2016

Actually, we should use a Salt file.managed state to set up the hosts file; this makes sure that the old entries get flushed out.

I checked on the builders and this should be suitable for the contents of that file:

127.0.0.1           localhost
::1                 localhost
255.255.255.255     broadcasthost

Here are some docs on file.managed: https://docs.saltstack.com/en/2015.5/ref/states/all/salt.states.file.html#salt.states.file.managed
We also use it in many other files if you want some examples.

@setupminimal setupminimal force-pushed the setupminimal:master branch 2 times, most recently from 9c4679c to a6ede2b May 11, 2016
@setupminimal
Copy link
Contributor Author

setupminimal commented May 11, 2016

There. All tested, /etc/hosts is managed, all the extraneous stuff is gone, and it's all squashed into one commit. Is there anything else I should do?

/etc/hosts:
file.managed:
- contents: "127.0.0.1 localhost\n::1 localhost\n255.255.255.255 broadcasthost"

This comment has been minimized.

@aneeshusa

aneeshusa May 11, 2016

Member

We should put these into a file and refer to them using source instead of contents. Please make a common/files directory, and put these into a file called hosts in that directory.

@setupminimal
Copy link
Contributor Author

setupminimal commented May 11, 2016

I have to go to bed, and a busy day tomorrow, so I will get back to you Thursday or Friday.

Thank you for your help.

/etc/hosts:
file.managed:
- contents: "127.0.0.1 localhost\n::1 localhost\n255.255.255.255 broadcasthost"
-

This comment has been minimized.

@aneeshusa

aneeshusa May 11, 2016

Member

Please also add user: root and mode: 644.

The group is variable and depends on the OS, so we need to use Jinja templating to set it up. It should be root on Ubuntu and wheel on OS X.

@jdm
Copy link
Member

jdm commented May 24, 2016

@setupminimal Any progress? Any questions?

@setupminimal setupminimal force-pushed the setupminimal:master branch from a6ede2b to 672cbe3 Jun 5, 2016
@setupminimal
Copy link
Contributor Author

setupminimal commented Jun 5, 2016

I have been banging my head on this for a while, interspersed with studying for finals, and I can't find the problem. I tried to put the /etc/hosts contents in a file, as you said, but then I haven't been able to refer to that file properly. Would you look at what I'm doing and tell me where I went wrong please?

@aneeshusa
Copy link
Member

aneeshusa commented Jun 8, 2016

@setupminimal

The way to refer to the file is adding this key/value pair to the file.managed state: - source: salt://{{ tpldir }}/files/hosts.

For reference, here is the docs link again: https://docs.saltstack.com/en/2015.5/ref/states/all/salt.states.file.html#salt.states.file.managed.

I'm not sure where you got the location parameter - that isn't one of the possible arguments to file.managed. The right one to use is source.

source takes a URL value; we want to reference the file from the Salt file tree (instead of a file on the Internet or S3, for example), so we use the salt:// scheme. The {{ tpldir }} part uses Jinja templating to insert the path to the directory the file is in - so, because we're using it from the common/init.sls file, it will resolve to just common. Then the /files/hosts completes the path.

Hopefully that helps; let me know if that's unclear.

{% for ssh_user in common.ssh_users %}
sshkey-{{ ssh_user }}:
ssh_auth.present:
- user: root
- source: salt://{{ tpldir }}/ssh/{{ ssh_user }}.pub
{% endfor %}
hosts-file:

This comment has been minimized.

@aneeshusa

aneeshusa Jun 8, 2016

Member

This can be /etc/hosts instead, and then the name parameter is unnecessary down below.

{% if grains['os'] == 'MacOS' %}
- group: wheel
{% elif grains['os'] == 'Ubuntu' %}
- group: root"

This comment has been minimized.

@aneeshusa

aneeshusa Jun 8, 2016

Member

There's a trailing " here.

@aneeshusa
Copy link
Member

aneeshusa commented Jul 8, 2016

@setupminimal Sorry to leave this hanging, I didn't see you had updated this! This LGTM and should be good to go after a squash.

{% elif grains['os'] == 'Ubuntu' %}
- group: root
{% endif %}
- source: salt://{{ tlpdir }}/files/hosts

This comment has been minimized.

@aneeshusa

aneeshusa Jul 8, 2016

Member

typo: Actually, this should be {{ tpldir }}.

@aneeshusa
Copy link
Member

aneeshusa commented Jul 8, 2016

Also please rebase against master when you squash.

@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2017

The latest upstream changes (presumably #569) made this pull request unmergeable. Please resolve the merge conflicts.

@aneeshusa
Copy link
Member

aneeshusa commented Jan 12, 2017

Closing due to lack of response; these commits have been folded into #577.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.