diff --git a/internal/controller/styra/system_controller.go b/internal/controller/styra/system_controller.go index 5f074c71..2da6942e 100644 --- a/internal/controller/styra/system_controller.go +++ b/internal/controller/styra/system_controller.go @@ -21,8 +21,11 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" "path" "reflect" + "regexp" + "strings" "time" "github.com/go-logr/logr" @@ -359,7 +362,11 @@ func (r *SystemReconciler) reconcile( } system.SetCondition(v1beta1.ConditionTypeGitCredentialsUpdated, metav1.ConditionTrue) - if r.systemNeedsUpdate(log, system, cfg) { + needsUpdate, err := r.systemNeedsUpdate(log, system, cfg) + if err != nil { + return ctrl.Result{}, err + } + if needsUpdate { updateSystemStart := time.Now() cfg, err = r.updateSystem(ctx, log, system) r.Metrics.ReconcileSegmentTime.WithLabelValues("updateSystem"). @@ -496,7 +503,12 @@ func (r *SystemReconciler) createSystem( ) (*styra.CreateSystemResponse, error) { log.Info("Creating system in Styra") - cfg := r.specToSystemConfig(system) + cfg, err := r.specToSystemConfig(system) + if err != nil { + return nil, ctrlerr.Wrap(err, "Error while reading system spec"). + WithEvent("ErrorCreateSystemInStyra"). + WithSystemCondition(v1beta1.ConditionTypeCreatedInStyra) + } // We dont set the sourcecontrol settings on system creation, as we havent // created the git secret yet. @@ -1181,7 +1193,12 @@ func (r *SystemReconciler) updateSystem( system *v1beta1.System, ) (*styra.SystemConfig, error) { log.Info("Updating system") - cfg := r.specToSystemConfig(system) + cfg, err := r.specToSystemConfig(system) + if err != nil { + return nil, ctrlerr.Wrap(err, "Error while reading system spec"). + WithEvent("ErrorUpdateSystem"). + WithSystemCondition(v1beta1.ConditionTypeSystemConfigUpdated) + } if log.V(1).Enabled() { log := log.V(1) @@ -1208,7 +1225,7 @@ func (r *SystemReconciler) updateSystem( return res.SystemConfig, nil } -func (r *SystemReconciler) specToSystemConfig(system *v1beta1.System) *styra.SystemConfig { +func (r *SystemReconciler) specToSystemConfig(system *v1beta1.System) (*styra.SystemConfig, error) { cfg := &styra.SystemConfig{ Name: system.DisplayName(r.Config.SystemPrefix, r.Config.SystemSuffix), Type: "custom", @@ -1262,6 +1279,10 @@ func (r *SystemReconciler) specToSystemConfig(system *v1beta1.System) *styra.Sys } if system.Spec.SourceControl != nil { + if !isURLValid(system.Spec.SourceControl.Origin.URL) { + return nil, ctrlerr.New("Invalid URL for source control") + } + cfg.SourceControl = &styra.SourceControlConfig{ Origin: styra.GitRepoConfig{ Credentials: system.GitSecretID(), @@ -1284,44 +1305,72 @@ func (r *SystemReconciler) specToSystemConfig(system *v1beta1.System) *styra.Sys cfg.DeploymentParameters.Discovery = system.Spec.DiscoveryOverrides } - return cfg + return cfg, nil +} + +func isURLValid(u string) bool { + //empty url is valid + if strings.TrimSpace(u) == "" { + return true + } + + parsedURL, err := url.Parse(u) + if err != nil { + return false + } + if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { + return false + } + + // Regular expression to match valid URL path characters + re := regexp.MustCompile(`^[a-zA-Z0-9\-._~!$&'()*+,;=:@/]*$`) + return re.MatchString(parsedURL.Path) } -func (r *SystemReconciler) systemNeedsUpdate(log logr.Logger, system *v1beta1.System, cfg *styra.SystemConfig) bool { +func (r *SystemReconciler) systemNeedsUpdate( + log logr.Logger, + system *v1beta1.System, + cfg *styra.SystemConfig, +) (bool, error) { if cfg == nil { log.Info("System needs update: cfg is nil") - return true + return true, nil } if cfg.ReadOnly != r.Config.ReadOnly { log.Info("System needs update: read only is not equal") - return true + return true, nil } - expectedModel := r.specToSystemConfig(system) + expectedModel, err := r.specToSystemConfig(system) + if err != nil { + return true, ctrlerr.Wrap(err, "Error while reading system spec"). + WithEvent("ErrorUpdateSystem"). + WithSystemCondition(v1beta1.ConditionTypeSystemConfigUpdated) + } if cfg.BundleDownload == nil || cfg.BundleDownload.DeltaBundles != expectedModel.BundleDownload.DeltaBundles { log.Info("System needs update: Deltabundle setting not equal") - return true + return true, nil } if !reflect.DeepEqual(expectedModel.SourceControl, cfg.SourceControl) { log.Info("System needs update: source control is not equal") - return true + return true, nil } namesAreEqual := expectedModel.Name == cfg.Name if !namesAreEqual { log.Info("System needs update: system names are not are not equal") - return true + return true, nil } dmsAreEqual := styra.DecisionMappingsEquals(expectedModel.DecisionMappings, cfg.DecisionMappings) if !dmsAreEqual { log.Info("System needs update: decision mappings are not equal") - return true + return true, nil } - return false + return false, nil } // SetupWithManager registers the the System controller with the Manager. diff --git a/internal/controller/styra/system_controller_test.go b/internal/controller/styra/system_controller_test.go index 72066424..2fd0ed2b 100644 --- a/internal/controller/styra/system_controller_test.go +++ b/internal/controller/styra/system_controller_test.go @@ -116,3 +116,18 @@ var _ = ginkgo.DescribeTable("createRolebindingSubjects", }, ), ) + +// test the isURLValid method +var _ = ginkgo.DescribeTable("isURLValid", + func(url string, expected bool) { + gomega.Ω(isURLValid(url)).To(gomega.Equal(expected)) + }, + ginkgo.Entry("valid url", "", true), + ginkgo.Entry("valid url", "https://www.github.com/test/repo.git", true), + ginkgo.Entry("valid url", "https://www.github.com/test/repo", true), + ginkgo.Entry("invalid url", "https://www.github.com/[test]/repo", false), + ginkgo.Entry("invalid url", "https://www.github.com/[test]/repo.git", false), + ginkgo.Entry("invalid url", "www.google.com", false), + ginkgo.Entry("invalid url", "google.com", false), + ginkgo.Entry("invalid url", "google", false), +) diff --git a/test/integration/controller/system_controller_test.go b/test/integration/controller/system_controller_test.go index 6ab54657..a4e08abd 100644 --- a/test/integration/controller/system_controller_test.go +++ b/test/integration/controller/system_controller_test.go @@ -41,8 +41,15 @@ import ( var _ = ginkgo.Describe("SystemReconciler.Reconcile", ginkgo.Label("integration"), func() { ginkgo.It("should reconcile", func() { + sourceControl := styrav1beta1.SourceControl{ + Origin: styrav1beta1.GitRepo{ + URL: "https://github.com/test/repo.git", + }, + } + spec := styrav1beta1.SystemSpec{ DeletionProtection: ptr.Bool(false), + SourceControl: &sourceControl, } key := types.NamespacedName{ @@ -91,6 +98,15 @@ var _ = ginkgo.Describe("SystemReconciler.Reconcile", ginkgo.Label("integration" Type: "custom", ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }).Return(&styra.UpdateSystemResponse{ StatusCode: http.StatusOK, @@ -100,6 +116,15 @@ var _ = ginkgo.Describe("SystemReconciler.Reconcile", ginkgo.Label("integration" ReadOnly: true, ID: cfg.ID, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }, nil).Once() @@ -144,6 +169,15 @@ var _ = ginkgo.Describe("SystemReconciler.Reconcile", ginkgo.Label("integration" Name: key.String(), ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }, nil).Once() @@ -178,6 +212,15 @@ var _ = ginkgo.Describe("SystemReconciler.Reconcile", ginkgo.Label("integration" Name: key.String(), ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }, nil).Once() @@ -268,6 +311,7 @@ discovery: getSystem int getSystemByName int createSystem int + updateSystem int deletePolicy int getUsers int rolebindingsListed int @@ -280,6 +324,8 @@ discovery: getSystem++ case "CreateSystem": createSystem++ + case "UpdateSystem": + updateSystem++ case "GetSystemByName": getSystemByName++ case "DeletePolicy": @@ -297,6 +343,7 @@ discovery: return getSystem == 2 && getSystemByName == 1 && createSystem == 1 && + updateSystem == 1 && deletePolicy == 2 && getUsers == 3 && rolebindingsListed == 3 && @@ -323,6 +370,15 @@ discovery: Type: "custom", ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }, nil).Once() @@ -349,7 +405,6 @@ discovery: }, nil).Once() // new reconcile as we create opa configmap that we watch - styraClientMock.On("GetSystem", mock.Anything, "system_id").Return( &styra.GetSystemResponse{ StatusCode: http.StatusOK, @@ -358,6 +413,15 @@ discovery: Type: "custom", ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }, nil).Once() @@ -384,7 +448,6 @@ discovery: }, nil).Once() // new reconcile as we create slp configmap that we watch - styraClientMock.On("GetSystem", mock.Anything, "system_id").Return( &styra.GetSystemResponse{ StatusCode: http.StatusOK, @@ -393,6 +456,15 @@ discovery: Type: "custom", ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, }, }, nil).Once() @@ -542,6 +614,15 @@ discovery: Type: "custom", ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -564,6 +645,15 @@ discovery: ID: "system_id", Name: key.String(), BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -654,6 +744,15 @@ discovery: Name: key.String(), ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -771,6 +870,15 @@ discovery: Name: key.String(), ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -878,6 +986,15 @@ discovery: ID: "system_id", Name: key.String(), ReadOnly: true, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -1008,6 +1125,15 @@ discovery: Name: key.String(), ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + SourceControl: &styra.SourceControlConfig{ + Origin: styra.GitRepoConfig{ + URL: sourceControl.Origin.URL, + Credentials: "systems/system_id/git", + Path: "", + Commit: "", + Reference: "", + }, + }, DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -1137,6 +1263,7 @@ discovery: Name: key.String(), ReadOnly: true, BundleDownload: &styra.BundleDownloadConfig{DeltaBundles: false}, + DecisionMappings: map[string]styra.DecisionMapping{ "": {}, "test": { @@ -1380,6 +1507,10 @@ discovery: ginkgo.By("Connecting to an existing Styra System by name with wrong deltabundle setting") + spec2 := styrav1beta1.SystemSpec{ + DeletionProtection: ptr.Bool(false), + } + key2 := types.NamespacedName{ Name: "test2", Namespace: "default", @@ -1390,7 +1521,7 @@ discovery: Name: key2.Name, Namespace: key2.Namespace, }, - Spec: spec, + Spec: spec2, } cfg2 := &styra.SystemConfig{ @@ -1535,7 +1666,6 @@ discovery: resetMock(&styraClientMock.Mock) // Create new system, which was not ReadOnly, so it gets updated - ginkgo.By("Changing ReadOnly flag in system") key3 := types.NamespacedName{ @@ -1549,7 +1679,7 @@ discovery: Name: key3.Name, Namespace: key3.Namespace, }, - Spec: spec, + Spec: spec2, } cfg3 := &styra.SystemConfig{ @@ -2007,6 +2137,67 @@ distributed_tracing: resetMock(&styraClientMock.Mock) + ginkgo.By("Creating the system will fail due to invalid url") + + key5 := types.NamespacedName{ + Name: "test5", + Namespace: "default", + } + + sourceControl5 := styrav1beta1.SourceControl{ + Origin: styrav1beta1.GitRepo{ + URL: "https://github.com/[test]/repo.git", + }, + } + spec5 := styrav1beta1.SystemSpec{ + DeletionProtection: ptr.Bool(false), + SourceControl: &sourceControl5, + } + + toCreate5 := &styrav1beta1.System{ + ObjectMeta: metav1.ObjectMeta{ + Name: key5.Name, + Namespace: key5.Namespace, + }, + Spec: spec5, + } + + // Mock the GetSystemByName call to return nil, indicating the system does not exist + styraClientMock.On("GetSystemByName", mock.Anything, key5.String()).Return(&styra.GetSystemResponse{ + StatusCode: http.StatusOK, + SystemConfig: nil, + }, nil).Times(13) + + // Create the system in the Kubernetes cluster + gomega.Expect(k8sClient.Create(ctx, toCreate5)).To(gomega.Succeed()) + + // Verify that the system creation failed due to invalid URL + gomega.Eventually(func() bool { + fetched := &styrav1beta1.System{} + err := k8sClient.Get(ctx, key5, fetched) + if err != nil { + return false + } + + return fetched.Status.Phase == styrav1beta1.SystemPhaseFailed && !fetched.Status.Ready + }, timeout, interval).Should(gomega.BeTrue()) + + gomega.Eventually(func() bool { + var ( + getSystemByName int + ) + for _, call := range styraClientMock.Calls { + switch call.Method { + case "GetSystemByName": + getSystemByName++ + } + } + + return getSystemByName == 13 + }, timeout, interval).Should(gomega.BeTrue()) + + resetMock(&styraClientMock.Mock) + styraClientMock.AssertExpectations(ginkgo.GinkgoT()) }) })