Make GCR image provider available #1344

nakabonne · 2021-01-04T08:41:34Z

What this PR does / why we need it:
This PR allows us to use GCR as an image provider for public and private images.

No client package provided under the cloud.google.com/go package because GCR simply implements the Docker Registry API V2. So we've used the docker/distribution package before, but I found GCR API wraps it to show the tags' uploaded time, that's why the package was replaced with google/go-containerregistry. This package has some problem so I'd happy to address them as another PR.

Which issue(s) this PR fixes:

Fixes #1205
Fixes #1206
Fixes #1204
Fixes #1207

Does this PR introduce a user-facing change?:

NONE

nakabonne · 2021-01-04T08:55:57Z

/workspace/bazel_out/sandbox/linux-sandbox/1473/execroot/pipe/pkg/app/piped/imageprovider/gcr/gcr.go:100:3: undefined: google.WithContext

https://kapetanios.io/builds/3aa6c2c5-cfee-46b0-ad58-98e175497c49/step-0#L91

Quite a long time melted away due to this error. Other properties (including google.WithAuth() etc) can resolve, but only this one has been not found. This method is newly added relatively, but included v0.3.0 which is specified in go.mod and repositories.bzl. Anyone know?

nakabonne · 2021-01-04T10:58:17Z

pkg/app/piped/imageprovider/gcr/gcr.go

-	return auth.NewAuthorizer(manager, authHandlers...)
+	return &model.ImageRef{
+		ImageName: *image,
+		// TODO: Enable to specify the tag if multiple tags are associated to an image


I'm looking into using regexp to specify which tag will be used. It would be nice to add the tagRegex field to ImageWatcher config under the .pipe/ directory.

apiVersion: pipecd.dev/v1beta1 kind: ImageWatcher spec: targets: - image: gcr.io/pipecd/foo provider: my-gcr filePath: foo/deployment.yaml field: $.spec.template.spec.containers[0].image tagRegex: ^([0-9]+)\.([0-9]+)\.([0-9]+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-]+)?$

Alternatively, simply providing useSemver or something like that might be nice.

This option is needed right now for us whose images' have multiple tags: https://gcr.io/pipecd/server

It would be wonderful if you could leverage pubsub instead of polling every 5 minutes for ImageWatcher, but I understand that you would need to do something specific for each registry, since everything implements this slightly differently.

@jonjohnsonjr Thank you for taking a look at our project! You're quite right. We don't have to be worried about the pull rate limit if we could support the event hooks for cloud-provider-specific. However, we decided not to do that so as to reduce the burden on users and avoid complexity.

FYI: Alternatively, we're looking into a completely different solution to deal with the rate limit: #1341

khanhtc1202 · 2021-01-05T01:55:12Z

@nakabonne looks like the error comes from here

/workspace/bazel_out/sandbox/linux-sandbox/1622/execroot/pipe/pkg/app/piped/imageprovider/gcr/gcr.go:100:3: undefined: google.WithContext

Could you check 👀

nakabonne · 2021-01-05T02:07:02Z

@khanhtc1202 Yup, that has caused me pain for a long time.

nghialv · 2021-01-05T04:14:42Z

@nakabonne Is that happening at your local environment too?

nakabonne · 2021-01-05T04:16:03Z

@nghialv Yup, it's happening on my laptop as well.

nghialv · 2021-01-05T04:16:43Z

And for sure you can check whether that function was included in the vendor directory or not.

nakabonne · 2021-01-05T04:17:07Z

That is included in the vendor directory.

nakabonne · 2021-01-05T04:18:36Z

❯ cat vendor/github.com/google/go-containerregistry/pkg/v1/google/options.go
// Copyright 2018 Google LLC All Rights Reserved.
//
// 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 google

import (
	"context"
	"net/http"

	"github.com/google/go-containerregistry/pkg/authn"
	"github.com/google/go-containerregistry/pkg/logs"
)

// WithTransport is a functional option for overriding the default transport
// on a remote image
func WithTransport(t http.RoundTripper) ListerOption {
	return func(l *lister) error {
		l.transport = t
		return nil
	}
}

// WithAuth is a functional option for overriding the default authenticator
// on a remote image
func WithAuth(auth authn.Authenticator) ListerOption {
	return func(l *lister) error {
		l.auth = auth
		return nil
	}
}

