tests/kola: remove dtb exceptions in upgrade test SELinux checks #3237

HuijingHei · 2024-10-30T05:45:33Z

dustymabe · 2024-10-30T12:16:07Z

I'm not really sure if we can remove these exceptions yet. After an upgrade there are still two deployments around. Does the OSTree fix fix the old deployment too such that we guarantee all files in /boot/ have the right labels?

You could try applying this patch to the upgrade test and then running it to see what the results are:

fedora-coreos-config/tests/kola/upgrade/extended/test.sh

Lines 22 to 28 in 7ef7e40

    
           # An example invocation for this test would look like: 
        
           # ``` 
        
           # cosa buildfetch --stream=next --build=34.20210904.1.0 --artifact=qemu 
        
           # cosa decompress --build=34.20210904.1.0 
        
           # cosa kola run --build=34.20210904.1.0 --tag extended-upgrade 
        
           # ```

so you would buildfetch an older build and start the test with that.

HuijingHei · 2024-10-31T03:09:31Z

Does the OSTree fix fix the old deployment too such that we guarantee all files in /boot/ have the right labels?

Unluckily no, sorry that I just did testing using the fixed ostree and ignore the old deployment which already has the wrong label, in this case the old deployment will keep the wrong label as ostree will not copy xattrs for devicetree refer to ostreedev/ostree#3323.

dustymabe · 2024-10-31T11:59:47Z

Does the OSTree fix fix the old deployment too such that we guarantee all files in /boot/ have the right labels?

Unluckily no, sorry that I just did testing using the fixed ostree and ignore the old deployment which already has the wrong label.

I think that's fine and expected. It just means we can't remove this exception for now.

OR maybe we could rpm-ostree cleanup -r first before doing the check. maybe that would work.

dustymabe · 2024-10-31T18:01:58Z

Thinking about this a little more can we make it conditional? Would you mind testing something like on aarch64?

diff --git a/tests/kola/upgrade/extended/test.sh b/tests/kola/upgrade/extended/test.sh
index 4c880d96..3bca64aa 100755
--- a/tests/kola/upgrade/extended/test.sh
+++ b/tests/kola/upgrade/extended/test.sh
@@ -162,10 +162,34 @@ wait-for-coreos-fix-selinux-labels() {
         echo "Waited for coreos-fix-selinux-labels.service to finish"
 }
 
