Add automation for updating `OpenSSL` dependency by facutuesca · Pull Request #45605 · nodejs/node · GitHub

Add a Github Action that checks for new versions of the OpenSSL library, and creates a PR to update it if a newer version than the one present in the repo is found.

Refs: nodejs/security-wg#828

Review requested:

@nodejs/actions

cc @RafaelGSS @mhdawson

Not related to this PR, but we should add a task somwhere to see if we can address this. Having it be more repeatable would be good.

again likely a follow on, but I wonder if we need to pin/capture what versions of perl etc was used. The same might be asked about the environment/os level in the action but. Not a blocker for this PR as we don't specify/fix those when people are running manually today.

LGTM

I'm adding the labels to not backport it to versions that use openssl 1

I'll review it properly tomorrow, but I believe is better to have a workflow_dispatch instead. At least I think we'll have more control on those updates. Wdyt @mhdawson?

This will only create one commit instead of two? I don't know if that will make the PR harder to review.

This will only create one commit instead of two? I don't know if that will make the PR harder to review.

Are the changes manually reviewed? Since the update consists a copy-paste of an entire repo followed by regenerating config files programatically, the resulting PRs are pretty big. For example, the latest update in this PR #45309 has changes in 800 files, with +15,840-5,825 lines changed.

I ask because the current CI workflow for creating PRs to update dependencies assume that all dependencies will be updated using a single commit. If there's value in having two commits for OpenSSL we can do it, but it will require modifying that workflow to have a special case for OpenSSL separate from the rest of the dependencies.

If there's value in having two commits for OpenSSL we can do it, but it will require modifying that workflow to have a special case for OpenSSL separate from the rest of the dependencies.

There's a value in two commits. The second commit is arch build and normally some errors are caught in this phase (for example, nasm build issue - mentioned in the maintaining guide) so you just need to re-do the second one.

I'll review it properly tomorrow, but I believe is better to have a workflow_dispatch instead. At least I think we'll have more control on those updates. Wdyt @mhdawson?

@RafaelGSS do you mean we should have workflow_dispatch and then trigger it manually, or just have that as another option for triggering and update?

I'll review it properly tomorrow, but I believe is better to have a workflow_dispatch instead. At least I think we'll have more control on those updates. Wdyt @mhdawson?

@RafaelGSS do you mean we should have workflow_dispatch and then trigger it manually, or just have that as another option for triggering and update?

Yes, the first option. Triggering it manually through workflow_dispatch.

I think having the ability to trigger it manually would be good, but having a PR auto generated and available when there is a new release seems useful, provided it does not require manual work most of the time. I think for OpenSSL we probably want to try to keep as up to date as possible in the main branch.

LGTM. I think we are good to go. As discussed previously in a SecurityWG meeting, adding the support for two commits will be a bit out-of-scope.

Therefore, I'm fine moving forward with it, and if needed, we include support for multiple commits.

The current documentation does say to land as 2 commits so unless its hard/lots of work to do that I'm thinking we should do that now versus later. @richardlau what's your take?

The current documentation does say to land as 2 commits so unless its hard/lots of work to do that I'm thinking we should do that now versus later. @richardlau what's your take?

My take is that the conservative option is the status quo -- two commits. On one hand that allows people to validate the first commit -- you should be able to get the exact same files. Keeping that property may be helpful in the future in terms of potential work around supply chain/verifying our dependencies.

The second commit has timestamps and local directories written into it which makes it harder to compare/validate.

On the other hand the two commit approach has always been the exception to the

All commits should be self-contained, meaning every commit should pass all tests.

guidance in our collaborator guide.

My take is that the conservative option is the status quo -- two commits. On one hand that allows people to validate the first commit -- you should be able to get the exact same files. Keeping that property may be helpful in the future in terms of potential work around supply chain/verifying our dependencies.

@facutuesca I think it would be best to try to implement the 2 commits unless its going to take a larger effort.

It will require including that support in our current tooling (dep_updaters)

@mhdawson @RafaelGSS @richardlau

I changed it to create 2 commits instead of one. I re-requested reviews since the new approach is different enough that it should be reviewed as a new PR. The things to keep in mind are:

The GH workflow for updating OpenSSL is now separate from the one used for the other dependencies. The OpenSSL steps (having multiple commits, each after running a different command) is different enough from the other dependencies that it can't be generalised to a single workflow without adding too much complexity
The bash script to update OpenSSL was also updated. Now it has two sub-commands (download and regenerate), corresponding to the two commits. A full update needs both sub-commands to be run, and the README was updated to reflect that

