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.

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

Jump to bottom

test/utils/image: Support a single repository #93510

Merged
merged 1 commit into from Jan 14, 2021
Merged

test/utils/image: Support a single repository #93510

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions test/utils/image/BUILD
View file
Open in desktop
Expand Up @@ -28,4 +28,5 @@ go_test(
name = "go_default_test",
srcs = ["manifest_test.go"],
embed = [":go_default_library"],
deps = ["//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library"],
)
113 changes: 104 additions & 9 deletions test/utils/image/manifest.go
View file
Open in desktop
Expand Up @@ -17,9 +17,12 @@ limitations under the License.
package image

import (
"crypto/sha256"
"encoding/base64"
"fmt"
"io/ioutil"
"os"
"regexp"
"strings"

yaml "gopkg.in/yaml.v2"
Expand Down Expand Up @@ -100,7 +103,12 @@ func initReg() RegistryList {
}

var (
registry = initReg()
registry = initReg()

// PrivateRegistry is an image repository that requires authentication
PrivateRegistry = registry.PrivateRegistry

// Preconfigured image configs
dockerLibraryRegistry = registry.DockerLibraryRegistry
dockerGluster = registry.DockerGluster
e2eRegistry = registry.E2eRegistry
Expand All @@ -112,14 +120,10 @@ var (
sigStorageRegistry = registry.SigStorageRegistry
gcrReleaseRegistry = registry.GcrReleaseRegistry
invalidRegistry = registry.InvalidRegistry
// PrivateRegistry is an image repository that requires authentication
PrivateRegistry = registry.PrivateRegistry
sampleRegistry = registry.SampleRegistry

// Preconfigured image configs
imageConfigs = initImageConfigs()
sampleRegistry = registry.SampleRegistry
microsoftRegistry = registry.MicrosoftRegistry

microsoftRegistry = registry.MicrosoftRegistry
imageConfigs, originalImageConfigs = initImageConfigs()
)

const (
Expand Down Expand Up @@ -206,7 +210,7 @@ const (
WindowsServer
)

func initImageConfigs() map[int]Config {
func initImageConfigs() (map[int]Config, map[int]Config) {
configs := map[int]Config{}
configs[Agnhost] = Config{promoterE2eRegistry, "agnhost", "2.21"}
configs[AgnhostPrivate] = Config{PrivateRegistry, "agnhost", "2.6"}
Expand Down Expand Up @@ -248,9 +252,83 @@ func initImageConfigs() map[int]Config {
configs[VolumeGlusterServer] = Config{e2eVolumeRegistry, "gluster", "1.0"}
configs[VolumeRBDServer] = Config{e2eVolumeRegistry, "rbd", "1.0.1"}
configs[WindowsServer] = Config{microsoftRegistry, "windows", "1809"}

// if requested, map all the SHAs into a known format based on the input
originalImageConfigs := configs
if repo := os.Getenv("KUBE_TEST_REPO"); len(repo) > 0 {
configs = GetMappedImageConfigs(originalImageConfigs, repo)
}

return configs, originalImageConfigs
}

// GetMappedImageConfigs returns the images if they were mapped to the provided
// image repository.
func GetMappedImageConfigs(originalImageConfigs map[int]Config, repo string) map[int]Config {
configs := make(map[int]Config)
for i, config := range originalImageConfigs {
switch i {
case InvalidRegistryImage, AuthenticatedAlpine,
AuthenticatedWindowsNanoServer, AgnhostPrivate:
// These images are special and can't be run out of the cloud - some because they
// are authenticated, and others because they are not real images. Tests that depend
// on these images can't be run without access to the public internet.
configs[i] = config
continue
}

// Build a new tag with a the index, a hash of the image spec (to be unique) and
// shorten and make the pull spec "safe" so it will fit in the tag
configs[i] = getRepositoryMappedConfig(i, config, repo)
}
return configs
}

var (
reCharSafe = regexp.MustCompile(`[^\w]`)
reDashes = regexp.MustCompile(`-+`)
)

// getRepositoryMappedConfig maps an existing image to the provided repo, generating a
// tag that is unique with the input config. The tag will contain the index, a hash of
// the image spec (to be unique) and shorten and make the pull spec "safe" so it will
// fit in the tag to allow a human to recognize the value. If index is -1, then no
// index will be added to the tag.
func getRepositoryMappedConfig(index int, config Config, repo string) Config {
Comment on lines +292 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to understand this use case -- is it because image tags are mutable and we want to make sure it's consistent through hash?

alternatively, I'm not sure if I follow the what scenario is implied with "safe" in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since tags have character limits that are shorter than the limit of a full image reference, the hash is to dedup similar images with very long names from one registry. I.e. something like:

docker.io/my-very-long-repo-name/very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-long:lots_of_characters_in_tags
docker.io/my-very-long-repo-name/very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-very-long:lots_of_characters_in_tags_2

needs to be able to fit into the character limit of an image tag, but we want to be able to guarantee that if you had two of these you could collapse them into a tag in the same repo (shorten the pull_spec, add the hash that uniquifies the rest)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, I missed this part of PR description:

k8s.gcr.io/prometheus-to-sd:v0.5.0 to quay.io/openshift/community-e2e-images:e2e-30-k8s-gcr-io-prometheus-to-sd-v0-5-0-6JI59Yih4oaj3oQOjRfhyQ

I just realized that all images are defined with the same name (i.e. community-e2e-images) but with different tags -- that part doesn't seem very intuitive to me. Am I missing something from this proposed approach vs having each image be its own entry in the repository? i.e. quay.io/openshift-k8s-e2e/prometheus-to-sd:v0.5.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly simplification - you may not own 50 different repositories (like on docker.io you couldn't mirror the same way). So then if you have a choice between one repo vs multiple - having one repo is more flexibility if you have quota or cost limits (i.e. on docker.io can't have 30 private repos without paying). So really trying to simplify it all down as much as possible to be most flexible for both people who have cloud provider registries, or docker.io accounts, or quay accounts, or maybe even on prem where you are only allowed to touch one repo in your artifactory server.

Also, think about if you're testing this. If you want to sync for 1.20, and for 1.21, you might actually want to test it first. To test it, you want a way to catch "oops". If you're mirroring into individual repos, then your oops factor could be much higher. If each mirror is one repo (and the code is designed to ensure the same image ends up in the same spot) then you can share AND separate. Once you've tested you can delete that repo - that's easier to do than 50 different repos. A repo only having one type of image is just a convention, it's not a hard rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for elaborating. The scenario of mirroring to docker.io isn't something that I considered nor has a use case for. My perspective on supporting this repository override is to make it easy to run conformance on airgapped clusters, which typically is locked to self-hosted container registries, making the cost to host individual image repository somewhat negligible (vs SaaS registry).

I do see value in minimizing "oops" factor though -- I agree it'd be a good idea to have such system!

parts := strings.SplitN(repo, "/", 2)
registry, name := parts[0], parts[1]

pullSpec := config.GetE2EImage()

h := sha256.New()
h.Write([]byte(pullSpec))
hash := base64.RawURLEncoding.EncodeToString(h.Sum(nil)[:16])

shortName := reCharSafe.ReplaceAllLiteralString(pullSpec, "-")
shortName = reDashes.ReplaceAllLiteralString(shortName, "-")
maxLength := 127 - 16 - 6 - 10
if len(shortName) > maxLength {
shortName = shortName[len(shortName)-maxLength:]
}
var version string
if index == -1 {
version = fmt.Sprintf("e2e-%s-%s", shortName, hash)
} else {
version = fmt.Sprintf("e2e-%d-%s-%s", index, shortName, hash)
}

return Config{
registry: registry,
name: name,
version: version,
}
}

// GetOriginalImageConfigs returns the configuration before any mapping rules.
func GetOriginalImageConfigs() map[int]Config {
return originalImageConfigs
}

// GetImageConfigs returns the map of imageConfigs
func GetImageConfigs() map[int]Config {
return imageConfigs
Expand Down Expand Up @@ -282,6 +360,23 @@ func ReplaceRegistryInImageURL(imageURL string) (string, error) {
countParts := len(parts)
registryAndUser := strings.Join(parts[:countParts-1], "/")

if repo := os.Getenv("KUBE_TEST_REPO"); len(repo) > 0 {
index := -1
for i, v := range originalImageConfigs {
if v.GetE2EImage() == imageURL {
index = i
break
}
}
last := strings.SplitN(parts[countParts-1], ":", 2)
config := getRepositoryMappedConfig(index, Config{
registry: parts[0],
name: strings.Join([]string{strings.Join(parts[1:countParts-1], "/"), last[0]}, "/"),
version: last[1],
}, repo)
return config.GetE2EImage(), nil
}

switch registryAndUser {
case "gcr.io/kubernetes-e2e-test-images":
registryAndUser = e2eRegistry
Expand Down
28 changes: 28 additions & 0 deletions test/utils/image/manifest_test.go
View file
Open in desktop
Expand Up @@ -18,7 +18,10 @@ package image

import (
"fmt"
"reflect"
"testing"

"k8s.io/apimachinery/pkg/util/diff"
)

type result struct {
Expand Down Expand Up @@ -135,3 +138,28 @@ func TestReplaceRegistryInImageURL(t *testing.T) {
})
}
}

func TestGetOriginalImageConfigs(t *testing.T) {
if len(GetOriginalImageConfigs()) == 0 {
t.Fatalf("original map should not be empty")
}
}

func TestGetMappedImageConfigs(t *testing.T) {
originals := map[int]Config{
0: {registry: "docker.io", name: "source/repo", version: "1.0"},
}
mapping := GetMappedImageConfigs(originals, "quay.io/repo/for-test")

actual := make(map[string]string)
for i, mapping := range mapping {
source := originals[i]
actual[source.GetE2EImage()] = mapping.GetE2EImage()
}
expected := map[string]string{
"docker.io/source/repo:1.0": "quay.io/repo/for-test:e2e-0-docker-io-source-repo-1-0-72R4aXm7YnxQ4_ekf1DrFA",
}
if !reflect.DeepEqual(expected, actual) {
t.Fatal(diff.ObjectReflectDiff(expected, actual))
}
}