autostructure / artifactory_utils Goto Github PK

You may or may not want to add this but I found the generic error message of "No file can be found at..." is not really sufficient. It is misleading when the error occurred for another reason. I would recommend including the error code in the error messages, i.e.

raise Puppet::Error, 'No file can be found at ' + url + " (Response code: " + response.code + ")"

Better yet, handle the file not found condition 404 specifically and put in a second error code for everything else. Maybe something like:

        if response.code == "200"
[...]
        elif response.code == "404"
          raise Puppet::Error, 'No file can be found at ' + url 
        else
          raise Puppet::Error, 'Error retrieving file at ' + url + " (Response code: " + response.code + ")"
        end

This came up in the process of debugging why ssl was not working, (Issue #2) but I could see this coming up in other cases as well.

The current module does not support ssl. We have moved our Artifactory instance to ssl to protect user credentials being used. This has broken our use of this module. I've added code to the existing module to fix the problem. Right now, rather than handle SSL certs I turned off SSL verify. This would be the minimum requirement for ssl support. If cert handling is added it would probably be desirable to have a configurable option ssl_verfy you can set to true of false. The change I've added to the existing code is to update the couple of call in the code similar to this:

Net::HTTP.start(uri.host, uri.port) do |http|

To this:

Net::HTTP.start(uri.host, uri.port, :use_ssl => (uri.scheme == 'https'), :verify_mode => OpenSSL::SSL::VERIFY_NONE) do |http|

I've encountered a problem where very large artifact will cache to /tmp before writing to the final destination. In most cases /tmp is not big enough to hold the cached response. In my case I am talking about 8G artifacts. There does seem to be a way in the Net::HTTP ruby module to stream a response body directly to a destination though this module does not currently use this feature. Some consideration should be given to a parameter that allows the specification of a destination tmp path or default to the destination if given. The caveat would be you need double the space to transfer the file down.

This is really not so much a bug but a suggested feature. We are using this module to transfer some very large files, such as install ISOs. One file for example is 9 Gig. Right now, even if the file exists, a digest is performed every Puppet run. For a very large file is very CPU intensive and I/O intensive. In most cases, for small file there is no harm but I was thinking for very large files it might make sense to have a resource option to skip digest checks. This could just be an optional setting you could specify in the resource. I can put this together and send you the changes if you like. This should be a very simple change. What are your thoughts?

I believe in the process of moving to file streaming that basic auth was broken. Below is my code to fix, at least for the functions I use.

  def get_header(source_url, user_name, password_hash)
    # [RELEASE] has special signifiance in Artifactory. Let's escape it
    url = source_url.gsub(/\[RELEASE\]/, '%5BRELEASE%5D')

    uri_get = URI.parse(url)

    http = Net::HTTP.start(uri_get.host, uri_get.port, use_ssl: (uri_get.scheme == 'https'), verify_mode: OpenSSL::SSL::VERIFY_NONE )

    request = Net::HTTP::Head.new(uri_get)

    if user_name && password_hash
       request.basic_auth user_name, password_hash
    end

    response = http.request request

    if response.code == 301
      response = get_query(URI.parse(response.header['location']), user_name, password_hash)
    end

    response
  end

  def write_file(url, destination, _user_name, _password_hash, owner, group)
    # [RELEASE] has special signifiance in Artifactory. Let's escape it
    escaped_url = url.gsub(/\[RELEASE\]/, '%5BRELEASE%5D')

    uri = URI(escaped_url)

    Net::HTTP.start(uri.host, uri.port, use_ssl: (uri.scheme == 'https'), verify_mode: OpenSSL::SSL::VERIFY_NONE) do |http|
      request = Net::HTTP::Get.new uri

      if _user_name && _password_hash
         request.basic_auth _user_name, _password_hash
      end