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

azure container registry runner #1107

Merged
merged 40 commits into from
Oct 15, 2018
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
21079db
Create new config version to support azure container registry builds
cedrickring Oct 3, 2018
ac5d24c
Added AzureContainerBuild to BuildType
cedrickring Oct 3, 2018
e7f1398
Added builder for azure container registry
cedrickring Oct 6, 2018
a5a5eac
Removed blank line
cedrickring Oct 6, 2018
cac520a
Use buffer pointer instead of buffer in azure blob storage
cedrickring Oct 7, 2018
7e08ed3
ensured all packages
cedrickring Oct 7, 2018
14d691e
Using proper paths inside source tar
cedrickring Oct 7, 2018
13e1155
Use sourceLocation from GetBuildSourceUploadURL
cedrickring Oct 7, 2018
44f869e
Formatting image tags to match azures requirements
cedrickring Oct 7, 2018
d14ee9f
Added build status polling
cedrickring Oct 7, 2018
15c6b3c
Try again if log blob is not available yet
cedrickring Oct 7, 2018
118938f
Fix missing output
cedrickring Oct 7, 2018
0662bc4
Check if response has data before checking for completion
cedrickring Oct 7, 2018
23e517e
always increment log lines
cedrickring Oct 7, 2018
b289bd1
Fix output
cedrickring Oct 7, 2018
f815042
Fix build context
cedrickring Oct 7, 2018
a625bed
Fixed image tag formatting, now using <repository>:<tag>
cedrickring Oct 7, 2018
adf3bcc
Fixed image tag regexp
cedrickring Oct 7, 2018
e9b090a
Renamed method to streamBuildLogs
cedrickring Oct 7, 2018
1e49214
Added example for Azure Container Registry builds
cedrickring Oct 7, 2018
ffe32f3
Fixed merge conflicts
cedrickring Oct 7, 2018
8b60578
Added boilerplate to new files
cedrickring Oct 7, 2018
8528f48
Changed variable names to conform golint
cedrickring Oct 7, 2018
5177b7f
Sorted imports by goimports
cedrickring Oct 7, 2018
783d7fb
Using raw string in regexp
cedrickring Oct 7, 2018
f60d6ab
Sorted imports with goimports
cedrickring Oct 7, 2018
9284d4c
Use version v1alpha4 for config changes
cedrickring Oct 8, 2018
93a3e30
Moved acr example to integration/examples
cedrickring Oct 8, 2018
f49086a
Using golang:alpine as baseimage for acr example
cedrickring Oct 8, 2018
88f8e32
Closing blob upload response now
cedrickring Oct 8, 2018
45dd7ef
Closing blob upload response after checking if an error has occurred
cedrickring Oct 8, 2018
cd32955
Simplified removing FQDN from image tag
cedrickring Oct 10, 2018
f952830
Derive registry name from image tag
cedrickring Oct 10, 2018
efc504d
Merge remote-tracking branch 'upstream/master'
cedrickring Oct 10, 2018
fce9621
Synced upstream
cedrickring Oct 10, 2018
622bbdf
Retrieve bearer token from azure cli if credentials are not provided …
cedrickring Oct 11, 2018
12c32c7
Forgot the error message
cedrickring Oct 11, 2018
9a5f2a4
Using integration/examples for examples instead of examples/
cedrickring Oct 12, 2018
d5dcbf5
Don't export build status header
cedrickring Oct 12, 2018
2bf4e10
Use go-containerregistry to get image tag for acr
cedrickring Oct 12, 2018
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
31 changes: 29 additions & 2 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions examples/acr/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
FROM gcr.io/google-appengine/golang
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, you might want to use a smaller image to accelerate the build. (on GCR, we use gcr.io/* images because some of them are pre-pulled on the build node.)


WORKDIR /go/src/github.com/GoogleCloudPlatform/skaffold
CMD ["./app"]
COPY main.go .
RUN go build -o app main.go
34 changes: 34 additions & 0 deletions examples/acr/README.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
=== Example: Azure Container Registry
Copy link
Contributor

Choose a reason for hiding this comment

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

We are in a nasty chicken and egg situation were a sample config can't be put in examples folder until a release of Skaffold actually supports that config.

You should move the whole sample to integration/examples folder and it will be copied to top level examples next time a release is cut.

:icons: font

This is an example demonstrating

* *building* a single go file app and with a single stage `Dockerfile` using https://docs.microsoft.com/en-us/azure/container-registry/container-registry-tutorial-quick-task[ACR build]
* *tagging* using the default tagPolicy (`gitCommit`)
* *deploying* a single container pod using `kubectl`

ifndef::env-github[]
==== Example files
link:{github-repo-tree}/examples/acr[see on Github icon:github[]]

[source,yaml, indent=3, title=skaffold.yaml]
----
include::skaffold.yaml[]
----

[source,go, indent=3, title=main.go, syntax=go]
----
include::main.go[]
----

[source,docker, indent=3, title=Dockerfile]
----
include::Dockerfile[]
----

[source,yaml, indent=3, title=k8s-pod.yaml]
----
include::k8s-pod.yaml[]
----

endif::[]
8 changes: 8 additions & 0 deletions examples/acr/k8s-pod.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: v1
kind: Pod
metadata:
name: getting-started-acr
spec:
containers:
- name: getting-started
image: registry.azurecr.io/skaffold-example
13 changes: 13 additions & 0 deletions examples/acr/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package main

import (
"fmt"
"time"
)

func main() {
for {
fmt.Println("Hello world!")
time.Sleep(time.Second * 1)
}
}
17 changes: 17 additions & 0 deletions examples/acr/skaffold.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
apiVersion: skaffold/v1alpha3
kind: Config
build:
artifacts:
- imageName: myregistry.azurecr.io/skaffold-example
acr:
containerRegistry: myregistry
resourceGroup: myresourcegroup
credentials:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the credentials be found automatically by Skaffold like it's done for GCB? With GCB, you don't have to configure anything in Skaffold if gcloud is properly authenticated to the right project.

I thing both scenario make sense :

  • One were you assume the user's machine is properly configured. In that case Skaffold can have the minimal configuration.
  • One were we prefer to configure Skaffold explicitly. We should add that mode to the GCP builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could create an Authorizer which uses the bearer token from the azure cli config.

So the priority would be: try to use provided credentials, if they're not present try to authenticate with bearer from the config, if the bearer has expired throw an error. I could try to call the azure cli to refresh the bearer though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I managed to retrieve all required information from the Azure CLI and other methods, which makes the config basically as empty as:

acr: {}

You can still provide the credentials, but you don't have to specify the resourceGroup and registryName explicitly.

subscriptionId: subscription-id
clientId: client-id
clientSecret: client-secret
tenantId: tenant-id
deploy:
kubectl:
manifests:
- k8s-*
178 changes: 178 additions & 0 deletions pkg/skaffold/build/acr/acr_build.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
/*
Copyright 2018 The Skaffold Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package acr

import (
"bufio"
"context"
"io"
"net/http"
"regexp"
"time"

cr "github.com/Azure/azure-sdk-for-go/services/containerregistry/mgmt/2018-09-01/containerregistry"
"github.com/Azure/go-autorest/autorest/azure/auth"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/pkg/errors"
)

const BuildStatusHeader = "x-ms-meta-Complete"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think you can leave this unexported


func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*latest.Artifact) ([]build.Artifact, error) {
return build.InParallel(ctx, out, tagger, artifacts, b.buildArtifact)
}

func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) {
client := cr.NewRegistriesClient(b.Credentials.SubscriptionID)
authorizer, err := auth.NewClientCredentialsConfig(b.Credentials.ClientID, b.Credentials.ClientSecret, b.Credentials.TenantID).Authorizer()
if err != nil {
return "", errors.Wrap(err, "authorizing client")
}
client.Authorizer = authorizer

result, err := client.GetBuildSourceUploadURL(ctx, b.ResourceGroup, b.ContainerRegistry)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that give <> urls for two artifacts that are built in parallel?

Copy link
Contributor Author

@cedrickring cedrickring Oct 8, 2018

Choose a reason for hiding this comment

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

Unfortunately not. When uploading to this URL, it saves the tar to a specific location result.RelativePath which has to be passed to the DockerBuildRequest as sourceLocation.

And it only returns a single URL.

if err != nil {
return "", errors.Wrap(err, "build source upload url")
}
blob := NewBlobStorage(*result.UploadURL)

err = docker.CreateDockerTarGzContext(blob.Buffer, artifact.Workspace, artifact.DockerArtifact)
if err != nil {
return "", errors.Wrap(err, "create context tar.gz")
}

err = blob.UploadFileToBlob()
if err != nil {
return "", errors.Wrap(err, "upload file to blob")
}

imageTag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, &tag.Options{
Digest: util.RandomID(),
ImageName: artifact.ImageName,
})
if err != nil {
return "", errors.Wrap(err, "create fully qualified image name")
}

imageTag, err = getImageTagWithoutFQDN(imageTag)
if err != nil {
return "", errors.Wrap(err, "get azure image tag")
}

buildRequest := cr.DockerBuildRequest{
ImageNames: &[]string{imageTag},
IsPushEnabled: &[]bool{true}[0], //who invented bool pointers
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice trick! You can use util.BoolPtr(tue) instead.

SourceLocation: result.RelativePath,
Platform: &cr.PlatformProperties{
Variant: cr.V8,
Os: cr.Linux,
Architecture: cr.Amd64,
},
DockerFilePath: &artifact.DockerArtifact.DockerfilePath,
Type: cr.TypeDockerBuildRequest,
}
future, err := client.ScheduleRun(ctx, b.ResourceGroup, b.ContainerRegistry, buildRequest)
if err != nil {
return "", errors.Wrap(err, "schedule build request")
}

run, err := future.Result(client)
if err != nil {
return "", errors.Wrap(err, "get run id")
}
runID := *run.RunID

runsClient := cr.NewRunsClient(b.Credentials.SubscriptionID)
runsClient.Authorizer = client.Authorizer
logURL, err := runsClient.GetLogSasURL(ctx, b.ResourceGroup, b.ContainerRegistry, runID)
if err != nil {
return "", errors.Wrap(err, "get log url")
}

err = streamBuildLogs(*logURL.LogLink, out)
if err != nil {
return "", errors.Wrap(err, "polling build status")
}

return imageTag, nil
}

func streamBuildLogs(logURL string, out io.Writer) error {
offset := int32(0)
for {
resp, err := http.Get(logURL)
if err != nil {
return err
}

if resp.StatusCode == http.StatusNotFound {
//if blob is not available yet, try again
time.Sleep(2 * time.Second)
continue
}

scanner := bufio.NewScanner(resp.Body)
line := int32(0)
for scanner.Scan() {
if line >= offset {
out.Write(scanner.Bytes())
out.Write([]byte("\n"))
offset++
}
line++
}
resp.Body.Close()

if offset > 0 {
switch resp.Header.Get(BuildStatusHeader) {
case "":
continue
case "internalerror":
case "failed":
return errors.New("run failed")
case "timedout":
return errors.New("run timed out")
case "canceled":
return errors.New("run was canceled")
default:
return nil
}
}

time.Sleep(2 * time.Second)
}
}

// ACR needs the image tag in the following format
// <repository>:<tag>
func getImageTagWithoutFQDN(imageTag string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a few test for this piece of code? Could you replace the manual parsing a an image name with something like docker.ParseReference() in github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docker.ParseReference() only provides the BaseName consisting of fqdn/repository, but Azure only needs the repository name. I could extend the function to extract the repository name from the BaseName.

r, err := regexp.Compile(`.*\..*\..*/(.*)`)
if err != nil {
return "", errors.Wrap(err, "create regexp")
}

matches := r.FindStringSubmatch(imageTag)
if len(matches) < 2 {
return "", errors.New("invalid image tag")
}

return matches[1], nil
}
Loading