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

Jump to bottom

fixes #10436 Don't configure dns_key if nsupdate_gss is used #171

Closed
wants to merge 1 commit into from
Closed

fixes #10436 Don't configure dns_key if nsupdate_gss is used #171

wants to merge 1 commit into from

Conversation

smags
Copy link
Contributor

@smags smags commented May 9, 2015

Fixes http://projects.theforeman.org/issues/10436

My first ever pull request. Be gentle...

mmoll
mmoll reviewed May 10, 2015
View reviewed changes
templates/dns.yml.erb
@@ -17,7 +16,10 @@
<%if scope.lookupvar("foreman_proxy::dns_provider") == 'nsupdate_gss' -%>
:dns_tsig_keytab: <%= scope.lookupvar("foreman_proxy::dns_tsig_keytab") %>
:dns_tsig_principal: <%= scope.lookupvar("foreman_proxy::dns_tsig_principal") %>
# dns_key must be disabled if nsupdate_gss is used
#:dns_key: <%= scope.lookupvar("foreman_proxy::keyfile") %>
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the docs, :dns_key: false should be set. It makes probably no difference -proxy wise, but please change it to align it with the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i set dns_key: false i get:
E, [2015-05-10T17:16:17.219244 #14988] ERROR -- : no implicit conversion of false into String

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 🐼
Could you open a PR to theforeman/theforeman.org to fix chapter 4.3.6.3 then? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like to wait for a comment from @stbenjam ?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs are indeed wrong, they should say to comment it out. The "false" suggestion probably came from back when we had this bug in 1.5: http://projects.theforeman.org/issues/5675. We had "false" as a workaround due to the bug in 1.5.0, then fixed the original issue in 1.5.2, only to find that keeping it set to false threw the error @smags noted above. We never did fix it to handle both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @domcleal . I will squash the commits tonight and create a pull request for the docs.

@mmoll
Copy link
Contributor

mmoll commented May 10, 2015

Looking OK Puppet wise for me, please address the line note and squash the commits together into one then.

Somebody more proficient with IPA and friends should review this. @stbenjam maybe?

@stbenjam
Copy link
Member

I've never used this, so can't really comment. I always let freeipa handle DNS without foreman being involved.

@smags
Copy link
Contributor Author

smags commented May 11, 2015

I only found it because i am simulating independent DNS and IPA servers. DNS/DHCP would later run on Microsoft.

@smags
Copy link
Contributor Author

smags commented May 11, 2015

Squashed commits and fixed the documentation: theforeman/theforeman.org#378

@mmoll
Copy link
Contributor

mmoll commented May 11, 2015

👍

@ekohl ekohl closed this in 753b65c May 12, 2015
@ekohl
Copy link
Member

ekohl commented May 12, 2015

Thanks!

domcleal pushed a commit that referenced this pull request May 20, 2015
fixes #10436 Don't configure dns_key if nsupdate_gss is used
9c64830 
Closes GH-171

(cherry picked from commit 753b65c)

Conflicts:
	spec/classes/foreman_proxy__config__spec.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@smags @mmoll @stbenjam @ekohl @domcleal