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

Allow --local-crds flag to receive multiple values #101

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions pkg/cmd/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type commandFlags struct {
kubeConfigOverrides clientcmd.ConfigOverrides
version string
localSchemasDir string
localCRDsDir string
localCRDsDir []string
schemaPatchesDir string
outputFormat OutputFormat
}
Expand All @@ -79,7 +79,7 @@ func NewRootCommand() *cobra.Command {
}
res.Flags().StringVarP(&invoked.version, "version", "", invoked.version, "Kubernetes version to validate native resources against. Required if not connected directly to cluster")
res.Flags().StringVarP(&invoked.localSchemasDir, "local-schemas", "", "", "--local-schemas=./path/to/schemas/dir. Path to a directory with format: /apis/<group>/<version>.json for each group-version's schema.")
res.Flags().StringVarP(&invoked.localCRDsDir, "local-crds", "", "", "--local-crds=./path/to/crds/dir. Path to a directory containing .yaml or .yml files for CRD definitions.")
res.Flags().StringSliceVarP(&invoked.localCRDsDir, "local-crds", "", []string{}, "--local-crds=./path/to/crds/dir. Paths to directories containing .yaml or .yml files for CRD definitions.")
res.Flags().StringVarP(&invoked.schemaPatchesDir, "schema-patches", "", "", "Path to a directory with format: /apis/<group>/<version>.json for each group-version's schema you wish to jsonpatch to the groupversion's final schema. Patches only apply if the schema exists")
res.Flags().VarP(&invoked.outputFormat, "output", "o", "Output format. Choice of: \"human\" or \"json\"")
clientcmd.BindOverrideFlags(&invoked.kubeConfigOverrides, res.Flags(), clientcmd.RecommendedConfigOverrideFlags("kube-"))
Expand Down
67 changes: 36 additions & 31 deletions pkg/openapiclient/local_crds.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,55 +33,60 @@ var metadataSchemas map[string]*spec.Schema = func() map[string]*spec.Schema {

// client which provides openapi read from files on disk
type localCRDsClient struct {
fs fs.FS
dir string
fs fs.FS
dirs []string
}

// Dir should have openapi files following directory layout:
// myCRD.yaml (groupversions read from file)
func NewLocalCRDFiles(fs fs.FS, dirPath string) openapi.Client {
func NewLocalCRDFiles(fs fs.FS, dirPaths []string) openapi.Client {
return &localCRDsClient{
fs: fs,
dir: dirPath,
fs: fs,
dirs: dirPaths,
}
}

func (k *localCRDsClient) Paths() (map[string]openapi.GroupVersion, error) {
if len(k.dir) == 0 {
if len(k.dirs) == 0 {
return nil, nil
}
files, err := utils.ReadDir(k.fs, k.dir)
if err != nil {
return nil, fmt.Errorf("error listing %s: %w", k.dir, err)
}
var documents []utils.Document
for _, f := range files {
path := filepath.Join(k.dir, f.Name())
if f.IsDir() {
continue

for _, dir := range k.dirs {
files, err := utils.ReadDir(k.fs, dir)
if err != nil {
return nil, fmt.Errorf("error listing %s: %w", dir, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to stop if any of the directory fails? I think we do
But do we want to report all errors if multiple directories fail? I suspect so too, thoughts?

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 agree for a better user experience we likely should accumulate and report all errors at once.
However it seems the current strategy for this function is to fail as soon as any error is encountered (e.g. first file that cannot be read). Perhaps we could revisit it outside of this PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

}
if utils.IsJson(f.Name()) {
fileBytes, err := utils.ReadFile(k.fs, path)
if err != nil {
return nil, fmt.Errorf("failed to read %s: %w", path, err)
}
documents = append(documents, fileBytes)
} else if utils.IsYaml(f.Name()) {
fileBytes, err := utils.ReadFile(k.fs, path)
if err != nil {
return nil, fmt.Errorf("failed to read %s: %w", path, err)
}
yamlDocs, err := utils.SplitYamlDocuments(fileBytes)
if err != nil {
return nil, fmt.Errorf("failed to read %s: %w", path, err)

for _, f := range files {
path := filepath.Join(dir, f.Name())
if f.IsDir() {
doom marked this conversation as resolved.
Show resolved Hide resolved
continue
}
for _, document := range yamlDocs {
if !utils.IsEmptyYamlDocument(document) {
documents = append(documents, document)
if utils.IsJson(f.Name()) {
fileBytes, err := utils.ReadFile(k.fs, path)
if err != nil {
return nil, fmt.Errorf("failed to read %s: %w", path, err)
}
documents = append(documents, fileBytes)
} else if utils.IsYaml(f.Name()) {
fileBytes, err := utils.ReadFile(k.fs, path)
if err != nil {
return nil, fmt.Errorf("failed to read %s: %w", path, err)
}
yamlDocs, err := utils.SplitYamlDocuments(fileBytes)
if err != nil {
return nil, fmt.Errorf("failed to read %s: %w", path, err)
}
for _, document := range yamlDocs {
if !utils.IsEmptyYamlDocument(document) {
documents = append(documents, document)
}
}
}
}
}

codecs := serializer.NewCodecFactory(apiserver.Scheme).UniversalDecoder()
crds := map[schema.GroupVersion]*spec3.OpenAPI{}
for _, document := range documents {
Expand Down
68 changes: 49 additions & 19 deletions pkg/openapiclient/local_crds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,24 @@ import (

func TestNewLocalCRDFiles(t *testing.T) {
tests := []struct {
name string
fs fs.FS
dirPath string
want openapi.Client
name string
fs fs.FS
dirPaths []string
want openapi.Client
}{{
name: "fs nil and dir empty",
want: &localCRDsClient{},
}, {
name: "only dir",
dirPath: "test",
name: "only dir",
dirPaths: []string{"test"},
want: &localCRDsClient{
dir: "test",
dirs: []string{"test"},
},
}, {
name: "multiple dirs",
dirPaths: []string{"test", "test2"},
want: &localCRDsClient{
dirs: []string{"test", "test2"},
},
}, {
name: "only fs",
Expand All @@ -34,17 +40,17 @@ func TestNewLocalCRDFiles(t *testing.T) {
fs: os.DirFS("."),
},
}, {
name: "both fs and dir",
fs: os.DirFS("."),
dirPath: "test",
name: "both fs and dir",
fs: os.DirFS("."),
dirPaths: []string{"test"},
want: &localCRDsClient{
fs: os.DirFS("."),
dir: "test",
fs: os.DirFS("."),
dirs: []string{"test"},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := NewLocalCRDFiles(tt.fs, tt.dirPath)
got := NewLocalCRDFiles(tt.fs, tt.dirPaths)
require.Equal(t, tt.want, got, "NewLocalCRDFiles not equal")
})
}
Expand All @@ -54,14 +60,14 @@ func Test_localCRDsClient_Paths(t *testing.T) {
tests := []struct {
name string
fs fs.FS
dir string
dirs []string
want map[string]sets.Set[string]
wantErr bool
}{{
name: "fs nil and dir empty",
}, {
name: "only dir",
dir: "../../testcases/crds",
dirs: []string{"../../testcases/crds"},
want: map[string]sets.Set[string]{
"apis/batch.x-k8s.io/v1alpha1": sets.New(
"batch.x-k8s.io/v1alpha1.JobSet",
Expand All @@ -80,13 +86,37 @@ func Test_localCRDsClient_Paths(t *testing.T) {
"cert-manager.io/v1.Issuer",
),
},
}, {
name: "multiple dirs",
dirs: []string{"../../testcases/crds", "../../testcases/more-crds"},
want: map[string]sets.Set[string]{
"apis/batch.x-k8s.io/v1alpha1": sets.New(
"batch.x-k8s.io/v1alpha1.JobSet",
),
"apis/stable.example.com/v1": sets.New(
"stable.example.com/v1.CELBasic",
),
"apis/acme.cert-manager.io/v1": sets.New(
"acme.cert-manager.io/v1.Challenge",
"acme.cert-manager.io/v1.Order",
),
"apis/cert-manager.io/v1": sets.New(
"cert-manager.io/v1.Certificate",
"cert-manager.io/v1.CertificateRequest",
"cert-manager.io/v1.ClusterIssuer",
"cert-manager.io/v1.Issuer",
),
"apis/externaldns.k8s.io/v1alpha1": sets.New(
"externaldns.k8s.io/v1alpha1.DNSEndpoint",
),
},
}, {
name: "only fs",
fs: os.DirFS("../../testcases/crds"),
}, {
name: "both fs and dir",
fs: os.DirFS("../../testcases"),
dir: "crds",
dirs: []string{"crds"},
want: map[string]sets.Set[string]{
"apis/batch.x-k8s.io/v1alpha1": sets.New(
"batch.x-k8s.io/v1alpha1.JobSet",
Expand All @@ -107,17 +137,17 @@ func Test_localCRDsClient_Paths(t *testing.T) {
},
}, {
name: "invalid dir",
dir: "invalid",
dirs: []string{"invalid"},
wantErr: true,
}, {
name: "invalid fs",
fs: os.DirFS("../../invalid"),
dir: ".",
dirs: []string{"."},
wantErr: true,
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
k := NewLocalCRDFiles(tt.fs, tt.dir)
k := NewLocalCRDFiles(tt.fs, tt.dirs)
paths, err := k.Paths()
if (err != nil) != tt.wantErr {
t.Errorf("localCRDsClient.Paths() error = %v, wantErr %v", err, tt.wantErr)
Expand Down
2 changes: 1 addition & 1 deletion pkg/validator/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestValidatorFactory_TestPatcher(t *testing.T) {
{
name: "cronjob_crd",
file: "./testdata/cronjob.yaml",
client: openapiclient.NewLocalCRDFiles(nil, "./testdata/crds/"),
client: openapiclient.NewLocalCRDFiles(nil, []string{"./testdata/crds/"}),
want: &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "batch.tutorial.kubebuilder.io/v1",
Expand Down
93 changes: 93 additions & 0 deletions testcases/more-crds/dnsendpoints.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.5.0
api-approved.kubernetes.io: "https://github.com/kubernetes-sigs/external-dns/pull/2007"
creationTimestamp: null
name: dnsendpoints.externaldns.k8s.io
spec:
group: externaldns.k8s.io
names:
kind: DNSEndpoint
listKind: DNSEndpointList
plural: dnsendpoints
singular: dnsendpoint
scope: Namespaced
versions:
- name: v1alpha1
schema:
openAPIV3Schema:
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
description: DNSEndpointSpec defines the desired state of DNSEndpoint
properties:
endpoints:
items:
description: Endpoint is a high-level way of a connection between a service and an IP
properties:
dnsName:
description: The hostname of the DNS record
type: string
labels:
additionalProperties:
type: string
description: Labels stores labels defined for the Endpoint
type: object
providerSpecific:
description: ProviderSpecific stores provider specific config
items:
description: ProviderSpecificProperty holds the name and value of a configuration which is specific to individual DNS providers
properties:
name:
type: string
value:
type: string
type: object
type: array
recordTTL:
description: TTL for the record
format: int64
type: integer
recordType:
description: RecordType type of record, e.g. CNAME, A, SRV, TXT etc
type: string
setIdentifier:
description: Identifier to distinguish multiple records with the same name and type (e.g. Route53 records with routing policies other than 'simple')
type: string
targets:
description: The targets the DNS record points to
items:
type: string
type: array
type: object
type: array
type: object
status:
description: DNSEndpointStatus defines the observed state of DNSEndpoint
properties:
observedGeneration:
description: The generation observed by the external-dns controller.
format: int64
type: integer
type: object
type: object
served: true
storage: true
subresources:
status: {}
status:
acceptedNames:
kind: ""
plural: ""
conditions: []
storedVersions: []
Loading