// WithAuthFromKeychain is a functional option for overriding the default
// authenticator on a remote image using an authn.Keychain
func WithAuthFromKeychain(keys authn.Keychain) ListerOption {
	return func(l *lister) error {
		auth, err := keys.Resolve(l.repo.Registry)
		if err != nil {
			return err
		}
		if auth == authn.Anonymous {
			logs.Warn.Printf("No matching credentials were found for %q, falling back on anonymous", l.repo.Registry)
		}
		l.auth = auth
		return nil
	}
}

// WithContext is a functional option for overriding the default
// context.Context for HTTP request to list remote images
func WithContext(ctx context.Context) ListerOption {
	return func(l *lister) error {
		l.ctx = ctx
		return nil
	}
}

nakabonne · 2021-01-05T04:21:08Z

@nghialv Have you ever been facing this or something similar before?

nghialv · 2021-01-05T04:29:06Z

No. It is weird. Is it ok to call other functions from that package, e.g. WithTransport(nil)?
Finally, can you try to make clean and make dep to clean some bad cache, not sure.

nakabonne · 2021-01-05T04:31:17Z

Is it ok to call other functions from that package, e.g. WithTransport(nil)?

Yes, other methods call goes well.

Finally, can you try to make clean and make dep to clean some bad cache, not sure.

Actually already done, but doesn't go well...

nghialv · 2021-01-05T04:59:46Z

Let me take a try at my local.

nakabonne · 2021-01-05T05:00:35Z

It helps me a lot. Thanks...

nghialv · 2021-01-05T08:09:35Z

I have just found that the downloaded source inside bazel-pipe/external/com_github_google_go_containerregistry is not containing that function. Maybe gazelle downloaded the wrong version.

nakabonne · 2021-01-05T08:11:56Z

I see. I'd be happy to look into why it downloads wrong one.

nghialv · 2021-01-05T09:04:58Z

I think somehow it was overridden by gazelle dependencies or rules_go dependencies. (I am finding it).
Btw, It will work by specifying the go_repository
https://github.com/pipe-cd/pipe/pull/1344/files#diff-c8bacb964b6cf475b85b8fa254a2769dcf34f28b813437048f877aa3ffa3d71bR777-R782

before this line
https://github.com/pipe-cd/pipe/blob/master/WORKSPACE#L31

khanhtc1202 · 2021-01-05T09:49:40Z

not sure will it help or not but update bazel-gazelle could be made as an option, I guess 🤔
https://github.com/bazelbuild/bazel-gazelle/releases/tag/v0.22.3

nghialv · 2021-01-05T09:55:13Z

Yes, I am trying to upgrade gazelle, rules_go.

nakabonne · 2021-01-06T00:22:53Z

I'm now at the stage of learning Bazel and its surrounding ecosystem.

nakabonne · 2021-01-06T04:43:48Z

Status: for now waiting for #1346; once got merged check it out and proceed next solution depending on the result.

nghialv · 2021-01-06T04:49:21Z

I tried to add the containerregistry package to #1346 but nothing has changed. For a better way, I think we need more time on investigation. So as a temporary solution, let's apply the way I posted above
#1344 (comment)