+# Check if the rollback deployment has the dtb copy fix and if not
+# delete the rollback deployment.
+# https://github.com/coreos/fedora-coreos-tracker/issues/1808
+#
+# NOTE: we can drop this once the newest barrier release for all
+# streams is newer than 41.20241028.x.x.
+drop_rollback_if_needed() {
+    # The dtb copy issue was only ever an issue ever on aarch64
+    [ "$(arch)" != 'aarch64' ] && return
+    # We only need to drop if the rollback deployment is older than
+    # when the fixed ostree was included. It should be fixed in the
+    # next round of releases after 41.20241028. Note 41.20241028.0.0
+    # is not a real build and uses `0` for the stream identifier, but
+    # should sort accordingly.
+    previous=$(rpm-ostree status --json | jq -r '.deployments[] | select(.booted == false).version')
+    if ! vergt $previous '41.20241028.0.0'; then
+        echo "Dropping rollback deployment because it could have mislabeled dtb files"
+        rpm-ostree cleanup -r
+    fi
+}
+
 selinux-sanity-check() {
     # First make sure the migrations/fix script has finished if this is the boot
     # where the fixes are taking place.
     wait-for-coreos-fix-selinux-labels
+    # Next if the dtb copy fix wasn't present in the previous deployment
+    # then we'll delete that deployment before proceeding
+    drop_rollback_if_needed
     # Verify SELinux labels are sane. Migration scripts should have cleaned
     # up https://github.com/coreos/fedora-coreos-tracker/issues/1772
     unlabeled="$(find /sysroot -context '*unlabeled_t*' -print0 | xargs --null -I{} ls -ldZ '{}')"

HuijingHei · 2024-11-01T01:22:23Z

Would you mind testing something like on aarch64?

You are correct, the dtb files are only on aarch64, will try to find out how to run upgrade tests on aarch64, ~~much appreciated if any pointer? Thanks!~~

HuijingHei · 2024-11-01T11:20:22Z

Do testing with this PR failed when running from 41.20241024.1.0 to latest 41.20241027.1.0 (which does not include the fixed ostree), maybe in this case we should not cleanup the rollback and still need the dtb exceptions?
steps:

# git clone -b remove-selinux-exception https://github.com/HuijingHei/fedora-coreos-config config

# cosa buildfetch --build=41.20241024.1.0 --artifact=qemu --arch=aarch64
# cosa decompress --build=41.20241024.1.0
# cosa kola run --build=41.20241024.1.0 --tag extended-upgrade -E ./config/

dustymabe · 2024-11-01T11:30:09Z

from 41.20241024.1.0 to latest 41.20241027.1.0 (which does not include the fixed ostree)

right. it won't work until the next builds on the prod streams, but you can test against the non-prod streams now, which is what all future tests will do:

$ cat tmp/target_stream.bu 
variant: fcos
version: 1.0.0
storage:
  files:
    - path: /etc/target_stream
      mode: 0644
      contents:
        inline: |
          next-devel
$ cosa kola run --build=41.20241024.1.0 --tag extended-upgrade --append-butane tmp/target_stream.bu --debug

This ^^ should work without your latest commit.

HuijingHei · 2024-11-01T11:31:44Z

tests/kola/upgrade/extended/test.sh

+    # is not a real build and uses `0` for the stream identifier, but
+    # should sort accordingly.
+    previous=$(rpm-ostree status --json | jq -r '.deployments[] | select(.booted == false).version')
+    if (! vergt $previous '41.20241028.0.0') && vergt $version '41.20241028.0.0'; then


If previous version is older and current still does not include the fixed ostree, will not cleanup the rollback, not sure if this makes sense.

can you see if this works: #3237 (comment)

With the patch in #3237 (comment), failed but not related the code, seems the next-devel latest is 41.20241027.10.0, maybe should use testing-devel

yes! sorry. we disalbed next-devel earlier this week and I guess it didn't have the new OSTree RPM yet. Can you try with testing-devel?

Yes, tried testing-devel, no lucky.
I think it is because when doing upgrade based on older version (with unfixed ostree), which will make the two deployments both with wrong label, and cleanup the rollback only remove the previous deployment.

ahh. I see, yes. in that case dropping the rollback deployment doesn't really give us anything.

what we can do is still apply the exceptions but only in case that it's needed. something like:

diff --git a/tests/kola/upgrade/extended/test.sh b/tests/kola/upgrade/extended/test.sh index 0e069e5a..39bc3278 100755 --- a/tests/kola/upgrade/extended/test.sh +++ b/tests/kola/upgrade/extended/test.sh @@ -162,10 +162,34 @@ wait-for-coreos-fix-selinux-labels() { echo "Waited for coreos-fix-selinux-labels.service to finish" } +# Check if the rollback deployment has the dtb copy fix, which +# means that the dtb files should have the correct SELinux labels. +# https://github.com/coreos/fedora-coreos-tracker/issues/1808 +# +# NOTE: we can drop this once the newest barrier release for all +# streams is newer than 41.20241028.x.x. +has_dtb_cp_fix() { + # The dtb copy issue was only ever an issue ever on aarch64 + [ "$(arch)" != 'aarch64' ] && return 0 + # We have the dtb copy fix if the rollback deployment is newer than + # when the fixed ostree was included. It should be fixed in the + # next round of releases after 41.20241028. Note 41.20241028.0.0 + # is not a real build and uses `0` for the stream identifier, but + # should sort accordingly. + previous=$(rpm-ostree status --json | jq -r '.deployments[] | select(.booted == false).version') + if ! vergt $previous '41.20241028.0.0'; then + return 0 + else + return 1 + fi +} + selinux-sanity-check() { # First make sure the migrations/fix script has finished if this is the boot # where the fixes are taking place. wait-for-coreos-fix-selinux-labels + # Check to see if we have the dtb copy fix + has_dtb_cp_fix || add_dtb_exception='true'$ # Verify SELinux labels are sane. Migration scripts should have cleaned # up https://github.com/coreos/fedora-coreos-tracker/issues/1772 unlabeled="$(find /sysroot -context '*unlabeled_t*' -print0 | xargs --null -I{} ls -ldZ '{}')" @@ -207,7 +231,9 @@ selinux-sanity-check() { # https://github.com/coreos/fedora-coreos-tracker/issues/1806 [[ "${path}" =~ /etc/selinux/targeted/active/ ]] && continue # https://github.com/coreos/fedora-coreos-tracker/issues/1808 - [[ "${path}" =~ /boot/ostree/.*/dtb ]] && continue + if [ "${add_dtb_exception}" == 'true' ]; then + [[ "${path}" =~ /boot/ostree/.*/dtb ]] && continue + fi if [[ "${exceptions[$path]:-noexception}" == 'noexception' ]]; then echo "Unexpected mislabeled file found: ${path}" found="1"

If update to following, test is passed.

+ if ! vergt $previous '41.20241028.0.0'; then + return 1 + else + return 0 + fi

tests/kola/upgrade/extended/test.sh

HuijingHei · 2024-11-01T13:13:00Z

Does the OSTree fix fix the old deployment too such that we guarantee all files in /boot/ have the right labels?

Unluckily no, sorry that I just did testing using the fixed ostree and ignore the old deployment which already has the wrong label, in this case the old deployment will keep the wrong label as ostree will not copy xattrs for devicetree refer to ostreedev/ostree#3323.

Sorry I misunderstood this, if booting os with unfixed ostree, then upgrade, then dtb files in both deployments will have the wrong labels. If in this case we get the fixed ostree, if do another upgrade, the old deployment will keep the wrong label, not sure if we have such upgrade test, for example upgrade A -> B -> C.

Fixes coreos/fedora-coreos-tracker#1808

dustymabe

LGTM

HuijingHei · 2024-11-01T14:41:18Z

[coreos-assembler]$ cosa kola run --build=41.20241024.1.0 --tag extended-upgrade --append-butane tmp/target_stream.bu --debug -E ./config/
--- PASS: ext.config.upgrade.extended (350.46s)
PASS, output in tmp/kola

HuijingHei added the hold label Oct 31, 2024

HuijingHei force-pushed the remove-selinux-exception branch from a6ce4e3 to 3eefeae Compare October 31, 2024 12:48

HuijingHei force-pushed the remove-selinux-exception branch from 3eefeae to 55370f9 Compare November 1, 2024 03:06

HuijingHei force-pushed the remove-selinux-exception branch from 55370f9 to 710456c Compare November 1, 2024 11:29

HuijingHei commented Nov 1, 2024

View reviewed changes

tests/kola/upgrade/extended/test.sh Outdated Show resolved Hide resolved

tests/kola: remove dtb exceptions in upgrade test SELinux checks

e573d80

Fixes coreos/fedora-coreos-tracker#1808

HuijingHei force-pushed the remove-selinux-exception branch from 710456c to e573d80 Compare November 1, 2024 14:30

dustymabe approved these changes Nov 1, 2024

View reviewed changes

dustymabe enabled auto-merge (rebase) November 1, 2024 14:35

dustymabe merged commit 8b6e75e into coreos:testing-devel Nov 1, 2024
3 checks passed

HuijingHei mentioned this pull request Nov 15, 2024

tests/upgrade: drop the rollback deployment on aarch64 #3261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/kola: remove dtb exceptions in upgrade test SELinux checks #3237

tests/kola: remove dtb exceptions in upgrade test SELinux checks #3237

HuijingHei commented Oct 30, 2024

dustymabe commented Oct 30, 2024

HuijingHei commented Oct 31, 2024

dustymabe commented Oct 31, 2024

dustymabe commented Oct 31, 2024

HuijingHei commented Nov 1, 2024 •

edited

Loading

HuijingHei commented Nov 1, 2024

dustymabe commented Nov 1, 2024 •

edited

Loading

HuijingHei Nov 1, 2024

dustymabe Nov 1, 2024

HuijingHei Nov 1, 2024

dustymabe Nov 1, 2024 •

edited

Loading

HuijingHei Nov 1, 2024

dustymabe Nov 1, 2024

HuijingHei Nov 1, 2024

HuijingHei commented Nov 1, 2024

dustymabe left a comment

HuijingHei commented Nov 1, 2024

tests/kola: remove dtb exceptions in upgrade test SELinux checks #3237

tests/kola: remove dtb exceptions in upgrade test SELinux checks #3237

Conversation

HuijingHei commented Oct 30, 2024

dustymabe commented Oct 30, 2024

HuijingHei commented Oct 31, 2024

dustymabe commented Oct 31, 2024

dustymabe commented Oct 31, 2024

HuijingHei commented Nov 1, 2024 • edited Loading

HuijingHei commented Nov 1, 2024

dustymabe commented Nov 1, 2024 • edited Loading

HuijingHei Nov 1, 2024

Choose a reason for hiding this comment

dustymabe Nov 1, 2024

Choose a reason for hiding this comment

HuijingHei Nov 1, 2024

Choose a reason for hiding this comment

dustymabe Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

HuijingHei Nov 1, 2024

Choose a reason for hiding this comment

dustymabe Nov 1, 2024

Choose a reason for hiding this comment

HuijingHei Nov 1, 2024

Choose a reason for hiding this comment

HuijingHei commented Nov 1, 2024

dustymabe left a comment

Choose a reason for hiding this comment

HuijingHei commented Nov 1, 2024

HuijingHei commented Nov 1, 2024 •

edited

Loading

dustymabe commented Nov 1, 2024 •

edited

Loading

dustymabe Nov 1, 2024 •

edited

Loading