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

OpenSSL update instructions - feedback wanted #42395

Open
mhdawson opened this issue Mar 18, 2022 · 8 comments
Open

OpenSSL update instructions - feedback wanted #42395

mhdawson opened this issue Mar 18, 2022 · 8 comments
Labels
openssl Issues and PRs related to the OpenSSL dependency.

Comments

@mhdawson
Copy link
Member

To help me get up to speed on the instructions to update OpenSSL in
maintaining-openssl.md
I ran through the instructions and built some docker files that would automate almost the full process.

These are in https://github.com/mhdawson/node-openssl-utils (one branch for main and one for 16.x so far)

What I like about them is that I can edit the version of openssl to pull in, start the process with bash buildit.sh and come back an hour later(on a big machine, likely longer on something more typical) to a docker container with the changes applied, including our standardize commit comments.

It does not push a PR, as I'm not sure it would be successful enough for that to make sense but it keeps the results of running the tests and you can confirm that all modified files were included in the commits. If there are any additional changes required (like the changes o the OpenSSL tests in the last security release) you can cherry pick those over and re-run the tests easily/quickly It is also handy for doing a quick sniff check of a PR by confirming that the same number of files have been modified as expected.

I wondering if we'd want to do one of the following

  1. Integrate using docker to do the full build into the main instructions (it is already partially used for the non-linux instructions).
  2. transfer the node-openssl-utils into the node.js repo as a set of examples/utilities that can be referenced in the instructions to help people if they want to use docker.

Automating in an action would not quite be the same in my mind as you don't get the container which you can use to carry out the last few steps or cherry pick commits if necessary. If we think the complete process might work frequently enough then an action running similar steps could make sense.

They are not quite complete as I can see that I missed some of the externalization of the git email and user name but I'd do that if 1) or 2) above make sense.

@richardlau, @hassaanp since I know you have done some of the recently OpenSSL updates.

@VoltrexKeyva VoltrexKeyva added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 19, 2022
@richardlau
Copy link
Member

I'm broadly in favour of adding more dockerisation of the process as an option -- I actually run the existing container (the non-Linux instructions) on Linux when I've done OpenSSL updates.

From my POV the main complexity of the OpenSSL update process is that it is slightly different for the older release lines using upstream OpenSSL vs the more recent release lines that use the quic fork.

@mhdawson
Copy link
Member Author

@richardlau in terms of

From my POV the main complexity of the OpenSSL update process is that it is slightly different for the older release lines using upstream OpenSSL vs the more recent release lines that use the quic fork.

A branch for those older versions which has the changes needed for the older versions is what I had in mind.

@RafaelGSS
Copy link
Member

@mhdawson I'm wondering if there is a chance to run those commands in a self-hosted CI, which may create the PR automatically.

I don't know how much time would it take in a normal CI (I assume a lot), but, we can use dedicated machines for those work. Is it feasible?

@mhdawson
Copy link
Member Author

@RafaelGSS I had wondered about setting up CI to run/generate the PR. What I'm not sure of is how many times it will just work or need some sort of tweaks. If it will just work 80% of the time then the CI/generate PR makes a lot of sense to be and that would be a good thing for us to add to be able to respond to OpenSSL updates quickly.

This was my comment above:

It does not push a PR, as I'm not sure it would be successful enough for that to make sense but it keeps the results of running the tests and you can confirm that all modified files were included in the commits. If there are any additional changes required (like the changes o the OpenSSL tests in the last security release) you can cherry pick those over and re-run the tests easily/quickly.

@richardlau what's your take on how often it should just work based on your past experience?

@richardlau
Copy link
Member

@mhdawson In the past it has generally just worked but the last two updates did not and caused test failures.

@mhdawson
Copy link
Member Author

If most of the past record is that it just works then trying to automate it makes sense to me.

@hassaanp
Copy link
Contributor

Sorry for the late response here.

My 2 cents: I have contributed to the ssl updates a few times across 2 years and I would say that apart from a single time (there was a change in the file structure), the steps were pretty much consistent and therefore can be automated.

Having a CI that creates a PR with the openssl update sounds neat. But I think we should also talk about the workflow in case the CI fails. I think in the event of failure, the pipeline should open a new issue making sure the relevant stakeholders are tagged to ensure necessary steps can be taken.

@RafaelGSS
Copy link
Member

FYI I'll be working on that action in the next coming days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

No branches or pull requests

5 participants