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

⚠️ API updates based on external review #443

Merged
merged 1 commit into from
Nov 7, 2024
Merged

⚠️ API updates based on external review #443

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
268 changes: 193 additions & 75 deletions api/core/v1alpha1/clustercatalog_types.go
View file
Open in desktop

Large diffs are not rendered by default.

287 changes: 258 additions & 29 deletions api/core/v1alpha1/clustercatalog_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,54 +4,141 @@ import (
"context"
"fmt"
"os"
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
celconfig "k8s.io/apiserver/pkg/apis/cel"
"k8s.io/utils/ptr"
"sigs.k8s.io/yaml"
)

func TestPollIntervalCELValidationRules(t *testing.T) {
func TestImageSourceCELValidationRules(t *testing.T) {
validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
pth := "openAPIV3Schema.properties.spec"
pth := "openAPIV3Schema.properties.spec.properties.source.properties.image"
validator, found := validators["v1alpha1"][pth]
require.True(t, found)

for name, tc := range map[string]struct {
spec ClusterCatalogSpec
spec ImageSource
wantErrs []string
}{
"digest based image ref, poll interval not allowed, poll interval specified": {
spec: ClusterCatalogSpec{
Source: CatalogSource{
Type: SourceTypeImage,
Image: &ImageSource{
Ref: "docker.io/test-image@sha256:asdf98234sd",
PollInterval: &metav1.Duration{Duration: time.Minute},
},
},
"valid digest based image ref, poll interval not allowed, poll interval specified": {
spec: ImageSource{
Ref: "docker.io/test-image@sha256:abcdef123456789abcdef123456789abc",
PollIntervalMinutes: ptr.To(1),
},
wantErrs: []string{
"openAPIV3Schema.properties.spec: Invalid value: \"object\": cannot specify PollInterval while using digest-based image",
"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": cannot specify pollIntervalMinutes while using digest-based image",
},
},
"digest based image ref, poll interval not allowed, poll interval not specified": {
spec: ClusterCatalogSpec{
Source: CatalogSource{
Type: SourceTypeImage,
Image: &ImageSource{
Ref: "docker.io/example/test-catalog@sha256:asdf123",
},
},
"valid digest based image ref, poll interval not allowed, poll interval not specified": {
spec: ImageSource{
Ref: "docker.io/test-image@sha256:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{},
},
"invalid digest based image ref, invalid domain": {
spec: ImageSource{
Ref: "-quay+docker/foo/bar@sha256:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": must start with a valid domain. valid domains must be alphanumeric characters (lowercase and uppercase) separated by the \".\" character.",
},
},
"invalid digest based image ref, invalid name": {
spec: ImageSource{
Ref: "docker.io/FOO/BAR@sha256:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": a valid name is required. valid names must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
},
},
"invalid digest based image ref, invalid digest algorithm": {
spec: ImageSource{
Ref: "docker.io/foo/bar@99-problems:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": digest algorithm is not valid. valid algorithms must start with an uppercase or lowercase alpha character followed by alphanumeric characters and may contain the \"-\", \"_\", \"+\", and \".\" characters.",
},
},
"invalid digest based image ref, too short digest encoding": {
spec: ImageSource{
Ref: "docker.io/foo/bar@sha256:abcdef123456789",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": digest is not valid. the encoded string must be at least 32 characters",
},
},
"invalid digest based image ref, invalid characters in digest encoding": {
spec: ImageSource{
Ref: "docker.io/foo/bar@sha256:XYZxy123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": digest is not valid. the encoded string must only contain hex characters (A-F, a-f, 0-9)",
},
},
"invalid image ref, no tag or digest": {
spec: ImageSource{
Ref: "docker.io/foo/bar",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": must end with a digest or a tag",
},
},
"invalid tag based image ref, tag too long": {
spec: ImageSource{
Ref: fmt.Sprintf("docker.io/foo/bar:%s", strings.Repeat("x", 128)),
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": tag is invalid. the tag must not be more than 127 characters",
},
},
"invalid tag based image ref, tag contains invalid characters": {
spec: ImageSource{
Ref: "docker.io/foo/bar:-foo_bar-",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": tag is invalid. valid tags must begin with a word character (alphanumeric + \"_\") followed by word characters or \".\", and \"-\" characters",
},
},
"valid tag based image ref": {
spec: ImageSource{
Ref: "docker.io/foo/bar:v1.0.0",
},
wantErrs: []string{},
},
"valid tag based image ref, pollIntervalMinutes specified": {
spec: ImageSource{
Ref: "docker.io/foo/bar:v1.0.0",
PollIntervalMinutes: ptr.To(5),
},
wantErrs: []string{},
},
"invalid image ref, only domain with port": {
spec: ImageSource{
Ref: "docker.io:8080",
},
wantErrs: []string{
"openAPIV3Schema.properties.spec.properties.source.properties.image.ref: Invalid value: \"string\": a valid name is required. valid names must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
},
},
"valid image ref, domain with port": {
spec: ImageSource{
Ref: "my-subdomain.docker.io:8080/foo/bar:latest",
},
wantErrs: []string{},
},
"valid image ref, tag ends with hyphen": {
spec: ImageSource{
Ref: "my-subdomain.docker.io:8080/foo/bar:latest-",
},
wantErrs: []string{},
},
Expand All @@ -60,6 +147,143 @@ func TestPollIntervalCELValidationRules(t *testing.T) {
obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&tc.spec) //nolint:gosec
require.NoError(t, err)
errs := validator(obj, nil)
require.Equal(t, len(tc.wantErrs), len(errs), "want", tc.wantErrs, "got", errs)
for i := range tc.wantErrs {
got := errs[i].Error()
assert.Equal(t, tc.wantErrs[i], got)
}
})
}
}

func TestResolvedImageSourceCELValidation(t *testing.T) {
validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
pth := "openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref"
validator, found := validators["v1alpha1"][pth]
require.True(t, found)

for name, tc := range map[string]struct {
spec ImageSource
wantErrs []string
}{
"valid digest based image ref": {
spec: ImageSource{
Ref: "docker.io/test-image@sha256:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{},
},
"invalid digest based image ref, invalid domain": {
spec: ImageSource{
Ref: "-quay+docker/foo/bar@sha256:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": must start with a valid domain. valid domains must be alphanumeric characters (lowercase and uppercase) separated by the \".\" character.",
},
},
"invalid digest based image ref, invalid name": {
spec: ImageSource{
Ref: "docker.io/FOO/BAR@sha256:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": a valid name is required. valid names must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
},
},
"invalid digest based image ref, invalid digest algorithm": {
spec: ImageSource{
Ref: "docker.io/foo/bar@99-problems:abcdef123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": digest algorithm is not valid. valid algorithms must start with an uppercase or lowercase alpha character followed by alphanumeric characters and may contain the \"-\", \"_\", \"+\", and \".\" characters.",
},
},
"invalid digest based image ref, too short digest encoding": {
spec: ImageSource{
Ref: "docker.io/foo/bar@sha256:abcdef123456789",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": digest is not valid. the encoded string must be at least 32 characters",
},
},
"invalid digest based image ref, invalid characters in digest encoding": {
spec: ImageSource{
Ref: "docker.io/foo/bar@sha256:XYZxy123456789abcdef123456789abc",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": digest is not valid. the encoded string must only contain hex characters (A-F, a-f, 0-9)",
},
},
"invalid image ref, no digest": {
spec: ImageSource{
Ref: "docker.io/foo/bar",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": must end with a digest",
},
},
"invalid image ref, only domain with port": {
spec: ImageSource{
Ref: "docker.io:8080",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": a valid name is required. valid names must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": must end with a digest",
},
},
"invalid image ref, tag-based ref": {
spec: ImageSource{
Ref: "docker.io/foo/bar:latest",
},
wantErrs: []string{
"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image.properties.ref: Invalid value: \"string\": must end with a digest",
},
},
joelanford marked this conversation as resolved.
Show resolved Hide resolved
} {
t.Run(name, func(t *testing.T) {
errs := validator(tc.spec.Ref, nil)
require.Equal(t, len(tc.wantErrs), len(errs), "want", tc.wantErrs, "got", errs)
for i := range tc.wantErrs {
got := errs[i].Error()
assert.Equal(t, tc.wantErrs[i], got)
}
})
}
}

func TestClusterCatalogURLsCELValidation(t *testing.T) {
validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
pth := "openAPIV3Schema.properties.status.properties.urls.properties.base"
validator, found := validators["v1alpha1"][pth]
require.True(t, found)
for name, tc := range map[string]struct {
urls ClusterCatalogURLs
wantErrs []string
}{
"base is valid": {
urls: ClusterCatalogURLs{
Base: "https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio",
},
wantErrs: []string{},
},
"base is invalid, scheme is not one of http or https": {
urls: ClusterCatalogURLs{
Base: "file://somefilepath",
},
wantErrs: []string{
fmt.Sprintf("%s: Invalid value: \"string\": scheme must be either http or https", pth),
},
},
"base is invalid": {
urls: ClusterCatalogURLs{
Base: "notevenarealURL",
},
wantErrs: []string{
fmt.Sprintf("%s: Invalid value: \"string\": must be a valid URL", pth),
},
},
} {
t.Run(name, func(t *testing.T) {
errs := validator(tc.urls.Base, nil)
fmt.Println(errs)
require.Equal(t, len(tc.wantErrs), len(errs))
for i := range tc.wantErrs {
got := errs[i].Error()
Expand All @@ -83,13 +307,15 @@ func TestSourceCELValidation(t *testing.T) {
Type: SourceTypeImage,
},
wantErrs: []string{
fmt.Sprintf("%s: Invalid value: \"object\": source type '%s' requires image field", pth, SourceTypeImage),
fmt.Sprintf("%s: Invalid value: \"object\": image is required when source type is %s, and forbidden otherwise", pth, SourceTypeImage),
},
},
"image source with required image field": {
source: CatalogSource{
Type: SourceTypeImage,
Image: &ImageSource{},
Type: SourceTypeImage,
Image: &ImageSource{
Ref: "docker.io/foo/bar:latest",
},
},
wantErrs: []string{},
},
Expand Down Expand Up @@ -123,13 +349,15 @@ func TestResolvedSourceCELValidation(t *testing.T) {
Type: SourceTypeImage,
},
wantErrs: []string{
fmt.Sprintf("%s: Invalid value: \"object\": source type '%s' requires image field", pth, SourceTypeImage),
fmt.Sprintf("%s: Invalid value: \"object\": image is required when source type is %s, and forbidden otherwise", pth, SourceTypeImage),
},
},
"image source with required image field": {
source: ResolvedCatalogSource{
Type: SourceTypeImage,
Image: &ResolvedImageSource{},
Type: SourceTypeImage,
Image: &ResolvedImageSource{
Ref: "docker.io/foo/bar@sha256:abcdef123456789abcdef123456789abc",
},
},
wantErrs: []string{},
},
Expand All @@ -149,6 +377,7 @@ func TestResolvedSourceCELValidation(t *testing.T) {

// fieldValidatorsFromFile extracts the CEL validators by version and JSONPath from a CRD file and returns
// a validator func for testing against samples.
// nolint:unparam
func fieldValidatorsFromFile(t *testing.T, crdFilePath string) map[string]map[string]CELValidateFunc {
data, err := os.ReadFile(crdFilePath)
require.NoError(t, err)
Expand Down
14 changes: 8 additions & 6 deletions api/core/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

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

Loading
Loading