(edit: for some reason I cannot correctly re-request reviews, maybe a permissions issue since I also don't have permissions to add reviewers)

I think this block the automatic backport. We won't be able to update v14 and v16 at same time considering the race condition of the branch creation.

So, I'll include the labels dont-land-on-vX and after landing you will need to manually backport and change the branch name to actions/tools-update-openssl-vX.

Does that make sense to you?

This is the same logic used for the other deps. See this line from the current dependency update action:

node/.github/workflows/tools.yml

Line 153 in 9b1ed04

    
           branch: actions/tools-update-${{ matrix.id }}  # Custom branch *just* for this Action.

As far as I understand, this would be already an issue for corepack, eslint, etc right? Since the branches created for them are called tools-update-corepack and tools-update-eslint without mentioning the Node version.

Do you know how those are handled currently?

I don't think we have different versions of eslint/corepack in each release line. Could you check if the bot has ever opened PR targeting other branches instead of main?

The bot will not because the automatic scheduled runs only happen on the default branch:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

Scheduled workflows run on the latest commit on the default or base branch.

this also blocks the automatic backport to v14.x. We use the original repo on v14.x

Does v14.x have any existing action for dependency updates? I could not find any

Currently there is no automation via Actions for dependency updates for any version of Node.js outside of the main branch.

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828

ping @mhdawson

LGTM

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828 PR-URL: #45605 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Landed in c4103c1

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828 PR-URL: #45605 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Nov 24, 2022

mhdawson reviewed Dec 1, 2022

View reviewed changes

mhdawson approved these changes Dec 1, 2022

View reviewed changes

RafaelGSS added dont-land-on-v14.x labels Dec 2, 2022

RafaelGSS reviewed Dec 2, 2022

View reviewed changes

RafaelGSS reviewed Dec 8, 2022

View reviewed changes

facutuesca mentioned this pull request Dec 8, 2022

Automate updates of all dependencies nodejs/security-wg#828

Closed

23 tasks

RafaelGSS approved these changes Dec 16, 2022

View reviewed changes

facutuesca requested review from RafaelGSS and mhdawson and removed request for mhdawson December 19, 2022 18:37

facutuesca requested review from mhdawson and removed request for RafaelGSS and mhdawson December 19, 2022 19:13

facutuesca requested review from RafaelGSS and mhdawson and removed request for RafaelGSS December 19, 2022 19:13

mhdawson requested a review from RafaelGSS December 19, 2022 20:14

RafaelGSS reviewed Dec 22, 2022

View reviewed changes

RafaelGSS added the dont-land-on-v18.x label Dec 22, 2022

marco-ippolito approved these changes Feb 24, 2023

View reviewed changes

facutuesca and others added 2 commits February 24, 2023 18:58

tools: add automation for updating OpenSSL dependency

a30e72c

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828

tools: add openssl version to commit

1e1cd4b

RafaelGSS approved these changes Feb 27, 2023

View reviewed changes

mhdawson approved these changes Mar 2, 2023

View reviewed changes

mhdawson closed this Mar 2, 2023

marco-ippolito mentioned this pull request Mar 5, 2023

deps: update OpenSSL to 3.0.8-quic1 #46957

Closed

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

Add automation for updating `OpenSSL` dependency by facutuesca · Pull Request #45605 · nodejs/node · GitHub

Uh oh!

Uh oh!

Add automation for updating OpenSSL dependency - #45605

Add automation for updating OpenSSL dependency#45605facutuesca wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom facutuesca:openssl-ci-updateCopy head branch name to clipboard

Conversation

facutuesca commented Nov 24, 2022

Uh oh!

nodejs-github-bot commented Nov 24, 2022

Uh oh!

facutuesca commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

RafaelGSS commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Dec 2, 2022

Uh oh!

facutuesca commented Dec 6, 2022

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Dec 16, 2022

Uh oh!

RafaelGSS commented Dec 16, 2022

Uh oh!

mhdawson commented Dec 16, 2022

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Dec 16, 2022

Uh oh!

richardlau commented Dec 16, 2022

Uh oh!

mhdawson commented Dec 16, 2022

Uh oh!

RafaelGSS commented Dec 17, 2022

Uh oh!

facutuesca commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RafaelGSS commented Feb 27, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Mar 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Add automation for updating OpenSSL dependency#45605
facutuesca wants to merge 2 commits into
nodejs:mainfrom
facutuesca:openssl-ci-update

facutuesca commented Nov 24, 2022 •
edited
Loading

RafaelGSS commented Dec 2, 2022 •
edited
Loading

facutuesca commented Dec 19, 2022 •
edited
Loading