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

azure container registry runner #1107

merged 40 commits into from
Oct 15, 2018

Conversation

cedrickring
Copy link
Contributor

@cedrickring cedrickring commented Oct 7, 2018

Added an acr runner similar to az acr build to build and push images on acr.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@cedrickring
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@codecov-io
Copy link

codecov-io commented Oct 7, 2018

Codecov Report

Merging #1107 into master will decrease coverage by 1.91%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1107      +/-   ##
==========================================
- Coverage   44.09%   42.17%   -1.92%     
==========================================
  Files          84       89       +5     
  Lines        3667     3995     +328     
==========================================
+ Hits         1617     1685      +68     
- Misses       1895     2142     +247     
- Partials      155      168      +13
Impacted Files Coverage Δ
pkg/skaffold/schema/versions.go 56.75% <ø> (+3.63%) ⬆️
pkg/skaffold/runner/runner.go 49.59% <0%> (-0.62%) ⬇️
pkg/skaffold/watch/changes.go 79.62% <0%> (-18.25%) ⬇️
pkg/skaffold/build/local/jib_gradle.go 25% <0%> (-17.11%) ⬇️
pkg/skaffold/build/local/local.go 48.33% <0%> (-8.49%) ⬇️
pkg/skaffold/build/local/jib_maven.go 26.66% <0%> (-8.34%) ⬇️
pkg/skaffold/kubernetes/wait.go 24.44% <0%> (-4.97%) ⬇️
pkg/skaffold/sync/sync.go 90.32% <0%> (-4.68%) ⬇️
cmd/skaffold/app/cmd/config.go 0% <0%> (ø) ⬆️
pkg/skaffold/jib/jib_gradle.go 100% <0%> (ø) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a651b8f...2bf4e10. Read the comment docs.

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

Thanks for adding another managed cloud builder to Skaffold! I've added feedback on a few things that could be improved. Feel free to disagree!


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.

}
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.


// 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.

pkg/skaffold/build/acr/azure_storage.go Show resolved Hide resolved
// on Azure Container Registry
type AzureContainerBuild struct {
Credentials AzureContainerBuildCredentials `yaml:"credentials,omitempty"`
ContainerRegistry string `yaml:"containerRegistry,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could a few of those field have sensible default values or be, by default, derived from the image name. Like if I push an image called myregistry.azurecr.io/skaffold-example would it make sense to set containerRegistry to myregistry by default?

That's something we do on GCB to make sure the Skaffold config is as short as possible and contains as few duplication as required.

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.

Yes, I can get the container registry name from the image name, but the resource group needs to be provided separately since it may differ from the registry name.

@@ -16,6 +16,17 @@
revision = "050b16d2314d5fc3d4c9a51e4cd5c7468e77f162"
version = "v0.17.0"

[[projects]]
digest = "1:fd5f5c00789da5a6ae78b6c6050eccea46bcd4a519a4913ac82d22bafd7fd368"
name = "github.com/Azure/azure-sdk-for-go"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should github.com/Azure/azure-sdk-for-go be added as a top level constraint in Gopkg.toml`?

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.

I don't think that's necessary. I'm using a versioned import path for azure sdk. See here

@@ -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.)

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.

@@ -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.

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

LGTM

@dgageot
Copy link
Contributor

dgageot commented Oct 12, 2018

@nkubala @priyawadhwa @r2d4 Could one of you TAL?
@cedrickring One thing that is missing is the ability to interrupt the build. It's also missing on the kaniko builder but was implemented in the GCB builder. I'm ok to add that in another PR though

@cedrickring
Copy link
Contributor Author

@dgageot I'd like to do that in another PR. I'm not sure if it's possible to stop the build actually. But I'll dig into it later.

Copy link
Contributor

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

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

Two small comments. Feel free to merge now and open an issue to fix them later, as I don't think they should matter too much.

"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 getImageTagWithoutFQDN(imageTag string) string {
return imageTag[strings.Index(imageTag, "/")+1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think theres a better way to do this using the go-containerregistry. Can you look at an example of the other builders

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test or two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip with go-containerregistry 👍

@@ -107,6 +107,19 @@ build:
# namespace: default
# timeout: 20m

# Docker artifacts can be built on an 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.

I think integration/examples/annotated-skaffold.yaml should be modified instead (or in addition to this one), so that when we copy over config changes we don't lose this.

@cedrickring
Copy link
Contributor Author

Should be ready to merge imo

@dgageot dgageot merged commit 9efeeac into GoogleContainerTools:master Oct 15, 2018
@priyawadhwa
Copy link
Contributor

Hey @cedrickring -- we just released support for custom builders and skaffold. If you're still interested in using skaffold with Azure, this could be an option for you! Here's an example using a custom build script and associated docs. If you end up trying it out, please open an issue if you run into any trouble, and let us know if it works for you!

@cedrickring
Copy link
Contributor Author

@priyawadhwa sounds really cool! I'd love to create a custom builder with Azure. Is there a way to specify a e.g. GitHub repo for the builder? So if I want to share my custom builder with others, they don't need to clone it everywhere they need it.

@priyawadhwa
Copy link
Contributor

hey @cedrickring , not as of now, but I've updated it as a TODO in #2009

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.

6 participants