Cleanups on top of #8212 #8590

mtrmac · 2024-09-11T00:15:52Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Assorted cleanups identified in reviews of #8212 .

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

See individual commit messages for details.

It might be better to split this, perhaps to discuss controversial parts separately, or because tests might be failing.

Does this PR introduce a user-facing change?

None

internal/oci/runtime_vm.go

codecov · 2024-09-11T01:08:06Z

Codecov Report

Attention: Patch coverage is 45.07042% with 39 lines in your changes missing coverage. Please review.

Project coverage is 46.48%. Comparing base (7a9778c) to head (0d68102).
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8590      +/-   ##
==========================================
+ Coverage   46.45%   46.48%   +0.03%     
==========================================
  Files         151      151              
  Lines       21978    21950      -28     
==========================================
- Hits        10209    10203       -6     
+ Misses      10703    10682      -21     
+ Partials     1066     1065       -1

saschagrunert · 2024-09-11T11:25:02Z

/retest

saschagrunert

Thank you!

saschagrunert · 2024-09-11T13:24:19Z

/lgtm cancel

@harche has some review feedback as well.

haircommander · 2024-09-11T13:24:40Z

server/container_create_linux.go

+	// WARNING: This hard-codes an assumption that SignaturePolicyPath set specifically for the namespace is never less restrictive
+	// than the default system-wide policy, i.e. that if an image is successfully pulled, it always conforms to the system-wide policy.


not for this PR: is there some way to confirm this invariant programatically? or change this check to also account for the system-wide one?

There always is ~~(or, well, soon will always be)~~ a system-wide /etc/containers/policy.json. So just a presence wouldn’t work.

It might be possible to load the two policies and confirm that one is a superset of the other (there are no ORs and NOTs, so adding a PolicyRequirement is always more restrictive)… based on reflect.DeepEqual I suppose? I’m not 100% sure but it might work.

Doing that on every operation seems icky to me, maybe that can be cached.

harche · 2024-09-11T13:29:30Z

I was wondering if this PR can be broken into multiple PRs, if that would help in understanding what bug an individual commit is trying to fix.

haircommander · 2024-09-11T13:30:01Z

