-
Notifications
You must be signed in to change notification settings - Fork 21
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
[preinst] Check for proxy and prexisting pgp key #143
[preinst] Check for proxy and prexisting pgp key #143
Conversation
I accidentally included the fingerprint for the key which expires on 2018-03-04. New fingerprint is for the key that expires on 2022-06-28. |
Thanks @dsalvador-dsalvador we'll review this shortly |
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.
Thanks @dsalvador-dsalvador, I've added a comment on the custom handling of the proxy, but otherwise this looks good
echo "... key already installed" | ||
else | ||
if [ "$http_proxy" != "" ]; then | ||
HKP_PROXY="--keyserver-options http-proxy=$http_proxy" |
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.
From man gpg
in the --keyserver-options
section:
http-proxy=value
Set the proxy to use for HTTP and HKP keyservers. This overrides the "http_proxy" environment variable, if any.
So I don't think we need to handle the proxy setting manually here.
I've also tried on a local environment and apt-key adv
seems to pick up the proxy defined in the http_proxy
env var on its own.
If you agree let's remove this block
Hey @dsalvador-dsalvador can you clarify what you mean when you say the logic breaks? Do you mean the install fails or just that fetching the keys fails? I can replicate failing to fetch the keys, but it always seems to install just fine (sans the keys). |
@gmmeyer The new keys fail to fetch, which causes the chef-datadog cookbook to fail during chef-client runs. |
@olivielpeau Removed the if block that sets the proxy argument. Although I left the part that checks if the key already exists. |
@dsalvador-dsalvador Thanks for the clarification. I don't think the present PR will fix this specific issue: if the chef run fails it's probably caused by this resource in the datadog cookbook that would fail on a node that doesn't have the key installed and doesn't have access to the key server: https://github.com/DataDog/chef-datadog/blob/v2.8.1/recipes/repository.rb#L29-L32 One way to work around it would be to install the key before the datadog cookbook's recipes are run. If that doesn't sound like a good solution, could you open an issue against the Even though the present PR won't solve your issue it looks like a good improvement, I'm going to do some local testing to check that it works as expected. |
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.
Requesting a small update of the comment, other than that this looks good!
Also, let me know what you think of my comment above about the chef cookbook
@@ -34,10 +34,15 @@ if [ "$DISTRIBUTION" != "Darwin" ]; then | |||
|
|||
# Prepare the GPG keys rotation: | |||
# Add the new one to the list of trusted APT keys. | |||
# Some servers are on severely resticted networks. Check for proxy, | |||
# and check if key already exists. |
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.
One last thing:
- there's a typo
resticted
->restricted
- could you update the comment? (i.e. remove or clarify
Check for proxy
since the script itself doesn't check for proxy anymore :) )
@olivielpeau That link to the resource isn't where the cookbook fails, it was during the installation of the deb package, which is in the dd-handler recipe before the repository recipe runs. I like your proposed change to the cookbook, though. I'm just not sure yet on how to implement it. |
@dsalvador-dsalvador Merging, thanks! About the issue on the chef cookbook, I'll let you open an issue on the |
The current logic breaks on servers with limited connections to the internet.
Check if the key already exists. Private Apt mirrors can provide these keys.
Alternatively, check for a proxy and apply it to the apt-key command.