Update to work with HTTP::Client to work with Vault secured with other CA like Letsencrypt

[ntanaskovic]

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

[ntanaskovic]

I've reviewed failed checks and they seem to be due to testing parameters used. Checks use "https://vault.doesnotexist:8200" and of course that domain doesn't exist, let me know if I am missing something here

[smortex]

#47 I just opened seems to show that the tests where working previously of this change. I believe that the connector being changed, the behavior is not the same and the tests need some adjustment to test the right (new) behavior.

I feel like connection was an established connection, and it is now just an HTTP client, so maybe some renaming can also help clarifying?

[ntanaskovic]

Hi Romain,

#47 <#47> I just opened seems to show that the tests where working previously of this change. I believe that the connector being changed, the behavior is not the same and the tests need some adjustment to test the right behavior.

yes that is correct. For Puppet agent to be able to connect to Vault secured with Letsencrypt CA certs, I needed to replace (now deprecated) Puppet::Network::HttpPool, as per this page https://www.rubydoc.info/gems/puppet/Puppet/Network/HttpPool with HTTP::Client instead, as per this page https://www.rubydoc.info/gems/puppet/Puppet/HTTP/Client and I was just following instructions in that page, converting parameters which are passed while invoking it. I was trying to make modifications to a minimum and to still keep compatibility with previous version. New module works with both Letsencrypt and Puppet certificates so it is backward compatible, however since module HTTP/Client is different, it probably gives different output than your CI tests expects, so it needs adjusting.

I feel like |connection| was an established connection, and it is now just an HTTP client, so maybe some renaming can also help clarifying?

Hmm yes, I think that new HTTP::Client doesn't offer established connections, but the name is not misleading, it is still appropriate. Fell free to rename it if you think of something a bit suitable. Best regards Nikola

…

On 09/12/2021 18:57, Romain Tartière wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In lib/puppet/functions/vault_lookup/lookup.rb <#45 (comment)>: > - connection = Puppet::Network::HttpPool.http_instance(uri.host, uri.port, use_ssl) + connection = Puppet.runtime[:http] #47 <#47> I just opened seems to show that the tests where working previously of this change. I believe that the connector being changed, the behavior is not the same and the tests need some adjustment to test the right behavior. I feel like |connection| was an established connection, and it is now just an HTTP client, so maybe some renaming can also help clarifying? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#45 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APCTXPZP5G4Z3HIXY6DSOLDUQD32NANCNFSM5FR5XAYA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[root-expert]

This was implemented at #50