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

openobserve: 0.7.2 -> 0.8.1 #289522

Merged
merged 2 commits into from
Feb 18, 2024
Merged

Conversation

risicle
Copy link
Contributor

@risicle risicle commented Feb 17, 2024

Description of changes

https://nvd.nist.gov/vuln/detail/CVE-2024-25106

Also needed to be switched to using darwin.apple_sdk_11_0 to continue building on darwin x86_64 (tested on macos 12), which means it needs an entry in all-packages.nix despite being by-name.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@risicle risicle added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Feb 17, 2024
@happysalada
Copy link
Contributor

happysalada commented Feb 17, 2024

it's uncanny, I was just about to do an update myself.
https://github.com/NixOS/nixpkgs/pull/289526/files
in general this looks good except for one problem that I ran into
I needed this in env for the ui part
NODE_OPTIONS="--max-old-space-size=8192";
so right after line 41.

@risicle
Copy link
Contributor Author

risicle commented Feb 17, 2024

Ah.. that problem went away for me when I built on a bigger machine, but if that does the trick all the better.

@happysalada
Copy link
Contributor

@risicle the pkgs/by-name/check is failing , are we still good to merge ?

@risicle
Copy link
Contributor Author

risicle commented Feb 17, 2024

I think so, I don't think it's clever enough to detect this situation (though I thought they were going to fix that 😕)

@delroth delroth added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Feb 17, 2024
@risicle risicle merged commit e015f24 into NixOS:master Feb 18, 2024
28 of 29 checks passed
@camelpunch
Copy link
Contributor

@happysalada @risicle I'm getting the nixpkgs-check-by-name failure on an unrelated PR. Is it worth looking into what changed in this one?

@camelpunch
Copy link
Contributor

You can reproduce this on master nixpkgs with pkgs/test/nixpkgs-check-by-name/scripts/run-local.sh master:

$ pkgs/test/nixpkgs-check-by-name/scripts/run-local.sh master
Using HEAD commit 573cead3dd6f4ca10ed1bc7dc3589bc85e150ab5
Creating Git worktree for the HEAD commit in /run/user/1000/tmp.fQkHak1Hbk/merged.. Done
Fetching base branch master to compare against.. 573cead3dd6f4ca10ed1bc7dc3589bc85e150ab5
Creating Git worktree for the base branch in /run/user/1000/tmp.fQkHak1Hbk/base.. Done
Merging base branch into the HEAD commit in /run/user/1000/tmp.fQkHak1Hbk/merged.. 573cead3dd6f4ca10ed1bc7dc3589bc85e150ab5
Reading pinned nixpkgs-check-by-name revision from pinned-tool.json.. d934204a0f8d9198e1e4515dd6fec76a139c87f0
Creating Git worktree for the nixpkgs-check-by-name revision in /run/user/1000/tmp.fQkHak1Hbk/tool-nixpkgs.. Done
Building/fetching nixpkgs-check-by-name..
/nix/store/5fjdmbiziyp47gfc9kmfgvxdlzd6bba1-nixpkgs-check-by-name
Running nixpkgs-check-by-name..
pkgs.openobserve: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/op/openobserve/package.nix { ... }` with a non-empty second argument.
Validation failed, see above errors
Cleaning up.. Done

@adisbladis
Copy link
Member

Because this broke the by-name check I reverted it in #289655.

@happysalada
Copy link
Contributor

Alright here is what I suggest for the next step for openobserve.
0.8.2-rc.1 has just been released, it's probably going to be released in a couple of days. I'm going to wait for that (I'm being lazy I know), and then push an update to my PR.
I'm wondering if we can just fix the apple_sdk_11_0 in the package.nix file for now. I've pushed that change to my branch and will test that when 0.8.2 comes about.

@happysalada
Copy link
Contributor

After another look, the vulnerability affects anything prior to 0.8.0, so 0.8.1 should be fine to merge. I'm going to test again my PR on x86_64 darwin and will merge if it passes.

@infinisil
Copy link
Member

infinisil commented Feb 18, 2024

I think so, I don't think it's clever enough to detect this situation (though I thought they were going to fix that 😕)

All unintentional pkgs/by-name false positives have been fixed by now. The failure is here because all packages defined via pkgs/by-name must use the standard callPackage, that's always been the case.

I fully admit that the error messages aren't great for this case, and they should be more actionable, and also indicate whether merging would break master. I'll work on this next week, see NixOS/nixpkgs-vet#6 for more updates.

I'll open a new PR to do this update that won't cause problems in a bit.

Edit: #289751

infinisil added a commit to tweag/nixpkgs that referenced this pull request Feb 19, 2024
This reverts commit ac637ef.

The original PR (NixOS#289522) broke the
`pkgs/by-name` check on master,
so it was reverted in NixOS#289655.

This reapplies the original commits and makes sure that the
`pkgs/by-name` check works by moving it out of `pkgs/by-name`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants