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

Add notes/known issues section to installation page #9053

Merged
merged 7 commits into from Jun 4, 2020

Conversation

joelhans
Copy link
Contributor

@joelhans joelhans commented May 14, 2020

Summary

Fixes #8306

I added this notes section to the installation page where we can put notices like SSL and workarounds for various distros. I don't love cluttering up that page any more, but I can't figure out where else we could put this information with the best mix of discoverability and not being too in-your-face.

Component Name

packaging/

Test Plan
Additional Information

@joelhans joelhans requested a review from cosmix as a code owner May 14, 2020 19:37
@squash-labs
Copy link

squash-labs bot commented May 14, 2020

Manage this branch in Squash

Test this branch here: https://joelhansfix-openssl-psk6s.squash.io

@joelhans joelhans requested review from Ferroin and prologic May 14, 2020 19:37
@github-actions github-actions bot added area/docs area/packaging Packaging and operating systems support labels May 14, 2020
@Ferroin
Copy link
Member

Ferroin commented May 15, 2020

We might want to mention here that the build also currently does not work with LibreSSL in place of OpenSSL, and also fails when using Clang in some configurations.

@joelhans
Copy link
Contributor Author

@Ferroin Done!

Comment on lines 177 to 178
**LibreSSL**: The Agent installer is only compatible with OpenSSL. Critical functions do not work on systems with
LibreSSL installed as a replacement for OpenSSL.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the build fails completely if you try to use LibreSSL instead of OpenSSL. They use the same naming for libraries and headers but aren't API compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ferroin I am working on a new PR with TLS right now, I will bring a fix for this on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ferroin Done.

@thiagoftsm We can always edit this text again when your PR is ready.

@prologic
Copy link
Contributor

We might want to mention here that the build also currently does not work with LibreSSL in place of OpenSSL, and also fails when using Clang in some configurations.

There was a PR to fix this. I'm not sure this is true anymore. Let me find the PR.

@prologic
Copy link
Contributor

Issue tracking LibreSSL fix #8796

@prologic
Copy link
Contributor

We should hold of fon merging this until #8796 is resolved otherwise it makes us look a bit silly to say it doens't compile with LibreSSL one day but does the next. :)

@thiagoftsm
Copy link
Contributor

@prologic the PR #9068 will fix the problem when it is merged, this is the motive I closed #8796.

@joelhans
Copy link
Contributor Author

We should hold of fon merging this until #8796 is resolved otherwise it makes us look a bit silly to say it doens't compile with LibreSSL one day but does the next. :)

Waiting is fine with me. You guys can ping me when this is ready for an update and I'll get on it.

@joelhans joelhans changed the title Add notes section to installer with notes on OpenSSL Add notes/known issues section to installation page May 27, 2020
@joelhans
Copy link
Contributor Author

An update on this one: @zack-shoylev and I need to add a few more workarounds for old systems into this PR, so I changed the title to better reflect what we're doing and we'll ask for reviews again when we've added all the notes.

Context is at netdata/marketing#240.

prologic
prologic previously approved these changes May 28, 2020
@zack-shoylev
Copy link
Contributor

Should we add @ktsaou's notes about running on ubuntu 14 from #8905 ? will have to add some security warnings (essentially you are disabling security so a hostile intermediary could steal data you are uploading such as metrics).

@prologic
Copy link
Contributor

Probably a good idea. Should also mention that using the Static install is a valid solution here (not even a work-around either).

Copy link
Contributor

@zack-shoylev zack-shoylev left a comment

Choose a reason for hiding this comment

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

Let's add the additional notes as suggested before merging.

@joelhans
Copy link
Contributor Author

@zack-shoylev Of course—that's been the plan all along. I'm hoping to get it done today.

export CFLAGS="-DACLK_SSL_ALLOW_SELF_SIGNED"
```

Then install Netdata using your method of choice.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this will leave your Netdata Cloud connection vulnerable to man-in-the-middle attacks and is not recommended.

^ I want to make sure this is accurate. @amoss or @prologic

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to review this very carefully...

Copy link
Contributor

@zack-shoylev zack-shoylev left a comment

Choose a reason for hiding this comment

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

wanted to make one thing very clear for the workaround

claim/README.md Outdated Show resolved Hide resolved
Comment on lines 199 to 210
#### Ubuntu 14.04 LTS

To use the `CFLAGS` workaround on Ubuntu 14.04 LTS, you must first install `libuv1-dev` via a PPA:

```bash
add-apt-repository ppa:acooks/libwebsockets6
apt-get update
apt-get install libuv1-dev
```

Then proceed with the CFLAGS workaround described above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is already handled by #8916 not sure we need to duplicate the code in our docuemtnation as we don't do this for similar reasons for handling other systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That PR is not merged yet. Is some other fix already present for Ubuntu 14.04?

Comment on lines +211 to +215
### CentOS 6 and CentOS 8

To install the Agent on certain CentOS and RHEL systems, you must enable non-default repositories, such as EPEL or
PowerTools, to gather hard dependencies. See the [CentOS 6](/packaging/installer/methods/manual.md#centos-rehel-6-x) and
[CentOS 8](/packaging/installer/methods/manual.md#centos-rehel-8-x) sections for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here? The installer takes care of this, it even tells the user and prompts then (optionally).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here because of the linked sections (6 and 8), which were added some months ago to explain how to install on these distros, since users were running into issues.

If those sections are no longer needed, let me know and I'll remove them in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just add a note to say that our installer automatically takes cares of this if you use our kickstart.sh.

@joelhans
Copy link
Contributor Author

joelhans commented Jun 1, 2020

@prologic Given that I wrote this PR on Friday and #9198 was then merged over the weekend, @zack-shoylev and I will need to take some time to figure out how to redesign this entire PR to accommodate those changes. More soon.

@prologic
Copy link
Contributor

prologic commented Jun 3, 2020

Thanks @joelhans :) Let me know if you need any help!

@joelhans
Copy link
Contributor Author

joelhans commented Jun 3, 2020

@prologic This is actually ready for another review when you have time. I chatted with @zack-shoylev off GitHub about what's in the PR right now, and while he's happy with it, the opinions of you and @amoss are what really count!

Copy link
Contributor

@prologic prologic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@amoss amoss left a comment

Choose a reason for hiding this comment

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

Looks good - really nice explanation of the security issue. LGTM

@joelhans
Copy link
Contributor Author

joelhans commented Jun 4, 2020

@zack-shoylev Can you hop in here and make any last requests or approve? I think merging is blocked, in part, because you requested changes. Thanks!

@prologic prologic merged commit b1c96ce into netdata:master Jun 4, 2020
@prologic
Copy link
Contributor

prologic commented Jun 4, 2020

You had two approvals. Merged.

@joelhans joelhans deleted the fix-openssl branch June 4, 2020 14:13
Saruspete pushed a commit to Saruspete/netdata that referenced this pull request Jun 17, 2020
* Add notes section to installer with OpenSSL

* Add LibreSSL and Clang

* Libre

* Remove LibreSSL warning

* Add sections about CFLAGS to install and claim

* Update

* Retrigger CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs area/packaging Packaging and operating systems support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Properly document that we have issues with multiple installed versions of OpenSSL.
7 participants