server/container_restore.go


 	var restoreArchivePath string
 	if restoreStorageImageID != nil {
+		sb, err := s.getPodSandboxFromRequest(ctx, sbID) // Note that we might call getPodSandboxFromRequest with a different sbID later. Is that necessary?


we haven't run

if sbID == "" { // restore into previous sandbox sbID = dumpSpec.Annotations[annotations.SandboxID] ctrID = config.ID } else { ctrID = "" }

yet here (line 147) so i don't think we know for sure what's the sandbox ID yet. that's why I opted to have it below

Yes, but that sandbox ID comes from inside the metadata image.

For simplicity, suppose the node had a policy “deny anything from docker.io”; then any attempt to restore a snapshot using a metadata image from docker.io should fail.

I don’t really know what I’m doing — I’m assuming Kubelet is always passing the same kind of metadata (at least, there’s only effectively a single CreateContainer call site in Kubelet), so using the same sandbox ID as in the non-snapshot-restore “create container” code path is using should be correct here.

But then again, I don’t know how restoring a sandbox ID / namespace from contents of the metadata image follows namespace restrictions, so I might well be missing something.

suppose the node had a policy…

suppose the namespace invoking the operation had a policy, rather.

@adrianreber wdyt?

The code to restore into an existing sandbox ID is a left-over from the initial pull request which supported to restore a pod. This is no longer possible with the current code. Most of the code was removed. This should haven been also removed.

Further discussion to follow in #8706 (comment) .

haircommander · 2024-09-11T13:31:12Z

pkg/config/config_unsupported.go

 	return errdefs.ErrNotImplemented
 }
+
+func (c *RuntimeConfig) ValidatePinnsPath(executable string) error {


should we add a gh action that compiles on mac so you don't need to keep playing catchup each time you commit to cri-o?

I’d personally like that, but if you don’t have it already, you probably don’t need it to be effective in maintaining the software.

(There’s much more in CRI-O that doesn’t compile on Mac; this was only targeted at making internal/storage unit tests run.)

haircommander · 2024-09-11T13:35:25Z

internal/storage/image.go

-	output <- pullImageOutputItem{Result: transports.ImageName(destRef), Name: canonicalRef.Name(), Digest: canonicalRef.Digest()}
+	rawCanonical, ok := canonicalRef.Raw().(reference.Canonical)
+	if !ok {
+		fmt.Fprintf(os.Stderr, "Returned reference %v is not canonical", canonicalRef.Raw().String())


can you use the cri-o/internal/log package here instead of logging right to Stderr?

I see this (and my below comment) are done above, could you cleanup both cases?

Both of these are in pullImageChild, a separate process. The error message is explicitly expected to go to Stderr, and pullImageParent reads that (as errOutput) and turns it into a Go error.

From skimming internal/log, it seems to add Logrus’ typical log metadata; I think that should not be included in the Go error returned from pullImageParent.

haircommander · 2024-09-11T13:36:13Z

internal/storage/image.go

+	rawCanonical, ok := canonicalRef.Raw().(reference.Canonical)
+	if !ok {
+		fmt.Fprintf(os.Stderr, "Returned reference %v is not canonical", canonicalRef.Raw().String())
+		os.Exit(1)


is this a fatal programming error? like can cri-o not recover at all? if so I think we should panic() here. If not, we should find a way to error the image pull

See above, this is in a subprocess and this is the currently-designed error reporting mechanism.

internal/factory/container/container.go

haircommander · 2024-10-22T18:58:31Z

pkg/annotations/internal.go


-	// Image is the container image ID annotation.
-	Image = "io.kubernetes.cri-o.Image"
+	// UserRequestedImage is ann annotation containing the image specified in the container spec


nit: ann -> an

Thanks, fixed.

haircommander · 2024-10-22T19:29:51Z

what would you think about keeping everything but dca8939 and fba4f75 and putting those in a separate PR for discussion? i know those are some of the more important commits, but I don't wanna keep you blocked on all of these for those to be muscled through

Signed-off-by: Miloslav Trmač <[email protected]>

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

…ullImage This is slightly less accurate in that it does not guarantee digest presence, but it guarantees a value to be present. That's actually what some code paths check (and incorrectly handle), so rely on the value presence to simplify. Also push the use of strongly typed values into the very higest layers of the PullImage call stack. Signed-off-by: Miloslav Trmač <[email protected]>

Use a single string for the whole RegistryImageReference, don't split it into a repo+digest only to combine it back. NOTE: The code is still broken, it doesn't correctly parse progress entries. Signed-off-by: Miloslav Trmač <[email protected]>

... and document its semantics in more places. Signed-off-by: Miloslav Trmač <[email protected]>

Signed-off-by: Miloslav Trmač <[email protected]>

It has no non-test users. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2024-10-23T19:41:19Z

what would you think about keeping everything but dca8939 and fba4f75 and putting those in a separate PR for discussion?

Thanks, done. #8706 currently includes the proposals from this PR, a merge commit, and the two commits.

haircommander · 2024-10-25T15:43:32Z

/lgtm

thanks!

mtrmac · 2024-10-25T19:30:48Z

/retest

saschagrunert · 2024-10-28T07:43:24Z

/override ci/prow/e2e-aws-ovn

openshift-ci · 2024-10-28T07:43:29Z

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/e2e-aws-ovn

In response to this:

/override ci/prow/e2e-aws-ovn

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2024-10-28T07:43:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saschagrunert · 2024-10-28T07:45:14Z

Are there any commits within this PR which should be backported to release-1.31 because they fix a regression?

mtrmac · 2024-10-29T12:48:13Z

I don’t think there are any regression fixes here. The net effect should be a minor efficiency improvement by removing no-longer-used operations (including removing a remote registry access, potentially improving reliability), and perhaps removing some small proven-to-be-unreachable code paths.

mtrmac requested review from fidencio and mrunalp as code owners September 11, 2024 00:15

openshift-ci bot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 11, 2024

openshift-ci bot requested review from hasan4791 and klihub September 11, 2024 00:16

mtrmac mentioned this pull request Sep 11, 2024

Image verification for namespaced policies #8212

Merged

mtrmac commented Sep 11, 2024

View reviewed changes

internal/oci/runtime_vm.go Outdated Show resolved Hide resolved

mtrmac force-pushed the post-sigstore-cleanups branch from ad456db to 21a12d4 Compare September 11, 2024 00:36

openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 11, 2024

mtrmac force-pushed the post-sigstore-cleanups branch from 21a12d4 to 4776d32 Compare September 11, 2024 00:53

kwilczynski assigned mtrmac and harche Sep 11, 2024

saschagrunert approved these changes Sep 11, 2024

View reviewed changes

openshift-ci bot assigned saschagrunert Sep 11, 2024

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 11, 2024

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2024

haircommander reviewed Sep 11, 2024

View reviewed changes

internal/factory/container/container.go Outdated Show resolved Hide resolved

haircommander reviewed Oct 22, 2024

View reviewed changes

mtrmac force-pushed the post-sigstore-cleanups branch from 409a7fb to a552d5e Compare October 22, 2024 19:03

mtrmac added 12 commits October 23, 2024 21:14

Fix build on macOS

14f4c64

Signed-off-by: Miloslav Trmač <[email protected]>

Remove no longer used code

ca1b550

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Inline prepareReference into its only caller

899266b

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Add a comment about possible future handling of complex situations.

c91de58

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Better document, and sometimes rename, parameters and return values

a2e29ba

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Simplify BROKEN pullImageOutputItem

fffc734

Use a single string for the whole RegistryImageReference, don't split it into a repo+digest only to combine it back. NOTE: The code is still broken, it doesn't correctly parse progress entries. Signed-off-by: Miloslav Trmač <[email protected]>

Consistently use the UserRequestedImage for the lookup input

3f9b09e

... and document its semantics in more places. Signed-off-by: Miloslav Trmač <[email protected]>

Add a comment warning against repeated lookups

b56ddca

Signed-off-by: Miloslav Trmač <[email protected]>

Add a warning about assuming per-namespace policies are stricter

4c164f5

Signed-off-by: Miloslav Trmač <[email protected]>

After pulling the pause image, use the canonical reference to look it up

06993f4

Signed-off-by: Miloslav Trmač <[email protected]>

Remove the first return value of PullImage

0d68102

It has no non-test users. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the post-sigstore-cleanups branch from a552d5e to 0d68102 Compare October 23, 2024 19:20

mtrmac mentioned this pull request Oct 23, 2024

Cleanups on top of #8212, second part #8706

Merged

openshift-ci bot assigned haircommander Oct 25, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2024

saschagrunert approved these changes Oct 28, 2024

View reviewed changes

openshift-merge-bot bot merged commit 1172884 into cri-o:main Oct 28, 2024
81 of 82 checks passed

mtrmac deleted the post-sigstore-cleanups branch October 29, 2024 12:48

		// WARNING: This hard-codes an assumption that SignaturePolicyPath set specifically for the namespace is never less restrictive
		// than the default system-wide policy, i.e. that if an image is successfully pulled, it always conforms to the system-wide policy.

Cleanups on top of #8212 #8590

Cleanups on top of #8212 #8590

Uh oh!

Conversation

mtrmac commented Sep 11, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

Uh oh!

codecov bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

saschagrunert commented Sep 11, 2024

Uh oh!

saschagrunert left a comment

Choose a reason for hiding this comment

Uh oh!

saschagrunert commented Sep 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harche commented Sep 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haircommander commented Oct 22, 2024

Uh oh!

mtrmac commented Oct 23, 2024

Uh oh!

haircommander commented Oct 25, 2024

Uh oh!

mtrmac commented Oct 25, 2024

Uh oh!

saschagrunert commented Oct 28, 2024

Uh oh!

openshift-ci bot commented Oct 28, 2024

Uh oh!

openshift-ci bot commented Oct 28, 2024

Uh oh!

saschagrunert commented Oct 28, 2024

Uh oh!

codecov bot commented Sep 11, 2024 •

edited

Loading

mtrmac Sep 11, 2024 •

edited

Loading

mtrmac Sep 11, 2024 •

edited

Loading