From 38f904d6b34530612b36d2dbdbfc1e4f201f8b77 Mon Sep 17 00:00:00 2001 From: Raj <54686422+LeelaChacha@users.noreply.github.com> Date: Thu, 24 Oct 2024 16:10:01 +0200 Subject: [PATCH] feat: Add support for spec.associatedResources (#75) * feat: Add support for spec.associatedResources * fix: unit test * refactor: lint issue * test: fix e2e * refactor: lint * refactor: Fix typo Co-authored-by: Nesma Badr * refactor: Enhance comment in ModuleConfig struct * refactor: Lint * test: Add unit test for AssociatedResources validation * chore: Update unit test coverage requirement * refactor: Indentation * refactor: Reformat Unit Test --------- Co-authored-by: Nesma Badr Co-authored-by: Jeremy Harisch <48282931+jeremyharisch@users.noreply.github.com> Co-authored-by: Nesma Badr --- internal/common/validation/validation.go | 16 +++++ internal/common/validation/validation_test.go | 41 ++++++++++++ .../service/contentprovider/moduleconfig.go | 31 +++++----- .../generator/moduleconfig_generator_test.go | 7 +++ .../reader/moduleconfig_reader.go | 29 +++++---- .../reader/moduleconfig_reader_test.go | 62 +++++++++++++++++++ .../templategenerator/templategenerator.go | 45 +++++++++----- .../templategenerator_test.go | 37 +++++++++++ tests/e2e/create/create_suite_test.go | 1 + tests/e2e/create/create_test.go | 40 ++++++++++++ .../valid/with-associated-resources.yaml | 8 +++ unit-test-coverage.yaml | 2 +- 12 files changed, 273 insertions(+), 46 deletions(-) create mode 100644 tests/e2e/create/testdata/moduleconfig/valid/with-associated-resources.yaml diff --git a/internal/common/validation/validation.go b/internal/common/validation/validation.go index 6fffb6f2..99d37b68 100644 --- a/internal/common/validation/validation.go +++ b/internal/common/validation/validation.go @@ -153,3 +153,19 @@ func validateSemanticVersion(version string) error { return nil } + +func ValidateGvk(group, version, kind string) error { + if kind == "" { + return fmt.Errorf("kind must not be empty: %w", commonerrors.ErrInvalidOption) + } + + if group == "" { + return fmt.Errorf("group must not be empty: %w", commonerrors.ErrInvalidOption) + } + + if version == "" { + return fmt.Errorf("version must not be empty: %w", commonerrors.ErrInvalidOption) + } + + return nil +} diff --git a/internal/common/validation/validation_test.go b/internal/common/validation/validation_test.go index fd745797..32d6df45 100644 --- a/internal/common/validation/validation_test.go +++ b/internal/common/validation/validation_test.go @@ -228,6 +228,47 @@ func TestValidateNamespace(t *testing.T) { } } +func TestValidateGvk(t *testing.T) { + type args struct { + group string + version string + kind string + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "valid GVK", + args: args{group: "kyma-project.io", version: "v1alpha1", kind: "Module"}, + wantErr: false, + }, + { + name: "invalid GVK when group empty", + args: args{version: "v1alpha1", kind: "Module"}, + wantErr: true, + }, + { + name: "invalid GVK when version empty", + args: args{group: "kyma-project.io", kind: "Module"}, + wantErr: true, + }, + { + name: "invalid GVK when kind empty", + args: args{group: "kyma-project.io", version: "v1alpha1"}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validation.ValidateGvk(tt.args.group, tt.args.version, tt.args.kind); (err != nil) != tt.wantErr { + t.Errorf("ValidateGvk() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + func TestValidateResources(t *testing.T) { tests := []struct { name string diff --git a/internal/service/contentprovider/moduleconfig.go b/internal/service/contentprovider/moduleconfig.go index b7a752eb..31137255 100644 --- a/internal/service/contentprovider/moduleconfig.go +++ b/internal/service/contentprovider/moduleconfig.go @@ -80,21 +80,22 @@ type Manager struct { } type ModuleConfig struct { - Name string `yaml:"name" comment:"required, the name of the Module"` - Version string `yaml:"version" comment:"required, the version of the Module"` - Channel string `yaml:"channel" comment:"required, channel that should be used in the ModuleTemplate"` - Manifest string `yaml:"manifest" comment:"required, relative path or remote URL to the manifests"` - Mandatory bool `yaml:"mandatory" comment:"optional, default=false, indicates whether the module is mandatory to be installed on all clusters"` - DefaultCR string `yaml:"defaultCR" comment:"optional, relative path or remote URL to a YAML file containing the default CR for the module"` - ResourceName string `yaml:"resourceName" comment:"optional, default={name}-{channel}, when channel is 'none', the default is {name}-{version}, the name for the ModuleTemplate that will be created"` - Namespace string `yaml:"namespace" comment:"optional, default=kcp-system, the namespace where the ModuleTemplate will be deployed"` - Security string `yaml:"security" comment:"optional, name of the security scanners config file"` - Internal bool `yaml:"internal" comment:"optional, default=false, determines whether the ModuleTemplate should have the internal flag or not"` - Beta bool `yaml:"beta" comment:"optional, default=false, determines whether the ModuleTemplate should have the beta flag or not"` - Labels map[string]string `yaml:"labels" comment:"optional, additional labels for the ModuleTemplate"` - Annotations map[string]string `yaml:"annotations" comment:"optional, additional annotations for the ModuleTemplate"` - Manager *Manager `yaml:"manager" comment:"optional, the module resource that can be used to indicate the installation readiness of the module. This is typically the manager deployment of the module"` - Resources ResourcesMap `yaml:"resources,omitempty" comment:"optional, additional resources of the ModuleTemplate that may be fetched"` + Name string `yaml:"name" comment:"required, the name of the Module"` + Version string `yaml:"version" comment:"required, the version of the Module"` + Channel string `yaml:"channel" comment:"required, channel that should be used in the ModuleTemplate"` + Manifest string `yaml:"manifest" comment:"required, relative path or remote URL to the manifests"` + Mandatory bool `yaml:"mandatory" comment:"optional, default=false, indicates whether the module is mandatory to be installed on all clusters"` + DefaultCR string `yaml:"defaultCR" comment:"optional, relative path or remote URL to a YAML file containing the default CR for the module"` + ResourceName string `yaml:"resourceName" comment:"optional, default={name}-{channel}, when channel is 'none', the default is {name}-{version}, the name for the ModuleTemplate that will be created"` + Namespace string `yaml:"namespace" comment:"optional, default=kcp-system, the namespace where the ModuleTemplate will be deployed"` + Security string `yaml:"security" comment:"optional, name of the security scanners config file"` + Internal bool `yaml:"internal" comment:"optional, default=false, determines whether the ModuleTemplate should have the internal flag or not"` + Beta bool `yaml:"beta" comment:"optional, default=false, determines whether the ModuleTemplate should have the beta flag or not"` + Labels map[string]string `yaml:"labels" comment:"optional, additional labels for the ModuleTemplate"` + Annotations map[string]string `yaml:"annotations" comment:"optional, additional annotations for the ModuleTemplate"` + AssociatedResources []*metav1.GroupVersionKind `yaml:"associatedResources" comment:"optional, GVK of the resources which are associated with the module and have to be deleted with module deletion"` + Manager *Manager `yaml:"manager" comment:"optional, the module resource that can be used to indicate the installation readiness of the module. This is typically the manager deployment of the module"` + Resources ResourcesMap `yaml:"resources,omitempty" comment:"optional, additional resources of the ModuleTemplate that may be fetched"` } type resource struct { diff --git a/internal/service/moduleconfig/generator/moduleconfig_generator_test.go b/internal/service/moduleconfig/generator/moduleconfig_generator_test.go index 1ee8ed2c..bbfc056a 100644 --- a/internal/service/moduleconfig/generator/moduleconfig_generator_test.go +++ b/internal/service/moduleconfig/generator/moduleconfig_generator_test.go @@ -131,6 +131,13 @@ func (*fileExistsStub) ReadFile(_ string) ([]byte, error) { Beta: false, Labels: map[string]string{"label1": "value1"}, Annotations: map[string]string{"annotation1": "value1"}, + AssociatedResources: []*metav1.GroupVersionKind{ + { + Group: "networking.istio.io", + Version: "v1alpha3", + Kind: "Gateway", + }, + }, Manager: &contentprovider.Manager{ Name: "manager-name", Namespace: "manager-namespace", diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader.go b/internal/service/moduleconfig/reader/moduleconfig_reader.go index 9029b19b..4b58955c 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader.go @@ -1,18 +1,16 @@ package moduleconfigreader import ( - "errors" "fmt" "gopkg.in/yaml.v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" commonerrors "github.com/kyma-project/modulectl/internal/common/errors" "github.com/kyma-project/modulectl/internal/common/validation" "github.com/kyma-project/modulectl/internal/service/contentprovider" ) -var ErrNoPathForDefaultCR = errors.New("no path for default CR given") - type FileSystem interface { ReadFile(path string) ([]byte, error) } @@ -76,6 +74,10 @@ func ValidateModuleConfig(moduleConfig *contentprovider.ModuleConfig) error { } } + if err := ValidateAssociatedResources(moduleConfig.AssociatedResources); err != nil { + return fmt.Errorf("failed to validate associated resources: %w", err) + } + if err := ValidateManager(moduleConfig.Manager); err != nil { return fmt.Errorf("failed to validate manager: %w", err) } @@ -83,6 +85,15 @@ func ValidateModuleConfig(moduleConfig *contentprovider.ModuleConfig) error { return nil } +func ValidateAssociatedResources(resources []*metav1.GroupVersionKind) error { + for _, resource := range resources { + if err := validation.ValidateGvk(resource.Group, resource.Version, resource.Kind); err != nil { + return fmt.Errorf("GVK is invalid: %w", err) + } + } + return nil +} + func ValidateManager(manager *contentprovider.Manager) error { if manager == nil { return nil @@ -92,16 +103,8 @@ func ValidateManager(manager *contentprovider.Manager) error { return fmt.Errorf("name must not be empty: %w", commonerrors.ErrInvalidOption) } - if manager.Kind == "" { - return fmt.Errorf("kind must not be empty: %w", commonerrors.ErrInvalidOption) - } - - if manager.Group == "" { - return fmt.Errorf("group must not be empty: %w", commonerrors.ErrInvalidOption) - } - - if manager.Version == "" { - return fmt.Errorf("version must not be empty: %w", commonerrors.ErrInvalidOption) + if err := validation.ValidateGvk(manager.Group, manager.Version, manager.Kind); err != nil { + return fmt.Errorf("GVK is invalid: %w", err) } if manager.Namespace != "" { diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go index d0bf7986..8e60a6f4 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go @@ -42,6 +42,9 @@ func Test_ParseModuleConfig_Returns_CorrectModuleConfig(t *testing.T) { require.False(t, result.Beta) require.Equal(t, map[string]string{"label1": "value1"}, result.Labels) require.Equal(t, map[string]string{"annotation1": "value1"}, result.Annotations) + require.Equal(t, "networking.istio.io", result.AssociatedResources[0].Group) + require.Equal(t, "v1alpha3", result.AssociatedResources[0].Version) + require.Equal(t, "Gateway", result.AssociatedResources[0].Kind) require.Equal(t, contentprovider.ResourcesMap{ "rawManifest": "https://github.com/kyma-project/template-operator/releases/download/1.0.1/template-operator.yaml", }, result.Resources) @@ -299,6 +302,58 @@ func Test_ValidateManager(t *testing.T) { } } +func Test_ValidateAssociatedResources(t *testing.T) { + tests := []struct { + name string + resources []*metav1.GroupVersionKind + wantErr bool + }{ + { + name: "pass on empty resources", + resources: []*metav1.GroupVersionKind{}, + wantErr: false, + }, + { + name: "pass when all resources are valid", + resources: []*metav1.GroupVersionKind{ + { + Group: "networking.istio.io", + Version: "v1alpha3", + Kind: "Gateway", + }, + { + Group: "apps", + Version: "v1", + Kind: "Deployment", + }, + }, + wantErr: false, + }, + { + name: "fail when even one resources is invalid", + resources: []*metav1.GroupVersionKind{ + { + Group: "networking.istio.io", + Version: "v1alpha3", + Kind: "Gateway", + }, + { + Group: "apps", + Kind: "Deployment", + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := moduleconfigreader.ValidateAssociatedResources(tt.resources); (err != nil) != tt.wantErr { + t.Errorf("ValidateAssociatedResources() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + // Test Stubs type fileExistsStub struct{} @@ -321,6 +376,13 @@ var expectedReturnedModuleConfig = contentprovider.ModuleConfig{ Beta: false, Labels: map[string]string{"label1": "value1"}, Annotations: map[string]string{"annotation1": "value1"}, + AssociatedResources: []*metav1.GroupVersionKind{ + { + Group: "networking.istio.io", + Version: "v1alpha3", + Kind: "Gateway", + }, + }, Resources: contentprovider.ResourcesMap{ "rawManifest": "https://github.com/kyma-project/template-operator/releases/download/1.0.1/template-operator.yaml", }, diff --git a/internal/service/templategenerator/templategenerator.go b/internal/service/templategenerator/templategenerator.go index 0d5a0bea..08e06f1b 100644 --- a/internal/service/templategenerator/templategenerator.go +++ b/internal/service/templategenerator/templategenerator.go @@ -8,6 +8,7 @@ import ( "text/template" "github.com/kyma-project/lifecycle-manager/api/shared" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "ocm.software/ocm/api/oci" "ocm.software/ocm/api/ocm/compdesc" "sigs.k8s.io/yaml" @@ -60,6 +61,14 @@ metadata: spec: channel: {{.Channel}} mandatory: {{.Mandatory}} +{{- with .AssociatedResources}} + associatedResources: + {{- range .}} + - group: {{.Group}} + version: {{.Version}} + kind: {{.Kind}} + {{- end}} +{{- end}} {{- with .Data}} data: {{. | indent 4}} @@ -87,16 +96,17 @@ spec: ) type moduleTemplateData struct { - ResourceName string - Namespace string - Descriptor compdesc.ComponentDescriptorVersion - Channel string - Labels map[string]string - Annotations map[string]string - Mandatory bool - Data string - Resources contentprovider.ResourcesMap - Manager *contentprovider.Manager + ResourceName string + Namespace string + Descriptor compdesc.ComponentDescriptorVersion + Channel string + Labels map[string]string + Annotations map[string]string + Mandatory bool + Data string + AssociatedResources []*metav1.GroupVersionKind + Resources contentprovider.ResourcesMap + Manager *contentprovider.Manager } func (s *Service) GenerateModuleTemplate( @@ -140,13 +150,14 @@ func (s *Service) GenerateModuleTemplate( } mtData := moduleTemplateData{ - ResourceName: moduleConfig.ResourceName, - Namespace: moduleConfig.Namespace, - Descriptor: cva, - Channel: moduleConfig.Channel, - Labels: labels, - Annotations: annotations, - Mandatory: moduleConfig.Mandatory, + ResourceName: moduleConfig.ResourceName, + Namespace: moduleConfig.Namespace, + Descriptor: cva, + Channel: moduleConfig.Channel, + Labels: labels, + Annotations: annotations, + Mandatory: moduleConfig.Mandatory, + AssociatedResources: moduleConfig.AssociatedResources, Resources: contentprovider.ResourcesMap{ "rawManifest": moduleConfig.Manifest, // defaults rawManifest to Manifest; may be overwritten by explicitly provided entries }, diff --git a/internal/service/templategenerator/templategenerator_test.go b/internal/service/templategenerator/templategenerator_test.go index a57eb9cb..04c6afa7 100644 --- a/internal/service/templategenerator/templategenerator_test.go +++ b/internal/service/templategenerator/templategenerator_test.go @@ -90,6 +90,43 @@ func TestGenerateModuleTemplate_Success_With_Overwritten_RawManifest(t *testing. require.NotContains(t, mockFS.writtenTemplate, "https://github.com/kyma-project/template-operator/releases/download/1.0.1/template-operator.yaml") } +func TestGenerateModuleTemplateWithAssociatedResources_Success(t *testing.T) { + mockFS := &mockFileSystem{} + svc, _ := templategenerator.NewService(mockFS) + + moduleConfig := &contentprovider.ModuleConfig{ + ResourceName: "test-resource", + Namespace: "default", + Channel: "stable", + Labels: map[string]string{"key": "value"}, + Annotations: map[string]string{"annotation": "value"}, + Mandatory: true, + AssociatedResources: []*metav1.GroupVersionKind{ + { + Group: "networking.istio.io", + Version: "v1alpha3", + Kind: "Gateway", + }, + }, + } + descriptor := testutils.CreateComponentDescriptor("example.com/component", "1.0.0") + data := []byte("test-data") + + err := svc.GenerateModuleTemplate(moduleConfig, descriptor, data, true, "output.yaml") + + require.NoError(t, err) + require.Equal(t, "output.yaml", mockFS.path) + require.Contains(t, mockFS.writtenTemplate, "test-resource") + require.Contains(t, mockFS.writtenTemplate, "default") + require.Contains(t, mockFS.writtenTemplate, "stable") + require.Contains(t, mockFS.writtenTemplate, "test-data") + require.Contains(t, mockFS.writtenTemplate, "example.com/component") + require.Contains(t, mockFS.writtenTemplate, "associatedResources") + require.Contains(t, mockFS.writtenTemplate, "networking.istio.io") + require.Contains(t, mockFS.writtenTemplate, "v1alpha3") + require.Contains(t, mockFS.writtenTemplate, "Gateway") +} + func TestGenerateModuleTemplateWithManager_Success(t *testing.T) { mockFS := &mockFileSystem{} svc, _ := templategenerator.NewService(mockFS) diff --git a/tests/e2e/create/create_suite_test.go b/tests/e2e/create/create_suite_test.go index f8afc3a4..7c5226e8 100644 --- a/tests/e2e/create/create_suite_test.go +++ b/tests/e2e/create/create_suite_test.go @@ -39,6 +39,7 @@ const ( withDefaultCrConfig = validConfigs + "with-defaultcr.yaml" withSecurityConfig = validConfigs + "with-security.yaml" withMandatoryConfig = validConfigs + "with-mandatory.yaml" + withAssociatedResourcesConfig = validConfigs + "with-associated-resources.yaml" withResources = validConfigs + "with-resources.yaml" withResourcesOverwrite = validConfigs + "with-resources-overwrite.yaml" withManagerConfig = validConfigs + "with-manager.yaml" diff --git a/tests/e2e/create/create_test.go b/tests/e2e/create/create_test.go index 9cea55d9..74e888dc 100644 --- a/tests/e2e/create/create_test.go +++ b/tests/e2e/create/create_test.go @@ -253,6 +253,9 @@ var _ = Describe("Test 'create' command", Ordered, func() { By("And spec.mandatory should be false") Expect(template.Spec.Mandatory).To(BeFalse()) + By("And spec.associatedResources should be empty") + Expect(template.Spec.AssociatedResources).To(BeEmpty()) + By("And spec.manager should be nil") Expect(template.Spec.Manager).To(BeNil()) @@ -539,6 +542,43 @@ var _ = Describe("Test 'create' command", Ordered, func() { Expect(manager.Kind).To(Equal("Deployment")) }) }) + + + Context("Given 'modulectl create' command", func() { + var cmd createCmd + It("When invoked with valid module-config containing associatedResources list", func() { + cmd = createCmd{ + moduleConfigFile: withAssociatedResourcesConfig, + registry: ociRegistry, + insecure: true, + output: templateOutputPath, + } + }) + It("Then the command should succeed", func() { + Expect(cmd.execute()).To(Succeed()) + + By("And module template file should be generated") + Expect(filesIn("/tmp/")).Should(ContainElement("template.yaml")) + }) + It("Then module template should contain the expected content", func() { + template, err := readModuleTemplate(templateOutputPath) + Expect(err).ToNot(HaveOccurred()) + descriptor := getDescriptor(template) + Expect(descriptor).ToNot(BeNil()) + + By("And annotation should have correct version") + annotations := template.Annotations + Expect(annotations[shared.ModuleVersionAnnotation]).To(Equal("1.0.7")) + + By("And spec.associatedResources should be correct") + resources := template.Spec.AssociatedResources + Expect(resources).ToNot(BeEmpty()) + Expect(len(resources)).To(Equal(1)) + Expect(resources[0].Group).To(Equal("networking.istio.io")) + Expect(resources[0].Version).To(Equal("v1alpha3")) + Expect(resources[0].Kind).To(Equal("Gateway")) + }) + }) Context("Given 'modulectl create' command", func() { var cmd createCmd diff --git a/tests/e2e/create/testdata/moduleconfig/valid/with-associated-resources.yaml b/tests/e2e/create/testdata/moduleconfig/valid/with-associated-resources.yaml new file mode 100644 index 00000000..29341c06 --- /dev/null +++ b/tests/e2e/create/testdata/moduleconfig/valid/with-associated-resources.yaml @@ -0,0 +1,8 @@ +name: kyma-project.io/module/template-operator +channel: regular +version: 1.0.7 +manifest: https://github.com/kyma-project/template-operator/releases/download/1.0.1/template-operator.yaml +associatedResources: + - group: networking.istio.io + version: v1alpha3 + kind: Gateway \ No newline at end of file diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 933014e3..83223ef4 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -8,7 +8,7 @@ packages: internal/service/filegenerator/reusefilegenerator: 94 internal/service/fileresolver: 100 internal/service/moduleconfig/generator: 100 - internal/service/moduleconfig/reader: 78 + internal/service/moduleconfig/reader: 76 internal/service/create: 43 internal/service/componentdescriptor: 78 internal/service/templategenerator: 85