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

Rewrite loadjson to use the modern function API #1415

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Dec 23, 2023

Summary

This also resolves a bug where JSON.parse returned a StringIO. To properly catch this, the tests are rewritten to avoid mocking where possible. Exceptions are with external URLs and where a failure is expected.

Related Issues (if any)

Fixes #1414

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

# $myhash = loadjson('https://example.local/my_hash.json')
# $myhash = loadjson('https://username:password@example.local/my_hash.json')
# $myhash = loadjson('no-file.json', {'default' => 'value'})
Puppet::Functions.create_function(:loadjson) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

At the same time, shouldn't we namespace this function and create a (deprecated) shim like we started with a lot of the other functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't consider this.

if Puppet::Util::Package.versioncmp(Puppet.version, '8.0.0').negative?
PSON.load(source)
else
JSON.load(source)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rubocop is complaining here. Are you specifically using JSON.load over JSON.parse to address the bug you mentioned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's copied from the old code to remain compatible.

if path.start_with?('http://', 'https://')
require 'open-uri'
begin
content = URI.open(path) { |f| load_json_source(f) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened to all the basic authentication related code and tests?

Copy link
Collaborator Author

@ekohl ekohl Apr 5, 2024

Choose a reason for hiding this comment

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

Good point. I assumed it would just use the credentials from the URI, but it doesn't.

Simple reproducer:

# dnf install httpd
# htpasswd -c /etc/httpd/passwords admin supersecret
# echo 'Hello World!' > /var/www/html/index.html

Now replace Require all granted in /etc/httpd/conf/httpd.conf with the following:

    AuthType Basic
    AuthName "Restricted Files"
    AuthUserFile "/etc/httpd/passwords"
    Require valid-user

Verify it works with curl:

# curl -s -I localhost | head -n 1
HTTP/1.1 401 Unauthorized
# curl -s -I localhost -u admin:supersecret | head -n 1
HTTP/1.1 200 OK

Then we can write a small Ruby script:

require 'open-uri'

uri = URI.parse('http://admin:supersecret@localhost/index.html')
options = {}
if uri.user
  options[:http_basic_authentication] = [uri.user, uri.password]
  uri.user = nil
end
uri.open(**options) do |f|
  puts f.read
end

This also resolves a bug where JSON.parse returned a StringIO. To
properly catch this, the tests are rewritten to avoid mocking where
possible. Exceptions are with external URLs and where a failure is
expected.
@ekohl ekohl force-pushed the rewrite-loadjson-to-modern-function branch from c9d26e5 to 05139bb Compare April 5, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loadjson returns StringIO data which doesn't work in Puppet 8
3 participants