-
Notifications
You must be signed in to change notification settings - Fork 225
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
(GH-3296) Try both token and cert based auth for puppetdb #3297
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,14 +16,27 @@ def initialize(config:, project: nil, load_defaults: false) | |
@logger = Bolt::Logger.logger(self) | ||
end | ||
|
||
def post_puppetdb(url, body) | ||
response = http_client.post(url, body: body, header: headers(@config.token)) | ||
if response.status == 401 && @token_and_cert | ||
@logger.debug("Invalid token: #{response.body}, retrying with cert based auth") | ||
response = http_client.post(url, body: body, header: headers) | ||
if response.ok? | ||
@logger.debug("Puppetdb token is invalid, but certs are not. No longer including token.") | ||
@bad_token = true | ||
end | ||
end | ||
response | ||
end | ||
|
||
def make_query(query, path = nil) | ||
body = JSON.generate(query: query) | ||
url = "#{uri}/pdb/query/v4" | ||
url += "/#{path}" if path | ||
|
||
begin | ||
@logger.debug("Sending PuppetDB query to #{url}") | ||
response = http_client.post(url, body: body, header: headers) | ||
response = post_puppetdb(url, body) | ||
donoghuc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rescue StandardError => e | ||
raise Bolt::PuppetDBFailoverError, "Failed to query PuppetDB: #{e}" | ||
end | ||
|
@@ -81,7 +94,7 @@ def send_command(command, version, payload) | |
# Send the command to PDB | ||
begin | ||
@logger.debug("Sending PuppetDB command '#{command}' to #{url}") | ||
response = http_client.post(url.to_s, body: body, header: headers) | ||
response = post_puppetdb(url.to_s, body) | ||
rescue StandardError => e | ||
raise Bolt::PuppetDBFailoverError, "Failed to invoke PuppetDB command: #{e}" | ||
end | ||
|
@@ -109,11 +122,15 @@ def http_client | |
require 'httpclient' | ||
@logger.trace("Creating HTTP Client") | ||
@http = HTTPClient.new | ||
@http.ssl_config.set_client_cert_file(@config.cert, @config.key) if @config.cert | ||
@http.ssl_config.add_trust_ca(@config.cacert) | ||
@http.connect_timeout = @config.connect_timeout if @config.connect_timeout | ||
@http.receive_timeout = @config.read_timeout if @config.read_timeout | ||
|
||
# Determine if there are both token and cert auth methods defined | ||
@token_and_cert = false | ||
if @config.cert | ||
@http.ssl_config.set_client_cert_file(@config.cert, @config.key) | ||
@token_and_cert = [email protected]? | ||
end | ||
@http | ||
end | ||
|
||
|
@@ -136,9 +153,9 @@ def uri | |
uri | ||
end | ||
|
||
def headers | ||
def headers(token = nil) | ||
headers = { 'Content-Type' => 'application/json' } | ||
headers['X-Authentication'] = @config.token if @config.token | ||
headers['X-Authentication'] = token if token && !@bad_token | ||
headers | ||
end | ||
end | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means if you have an invalid token, it's always going to be making two requests every time and you'll get failures in the PDB log for each one. Also, if you don't have a token but you do have certs specified, this is going to have the same behavior (actually not sure if it's 401 if there is no X-Authentication header at all).
What if this chooses one or the other if only one is specified, and if both are specified, try the token first. If that fails, record that it failed and don't try using it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to that. I'll look at reducing requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to persist token exclusion when it is determined to be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unable to reproduce a 401 other than the case where an invalid token is provided.