Skip to content

Feat: send additional build contexts as tar files for remote builds #26628

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+705 −17
Open

Feat: send additional build contexts as tar files for remote builds #26628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 146 additions & 4 deletions pkg/api/handlers/compat/images_build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,20 @@ func genSpaceErr(err error) error {
}

func BuildImage(w http.ResponseWriter, r *http.Request) {
var multipart bool
if hdr, found := r.Header["Content-Type"]; found && len(hdr) > 0 {
contentType := hdr[0]
multipart = strings.HasPrefix(contentType, "multipart/form-data")
if multipart {
contentType = "multipart/form-data"
}
Comment on lines +50 to +53
Copy link
Member

Choose a reason for hiding this comment

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

I think the proper way to parse the Content Type header is to use mime.ParseMediaType

switch contentType {
case "application/tar":
logrus.Infof("tar file content type is %s, should use \"application/x-tar\" content type", contentType)
case "application/x-tar":
break
case "multipart/form-data":
logrus.Debugf("Received multipart/form-data: %s", contentType)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this always going to log "Received multipart/form-data: multipart/form-data"?

default:
if utils.IsLibpodRequest(r) {
utils.BadRequest(w, "Content-Type", hdr[0],
Expand Down Expand Up @@ -81,10 +88,21 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
}
}()

contextDirectory, err := extractTarFile(anchorDir, r)
if err != nil {
utils.InternalServerError(w, genSpaceErr(err))
return
var contextDirectory string
var additionalBuildContextsFromMultipart map[string]*buildahDefine.AdditionalBuildContext

if multipart {
contextDirectory, additionalBuildContextsFromMultipart, err = handleMultipartBuild(anchorDir, r)
if err != nil {
utils.InternalServerError(w, fmt.Errorf("handling multipart request: %w", err))
Copy link
Member

Choose a reason for hiding this comment

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

you likely should wrap the error with genSpaceErr(err) here to return a more human friendly error if the server is out of space

return
}
} else {
contextDirectory, err = extractTarFile(anchorDir, r)
if err != nil {
utils.InternalServerError(w, genSpaceErr(err))
return
}
}

runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
Expand Down Expand Up @@ -448,6 +466,14 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
}
}

if additionalBuildContextsFromMultipart != nil {
logrus.Debugf("Merging %d additional contexts from multipart", len(additionalBuildContextsFromMultipart))
for name, ctx := range additionalBuildContextsFromMultipart {
additionalBuildContexts[name] = ctx
logrus.Debugf("Added multipart context %q with path %q", name, ctx.Value)
}
}

var idMappingOptions buildahDefine.IDMappingOptions
if _, found := r.URL.Query()["idmappingoptions"]; found {
if err := json.Unmarshal([]byte(query.IDMappingOptions), &idMappingOptions); err != nil {
Expand Down Expand Up @@ -920,6 +946,122 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
}
}

func handleMultipartBuild(anchorDir string, r *http.Request) (contextDir string, additionalContexts map[string]*buildahDefine.AdditionalBuildContext, err error) {
reader, err := r.MultipartReader()
if err != nil {
return "", nil, fmt.Errorf("failed to create multipart reader: %w", err)
}

additionalContexts = make(map[string]*buildahDefine.AdditionalBuildContext)
for {
part, err := reader.NextPart()
if err == io.EOF {
break
}
if err != nil {
return "", nil, fmt.Errorf("failed to read part: %w", err)
}

fieldName := part.FormName()
fileName := part.FileName()

logrus.Debugf("Processing part: field=%q, file=%q", fieldName, fileName)

switch {
case fieldName == "context" && fileName != "":
Copy link
Member

Choose a reason for hiding this comment

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

Is fileName used at all?

contextPath := filepath.Join(anchorDir, "context")
if err := os.MkdirAll(contextPath, 0755); err != nil {
part.Close()
return "", nil, fmt.Errorf("creating context directory: %w", err)
}

if err := archive.Untar(part, contextPath, nil); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should consider using extractTarFile() here, since it's what handles the default build context archive when the request isn't a multipart form.

part.Close()
return "", nil, fmt.Errorf("extracting main context: %w", err)
}
contextDir = contextPath
logrus.Debugf("Extracted main context to %q", contextDir)
part.Close()

case strings.HasPrefix(fieldName, "buildcontext-url-"):
contextName := strings.TrimPrefix(fieldName, "buildcontext-url-")
urlBytes, err := io.ReadAll(part)
part.Close()
if err != nil {
return "", nil, fmt.Errorf("reading URL value: %w", err)
}
urlValue := string(urlBytes)

logrus.Debugf("Found URL context %q: %s", contextName, urlValue)

tempDir, subDir, err := buildahDefine.TempDirForURL(anchorDir, "buildah", urlValue)
if err != nil {
return "", nil, fmt.Errorf("downloading URL context %q: %w", contextName, err)
}

contextPath := filepath.Join(tempDir, subDir)
additionalContexts[contextName] = &buildahDefine.AdditionalBuildContext{
IsURL: true,
IsImage: false,
Value: contextPath,
DownloadedCache: contextPath,
}
logrus.Debugf("Downloaded URL context %q to %q", contextName, contextPath)

case strings.HasPrefix(fieldName, "buildcontext-image-"):
contextName := strings.TrimPrefix(fieldName, "buildcontext-image-")
imageBytes, err := io.ReadAll(part)
part.Close()
if err != nil {
return "", nil, fmt.Errorf("reading image reference: %w", err)
}
imageRef := string(imageBytes)

logrus.Debugf("Found image context %q: %s", contextName, imageRef)

additionalContexts[contextName] = &buildahDefine.AdditionalBuildContext{
IsImage: true,
IsURL: false,
Value: imageRef,
}
logrus.Debugf("Added image context %q with reference %q", contextName, imageRef)
Comment on lines +986 to +1027
Copy link
Member

Choose a reason for hiding this comment

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

why are these part of the request body at all, would it not be more logically to keep them as query param like before then you don't have to do this special casing at all. Only the actual tar files must be part of the multi part request.


case strings.HasPrefix(fieldName, "buildcontext-local-") && fileName != "":
Copy link
Member

Choose a reason for hiding this comment

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

Is fileName used at all?

contextName := strings.TrimPrefix(fieldName, "buildcontext-local-")
additionalAnchor := filepath.Join(anchorDir, "additional", contextName)
Copy link
Member

Choose a reason for hiding this comment

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

Rule of thumb: when possible, avoid using values supplied by the client when constructing path names. os.MkdirTemp() would be fine here.


if err := os.MkdirAll(additionalAnchor, 0700); err != nil {
part.Close()
return "", nil, fmt.Errorf("creating additional context directory %q: %w", contextName, err)
}

if err := archive.Untar(part, additionalAnchor, nil); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

The archive content's coming from a client. Why isn't it extracted using chrootarchive.Untar(), or extractTarFile()?

part.Close()
return "", nil, fmt.Errorf("extracting additional context %q: %w", contextName, err)
}

additionalContexts[contextName] = &buildahDefine.AdditionalBuildContext{
IsURL: false,
IsImage: false,
Value: additionalAnchor,
}
logrus.Debugf("Extracted additional context %q to %q", contextName, additionalAnchor)
part.Close()

default:
part.Close()
}
}

if contextDir == "" {
return "", nil, fmt.Errorf("no main context provided in multipart form")
}

logrus.Debugf("Successfully parsed multipart form, main context: %q and additional contexts: %v", contextDir, additionalContexts)

return contextDir, additionalContexts, nil
}

func parseNetworkConfigurationPolicy(network string) buildah.NetworkConfigurationPolicy {
if val, err := strconv.Atoi(network); err == nil {
return buildah.NetworkConfigurationPolicy(val)
Expand Down
Loading