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

crdutil: Add feature to also apply single files #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

crdutil: Add feature to also apply single files #58

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
1 change: 1 addition & 0 deletions .golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ linters-settings:
- github.com/go-logr/logr
- k8s.io
- sigs.k8s.io
- github.com/spf13/pflag
dupl:
threshold: 100
funlen:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require (
github.com/go-logr/logr v1.4.2
github.com/onsi/ginkgo/v2 v2.21.0
github.com/onsi/gomega v1.34.2
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.9.0
k8s.io/api v0.31.2
k8s.io/apiextensions-apiserver v0.31.0
Expand Down Expand Up @@ -70,7 +71,6 @@ require (
github.com/prometheus/procfs v0.15.1 // indirect
github.com/russross/blackfriday/v2 v2.1.0 // indirect
github.com/spf13/cobra v1.8.1 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/stretchr/objx v0.5.2 // indirect
github.com/x448/float16 v0.8.4 // indirect
github.com/xlab/treeprint v1.2.0 // indirect
Expand Down
100 changes: 60 additions & 40 deletions pkg/crdutil/crdutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"path/filepath"
"strings"

"github.com/spf13/pflag"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
v1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
Expand All @@ -37,38 +38,31 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
)

type StringList []string

func (s *StringList) String() string {
return strings.Join(*s, ", ")
}

func (s *StringList) Set(value string) error {
*s = append(*s, value)
return nil
}

var (
crdsDir StringList
filenames []string
recursive bool
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to separate the handling of multiple non-recursive paths (including files) from the addition of recursive handling?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK kubectl is also not separating it, so it is totally fine to keep it pretty open IMO. But also fine to me to split it somehow (you mean like: if multiple files are passed the recursive flag is not possible?)

Copy link
Member

Choose a reason for hiding this comment

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

I actually just meant as separate PRs. From NVIDIA/gpu-operator#1137 it seems that we don't use recursive mode. Is it used in the network operator?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is not used by the network-operator. Maybe it makes sense to remove this new feature from the PR, you are right. The request adding it to this PR came through #58 (comment) (at least that's how I understood it)

)

func initFlags() {
flag.Var(&crdsDir, "crds-dir", "Path to the directory containing the CRD manifests")
Copy link
Member

Choose a reason for hiding this comment

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

Are we concerned about this as a breaking change since the command line arguments are changed?

Copy link
Member Author

@tobiasgiese tobiasgiese Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was also thinking about this but this pkg is pretty new and we are only using it for the network-operator currently. Do you think we should deprecate it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the network-operator is not released with this change, better to just update as it will be first use of this package.

Copy link
Member Author

@tobiasgiese tobiasgiese Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The network-operator 24.10.0 has been released 2 days ago with the change in it, see https://github.com/Mellanox/network-operator/blob/v24.10.0/deployment/network-operator/templates/upgrade-crd-hook.yaml#L84.

But IMO it is totally fine to change it, because it is a Helm pre-install hook. If it fails, it fails the installation/upgrade and the user has to use the latest Helm chart. There is no dependency to the runtime which can cause the operator to fail.

WDYT @e0ne @rollandf

tl;dr: this PR changes the flag --crds-dir to --filename/-f

Copy link
Member

Choose a reason for hiding this comment

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

If there is a concern, we could support both -- marking crds-dir as deprecated -- for a release. Happy with the change since this is effectively an internal API.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also revert it to --crds-dir. Both crds-dir and filename does not contain both files and dirs, so it is not 100% explicit anyway :)
Request came through #58 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

@shivamerla WDYT? Should we remove the flag rename and recursive flag (see #58 (comment)) out of this PR and change it in a follow-up? I'd say this will reduce the size of this PR.

flag.Parse()
pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
pflag.StringSliceVarP(&filenames, "filename", "f", filenames, "The files that contain the configurations to apply.")
pflag.BoolVarP(&recursive, "recursive", "R", false, "Process the directory used in -f, --filename recursively.")
pflag.Parse()

if len(crdsDir) == 0 {
log.Fatalf("CRDs directory is required")
if len(filenames) == 0 {
log.Fatalf("CRDs directory or single CRDs are required")
}

for _, crdDir := range crdsDir {
for _, crdDir := range filenames {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for _, crdDir := range filenames {
for _, path := range filenames {

if _, err := os.Stat(crdDir); os.IsNotExist(err) {
log.Fatalf("CRDs directory %s does not exist", crdsDir)
log.Fatalf("CRDs directory %s does not exist", filenames)
Copy link
Member

Choose a reason for hiding this comment

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

should this be:

Suggested change
log.Fatalf("CRDs directory %s does not exist", filenames)
log.Fatalf("path does not exist: %s", cdrDir)

}
}
}

// EnsureCRDsCmd reads each YAML file in the directory, splits it into documents, and applies each CRD to the cluster.
// The parameter --crds-dir is required and should point to the directory containing the CRD manifests.
// TODO: add unit test for this command.
func EnsureCRDsCmd() {
ctx := context.Background()

Expand All @@ -84,38 +78,59 @@ func EnsureCRDsCmd() {
log.Fatalf("Failed to create API extensions client: %v", err)
}

if err := walkCrdsDir(ctx, client.ApiextensionsV1().CustomResourceDefinitions()); err != nil {
log.Fatalf("Failed to apply CRDs: %v", err)
dirsToApply, err := walkCRDs(recursive, filenames)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dirsToApply, err := walkCRDs(recursive, filenames)
pathsToApply, err := collectYamlPaths(recursive, filenames)

if err != nil {
log.Fatalf("Failed to walk through CRDs: %v", err)
}

for _, dir := range dirsToApply {
log.Printf("Apply CRDs from file: %s", dir)
if err := applyCRDs(ctx, client.ApiextensionsV1().CustomResourceDefinitions(), dir); err != nil {
log.Fatalf("Failed to apply CRDs: %v", err)
}
}
}

// walkCrdsDir walks the CRDs directory and applies each YAML file.
func walkCrdsDir(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface) error {
for _, crdDir := range crdsDir {
// walkCRDs walks the CRDs directory and applies each YAML file.
// TODO: add unit test for this function.
func walkCRDs(recursive bool, crdDirs []string) ([]string, error) {
Comment on lines +94 to +96
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// walkCRDs walks the CRDs directory and applies each YAML file.
// TODO: add unit test for this function.
func walkCRDs(recursive bool, crdDirs []string) ([]string, error) {
// collectYAMLPaths processes a list of paths and returns all YAML files.
// If `recursive` is specified directories in the list are searched recursively for YAML files.
// TODO: add unit test for this function.
func collectYAMLPaths(recursive bool, paths []string) ([]string, error) {

Copy link
Member

Choose a reason for hiding this comment

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

Note that we also need to rename some local variables to better indicate what we are collecting.

var dirs []string
for _, crdDir := range crdDirs {
// We need the parent directory to check if we are in the top-level directory.
// This is necessary for the recursive logic.
// We can skip the errors as it has been checked in initFlags.
parentDir, _ := os.Stat(crdDir)
// Walk the directory recursively and apply each YAML file.
err := filepath.Walk(crdDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() || filepath.Ext(path) != ".yaml" {
// If this is a directory, skip it.
// filepath.Walk() is also called for directories, but we only want to apply CRDs from files.
if info.IsDir() {
tobiasgiese marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

log.Printf("Apply CRDs from file: %s", path)
if err := applyCRDsFromFile(ctx, crdClient, path); err != nil {
return fmt.Errorf("apply CRD %s: %w", path, err)
if filepath.Ext(path) != ".yaml" && filepath.Ext(path) != ".yml" {
return nil
}
// If not recursive we want to only apply the CRDs in the top-level directory.
// filepath.Dir() does not add a trailing slash, thus we need to trim it in the crdDir.
if !recursive && parentDir.IsDir() && filepath.Dir(path) != strings.TrimRight(crdDir, "/") {
Copy link
Member

Choose a reason for hiding this comment

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

should we add a test case for this new branch or should these all be included in the "TODO add unit tests" comment you have added?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me take another look at the tests, I think I can easily test the walkCRDs func

return nil
}

dirs = append(dirs, path)
return nil
})
if err != nil {
return fmt.Errorf("walk the path %s: %w", crdsDir, err)
return []string{}, fmt.Errorf("walk the path %s: %w", crdDirs, err)
}
}
return nil
return dirs, nil
}

// applyCRDsFromFile reads a YAML file, splits it into documents, and applies each CRD to the cluster.
func applyCRDsFromFile(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface, filePath string) error {
// applyCRDs reads a YAML file, splits it into documents, and applies each CRD to the cluster.
func applyCRDs(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface, filePath string) error {
Copy link
Member

Choose a reason for hiding this comment

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

This rename wasn't strictly required, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's not required. It was a review finding #58 (comment)

file, err := os.Open(filePath)
if err != nil {
return fmt.Errorf("open file %q: %w", filePath, err)
Expand Down Expand Up @@ -170,14 +185,19 @@ func applyCRD(
if err != nil {
return fmt.Errorf("create CRD %s: %w", crd.Name, err)
Copy link
Member

Choose a reason for hiding this comment

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

Note: we already add crd.Name at the caller. Could we just return:

Suggested change
return fmt.Errorf("create CRD %s: %w", crd.Name, err)
return fmt.Errorf("create CRD: %w", err)

(and similarly for Update below)?

}
} else {
log.Printf("Update CRD %s", crd.Name)
// Set resource version to update an existing CRD.
crd.SetResourceVersion(curCRD.GetResourceVersion())
_, err = crdClient.Update(ctx, crd, metav1.UpdateOptions{})
if err != nil {
return fmt.Errorf("update CRD %s: %w", crd.Name, err)
}
return nil
}
if err != nil {
return fmt.Errorf("get CRD %s: %w", crd.Name, err)
}

log.Printf("Update CRD %s", crd.Name)
// Set resource version to update an existing CRD.
crd.SetResourceVersion(curCRD.GetResourceVersion())
_, err = crdClient.Update(ctx, crd, metav1.UpdateOptions{})
if err != nil {
return fmt.Errorf("update CRD %s: %w", crd.Name, err)
}

return nil
}
14 changes: 7 additions & 7 deletions pkg/crdutil/crdutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ var _ = Describe("CRD Application", func() {
Expect(testCRDClient.DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{})).NotTo(HaveOccurred())
})

Describe("applyCRDsFromFile", func() {
Describe("applyCRDs", func() {
It("should apply CRDs multiple times from a valid YAML file", func() {
By("applying CRDs")
Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDs(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDs(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDs(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDs(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())

By("verifying CRDs are applied")
crds, err := testCRDClient.List(ctx, metav1.ListOptions{})
Expand All @@ -54,7 +54,7 @@ var _ = Describe("CRD Application", func() {

It("should update CRDs", func() {
By("applying CRDs")
Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
Expect(applyCRDs(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())

By("verifying CRDs do not have spec.foobar")
for _, crdName := range []string{"bars.example.com", "foos.example.com"} {
Expand All @@ -66,7 +66,7 @@ var _ = Describe("CRD Application", func() {
}

By("updating CRDs")
Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/updated-test-crds.yaml")).To(Succeed())
Expect(applyCRDs(ctx, testCRDClient, "test-files/updated-test-crds.yaml")).To(Succeed())

By("verifying CRDs are updated")
for _, crdName := range []string{"bars.example.com", "foos.example.com"} {
Expand Down
Loading