Moving this part from repositories.bzl to WORKSPACE (before loading the rules_go's dependencies)

    go_repository(
        name = "com_github_google_go_containerregistry",
        importpath = "github.com/google/go-containerregistry",
        sum = "h1:+vqpHdgIbD7xSeufHJq0iuAx7ILcEeh3fR5Og2nW1R0=",
        version = "v0.3.0",
    )

nakabonne · 2021-01-06T05:02:43Z

@nghialv Thanks for spending your time. I'm now trying to do that.

pipecd-bot · 2021-01-06T05:47:21Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Investigate why "go-containerregistry" package is overridden

https://github.com/pipe-cd/pipe/blob/87a5339df3d21d6eae9ef4630c05e7bda004cbe6/WORKSPACE#L46-L49

This was created by todo plugin since "TODO:" was found in `87a5339` when #1344 was merged. cc: @nakabonne.

2. Use pagination to retrieve image tags from GCR

https://github.com/pipe-cd/pipe/blob/87a5339df3d21d6eae9ef4630c05e7bda004cbe6/pkg/app/piped/imageprovider/gcr/gcr.go#L105-L108

This was created by todo plugin since "TODO:" was found in `87a5339` when #1344 was merged. cc: @nakabonne.

3. Enable to specify the tag if multiple tags are associated to an image

https://github.com/pipe-cd/pipe/blob/87a5339df3d21d6eae9ef4630c05e7bda004cbe6/pkg/app/piped/imageprovider/gcr/gcr.go#L132-L135

This was created by todo plugin since "TODO:" was found in `87a5339` when #1344 was merged. cc: @nakabonne.

pipecd-bot · 2021-01-06T05:54:14Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

nakabonne · 2021-01-06T05:57:09Z

@nghialv @khanhtc1202 Just applied a workaround. PTAL!

nghialv · 2021-01-06T06:17:25Z

/lgtm

khanhtc1202 · 2021-01-06T06:34:07Z

Thank you
/approve

pipecd-bot · 2021-01-06T06:34:10Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

**What this PR does / why we need it**: This PR gets rid of Bazel's repositories no longer needed by running `gazelle update-repos` with the `-prune` option. For that option, see: https://github.com/bazelbuild/bazel-gazelle#update-repos Also, it reverts the workaround due to #1344 (comment). We can undo it because we just settled on stop watching container registries. If there are some reasons why we didn't use the `-prune` option, I don't mind dropping this patch. **Which issue(s) this PR fixes**: Fixes #1353 **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

Make GCR image provider available

fa1eac8

pipecd-bot added the release-note-none label Jan 4, 2021

pipecd-bot requested review from khanhtc1202 and nghialv January 4, 2021 08:41

pipecd-bot added area/build area/piped size/XL labels Jan 4, 2021

nakabonne commented Jan 4, 2021

View reviewed changes

nakabonne mentioned this pull request Jan 6, 2021

Investigate why "go-containerregistry" package is overridden #1352

Closed

Move go_repository as a workaround

87a5339

pipecd-bot added the lgtm label Jan 6, 2021

pipecd-bot added the approved label Jan 6, 2021

pipecd-bot merged commit d497e38 into master Jan 6, 2021

pipecd-bot deleted the gcr-client branch January 6, 2021 06:41

This was referenced Jan 6, 2021

Investigate why "go-containerregistry" package is overridden #1353

Closed

Use pagination to retrieve image tags from GCR #1354

Closed

Enable to specify the tag if multiple tags are associated to an image #1355

Closed

nakabonne mentioned this pull request Jan 9, 2021

Prune repositories no longer needed #1387

Merged

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

Make GCR image provider available #1344

Make GCR image provider available #1344

Uh oh!

Conversation

nakabonne commented Jan 4, 2021

Uh oh!

nakabonne commented Jan 4, 2021

Uh oh!

nakabonne Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

nakabonne Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

nakabonne Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khanhtc1202 commented Jan 5, 2021

Uh oh!

nghialv commented Jan 5, 2021

Uh oh!

nakabonne commented Jan 6, 2021

Uh oh!

nakabonne commented Jan 6, 2021

Uh oh!

nghialv commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nakabonne commented Jan 6, 2021

Uh oh!

pipecd-bot commented Jan 6, 2021

1. Investigate why "go-containerregistry" package is overridden

This was created by todo plugin since "TODO:" was found in 87a5339 when #1344 was merged. cc: @nakabonne.

2. Use pagination to retrieve image tags from GCR

This was created by todo plugin since "TODO:" was found in 87a5339 when #1344 was merged. cc: @nakabonne.

3. Enable to specify the tag if multiple tags are associated to an image

This was created by todo plugin since "TODO:" was found in 87a5339 when #1344 was merged. cc: @nakabonne.

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

nakabonne commented Jan 6, 2021

Uh oh!

nghialv commented Jan 6, 2021

Uh oh!

khanhtc1202 commented Jan 6, 2021

Uh oh!

nghialv commented Jan 5, 2021 •

edited

Loading

nghialv commented Jan 6, 2021 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `87a5339` when #1344 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `87a5339` when #1344 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `87a5339` when #1344 was merged. cc: @nakabonne.