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

Ensure read access to the run image selected by extensions #1364

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented Jun 17, 2024

Summary

When an extension switches the run image to one listed in the run.toml, the read access check is not triggered, as it is done in the analyze phase. This change will iterate over the requested run image and it's mirrors, to select the first accessible one.

Release notes


Related

Resolves #___


Context

@pbusko pbusko requested a review from a team as a code owner June 17, 2024 08:40
@pbusko pbusko force-pushed the ensure-read-access-after-extensions branch from 7032ccb to a1dc432 Compare June 17, 2024 08:52
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
Co-authored-by: Pavel Busko <pavel.busko@sap.com>
@pbusko pbusko force-pushed the ensure-read-access-after-extensions branch from a1dc432 to 499f875 Compare June 17, 2024 08:56
@loewenstein-sap
Copy link

@jabrown85 With your approval, what's missing to get it merged and released? Do we need @natalieparellano's approval in addition?

@jabrown85
Copy link
Contributor

@loewenstein-sap yeah - I think I'd like a second set of eyes on this. What comes to mind now is that this phase may need registry auth, which isn't typically a thing in the untrusted phase executions. CNB_REGISTRY_AUTH is set for exporter/analyzer/restorer but is not mounted for builder/detector. I'm not sure that value should be set but that also doesn't mean generator phase shouldn't check access for what should be assumed to be unauthenticated image access.

There are other registry related settings like CNB_INSECURE_REGISTRIES, CNB_USE_LAYOUT, CNB_LAYOUT_DIR to think about as well.

@natalieparellano
Copy link
Member

What comes to mind now is that this phase may need registry auth, which isn't typically a thing in the untrusted phase executions.

This is true. Can we move this check to the restore phase?

Signed-off-by: Pavel Busko <pavel.busko@sap.com>
@pbusko pbusko force-pushed the ensure-read-access-after-extensions branch from 67cf4bc to f4f38a2 Compare June 19, 2024 09:23
@pbusko
Copy link
Contributor Author

pbusko commented Jun 19, 2024

the check has been moved to the restore phase, however I couldn't find any unit tests for the file, and the acceptance tests are using docker daemon only, which makes it hard to test without mock registry.

@loewenstein-sap
Copy link

@natalieparellano ^^

@natalieparellano
Copy link
Member

#1338 would have made this PR much easier to test :) unfortunately, that PR is blocked on me having some bandwidth to add units.

Apologies for being slow here, I've just returned from vacation. I'll take a look today.

@@ -50,6 +50,7 @@ func (r *restoreCmd) DefineFlags() {
cli.FlagBuildImage(&r.BuildImageRef)
}
cli.FlagAnalyzedPath(&r.AnalyzedPath)
cli.FlagRunPath(&r.RunPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry to raise the specter of another Platform API version bump, but I think we need to gate this on Platform 0.14 also (an raise another spec PR).

Copy link
Contributor Author

@pbusko pbusko Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbusko pbusko force-pushed the ensure-read-access-after-extensions branch from b1a2cce to af38ac5 Compare July 3, 2024 10:19
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
@natalieparellano natalieparellano merged commit a02be03 into buildpacks:main Jul 3, 2024
8 checks passed
@natalieparellano
Copy link
Member

I may have been too hasty in merging this one and #1346, as we're due for a go version / dependencies patch and this should be in the next minor...

@pbusko pbusko deleted the ensure-read-access-after-extensions branch July 3, 2024 17:49
@natalieparellano natalieparellano added this to the lifecycle 0.20.0 milestone Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants