From 91c1ab10169daf9c43b7f4bf7b574afabde058fe Mon Sep 17 00:00:00 2001 From: pashavictorovich Date: Tue, 28 Jun 2022 22:46:35 +0300 Subject: [PATCH 1/4] disable sync limitation and report errors in better way --- controller/sync.go | 3 +-- .../application/v1alpha1/app_project_types.go | 21 ++-------------- .../v1alpha1/app_project_types_test.go | 8 +++--- pkg/apis/application/v1alpha1/types_test.go | 2 +- .../application/application_errors_parser.go | 23 +++++++++++++++++ .../application/application_event_reporter.go | 25 ++++--------------- .../applications_errors_parser_test.go | 16 ++++++++++++ server/project/project.go | 4 +-- util/argo/argo.go | 2 +- 9 files changed, 55 insertions(+), 49 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 44ab2328eadcb..a50aa1c0e6a74 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -204,8 +204,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha if !proj.IsGroupKindPermitted(un.GroupVersionKind().GroupKind(), res.Namespaced) { return fmt.Errorf("Resource %s:%s is not permitted in project %s.", un.GroupVersionKind().Group, un.GroupVersionKind().Kind, proj.Name) } - allowedApplications, _ := m.settingsMgr.GetAppsAllowedToDeliverIncluster() - if res.Namespaced && !proj.IsDestinationPermitted(v1alpha1.ApplicationDestination{Namespace: un.GetNamespace(), Server: app.Spec.Destination.Server, Name: app.Spec.Destination.Name}, app.ObjectMeta.Name, allowedApplications) { + if res.Namespaced && !proj.IsDestinationPermitted(v1alpha1.ApplicationDestination{Namespace: un.GetNamespace(), Server: app.Spec.Destination.Server, Name: app.Spec.Destination.Name}) { return fmt.Errorf("namespace %v is not permitted in project '%s'", un.GetNamespace(), proj.Name) } return nil diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 58d84cf7da899..02c0356875f65 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -321,7 +321,7 @@ func (proj AppProject) IsResourcePermitted(groupKind schema.GroupKind, namespace return false } if namespace != "" { - return proj.IsDestinationPermitted(ApplicationDestination{Server: dest.Server, Name: dest.Name, Namespace: namespace}, "", nil) + return proj.IsDestinationPermitted(ApplicationDestination{Server: dest.Server, Name: dest.Name, Namespace: namespace}) } return true } @@ -356,24 +356,7 @@ func (proj AppProject) IsSourcePermitted(src ApplicationSource) bool { } // IsDestinationPermitted validates if the provided application's destination is one of the allowed destinations for the project -func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, applicationName string, appsThatAllowedRunInCluster []string) bool { - applicationExistsFunc := func() bool { - for _, appNameFromSettings := range appsThatAllowedRunInCluster { - if appNameFromSettings == applicationName { - return true - } - } - return false - } - - // in case if application destination is in-cluster and allowed apps defined in argo-cm - if (dst.Server == KubernetesInternalAPIServerAddr || dst.Name == "in-cluster") && len(appsThatAllowedRunInCluster) > 0 { - applicationExists := applicationExistsFunc() - if !applicationExists { - return false - } - } - +func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination) bool { for _, item := range proj.Spec.Destinations { dstNameMatched := dst.Name != "" && globMatch(item.Name, dst.Name) dstServerMatched := dst.Server != "" && globMatch(item.Server, dst.Server) diff --git a/pkg/apis/application/v1alpha1/app_project_types_test.go b/pkg/apis/application/v1alpha1/app_project_types_test.go index 2d6c446758892..7807e6659af38 100644 --- a/pkg/apis/application/v1alpha1/app_project_types_test.go +++ b/pkg/apis/application/v1alpha1/app_project_types_test.go @@ -18,7 +18,7 @@ func TestIsDestinationPermitted(t *testing.T) { }, } - rs := pr.IsDestinationPermitted(ApplicationDestination{Server: KubernetesInternalAPIServerAddr}, "test", []string{"test"}) + rs := pr.IsDestinationPermitted(ApplicationDestination{Server: KubernetesInternalAPIServerAddr}) assert.True(t, rs) }) t.Run("rejected", func(t *testing.T) { @@ -32,7 +32,7 @@ func TestIsDestinationPermitted(t *testing.T) { }, } - rs := pr.IsDestinationPermitted(ApplicationDestination{Server: KubernetesInternalAPIServerAddr}, "test", []string{"test2"}) + rs := pr.IsDestinationPermitted(ApplicationDestination{Server: KubernetesInternalAPIServerAddr}) assert.False(t, rs) }) t.Run("allowed-name", func(t *testing.T) { @@ -46,7 +46,7 @@ func TestIsDestinationPermitted(t *testing.T) { }, } - rs := pr.IsDestinationPermitted(ApplicationDestination{Name: "in-cluster"}, "test", []string{"test"}) + rs := pr.IsDestinationPermitted(ApplicationDestination{Name: "in-cluster"}) assert.True(t, rs) }) t.Run("rejected-name", func(t *testing.T) { @@ -60,7 +60,7 @@ func TestIsDestinationPermitted(t *testing.T) { }, } - rs := pr.IsDestinationPermitted(ApplicationDestination{Name: "in-cluster"}, "test", []string{"test2"}) + rs := pr.IsDestinationPermitted(ApplicationDestination{Name: "in-cluster"}) assert.False(t, rs) }) } diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index c9cc8dd4491b5..4681258c7165c 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -137,7 +137,7 @@ func TestAppProject_IsDestinationPermitted(t *testing.T) { Destinations: data.projDest, }, } - assert.Equal(t, proj.IsDestinationPermitted(data.appDest, "", nil), data.isPermitted) + assert.Equal(t, proj.IsDestinationPermitted(data.appDest), data.isPermitted) } } diff --git a/server/application/application_errors_parser.go b/server/application/application_errors_parser.go index 362024153ad10..b59c16103bf49 100644 --- a/server/application/application_errors_parser.go +++ b/server/application/application_errors_parser.go @@ -1,6 +1,7 @@ package application import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "strings" "github.com/argoproj/gitops-engine/pkg/health" @@ -25,6 +26,28 @@ func parseApplicationSyncResultErrors(os *appv1.OperationState) []*events.Object return errors } +func parseApplicationSyncResultErrorsFromConditions(conditions []appv1.ApplicationCondition) []*events.ObjectError { + var errs []*events.ObjectError + for _, cnd := range conditions { + if !strings.Contains(strings.ToLower(cnd.Type), "error") { + continue + } + + lastSeen := metav1.Now() + if cnd.LastTransitionTime != nil { + lastSeen = *cnd.LastTransitionTime + } + + errs = append(errs, &events.ObjectError{ + Type: "sync", + Level: "error", + Message: cnd.Message, + LastSeen: lastSeen, + }) + } + return errs +} + func parseResourceSyncResultErrors(rs *appv1.ResourceStatus, os *appv1.OperationState) []*events.ObjectError { errors := []*events.ObjectError{} if os.SyncResult == nil { diff --git a/server/application/application_event_reporter.go b/server/application/application_event_reporter.go index acd91d64caaf6..098bf8cbe47af 100644 --- a/server/application/application_event_reporter.go +++ b/server/application/application_event_reporter.go @@ -390,6 +390,10 @@ func getResourceEventPayload( errors = append(errors, parseApplicationSyncResultErrors(originalApplication.Status.OperationState)...) } + if originalApplication != nil && originalApplication.Status.Conditions != nil { + errors = append(errors, parseApplicationSyncResultErrorsFromConditions(originalApplication.Status.Conditions)...) + } + if len(desiredState.RawManifest) == 0 && len(desiredState.CompiledManifest) != 0 { // for handling helm defined resources, etc... y, err := yaml.JSONToYAML([]byte(desiredState.CompiledManifest)) @@ -515,30 +519,11 @@ func (s *applicationEventReporter) getApplicationEventPayload(ctx context.Contex Cluster: a.Spec.Destination.Server, } - errs := []*events.ObjectError{} - for _, cnd := range a.Status.Conditions { - if !strings.Contains(strings.ToLower(cnd.Type), "error") { - continue - } - - lastSeen := metav1.Now() - if cnd.LastTransitionTime != nil { - lastSeen = *cnd.LastTransitionTime - } - - errs = append(errs, &events.ObjectError{ - Type: "sync", - Level: "error", - Message: cnd.Message, - LastSeen: lastSeen, - }) - } - payload := events.EventPayload{ Timestamp: ts, Object: object, Source: source, - Errors: errs, + Errors: parseApplicationSyncResultErrorsFromConditions(a.Status.Conditions), } payloadBytes, err := json.Marshal(&payload) diff --git a/server/application/applications_errors_parser_test.go b/server/application/applications_errors_parser_test.go index c05cde0eff3e6..7463451152bd2 100644 --- a/server/application/applications_errors_parser_test.go +++ b/server/application/applications_errors_parser_test.go @@ -96,6 +96,22 @@ func TestParseResourceSyncResultErrors(t *testing.T) { }) } +func TestParseApplicationSyncResultErrorsFromConditions(t *testing.T) { + t.Run("conditions exists", func(t *testing.T) { + errors := parseApplicationSyncResultErrorsFromConditions([]v1alpha1.ApplicationCondition{ + { + Type: "error", + Message: "error message", + }, + }) + + assert.Len(t, errors, 1) + assert.Equal(t, errors[0].Message, "error message") + assert.Equal(t, errors[0].Type, "sync") + assert.Equal(t, errors[0].Level, "error") + }) +} + func TestParseAggregativeHealthErrors(t *testing.T) { t.Run("application tree is nil", func(t *testing.T) { errs := parseAggregativeHealthErrors(&v1alpha1.ResourceStatus{ diff --git a/server/project/project.go b/server/project/project.go index 5adb99e180ea3..6877d3a9d6982 100644 --- a/server/project/project.go +++ b/server/project/project.go @@ -341,7 +341,7 @@ func (s *Server) Update(ctx context.Context, q *project.ProjectUpdateRequest) (* if oldProj.IsSourcePermitted(a.Spec.Source) { srcValidatedApps = append(srcValidatedApps, a) } - if oldProj.IsDestinationPermitted(a.Spec.Destination, "", nil) { + if oldProj.IsDestinationPermitted(a.Spec.Destination) { dstValidatedApps = append(dstValidatedApps, a) } } @@ -355,7 +355,7 @@ func (s *Server) Update(ctx context.Context, q *project.ProjectUpdateRequest) (* } } for _, a := range dstValidatedApps { - if !q.Project.IsDestinationPermitted(a.Spec.Destination, "", nil) { + if !q.Project.IsDestinationPermitted(a.Spec.Destination) { invalidDstCount++ } } diff --git a/util/argo/argo.go b/util/argo/argo.go index 7390d92ab3e04..a84dc04a82724 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -382,7 +382,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p } if spec.Destination.Server != "" { - if !proj.IsDestinationPermitted(spec.Destination, "", nil) { + if !proj.IsDestinationPermitted(spec.Destination) { conditions = append(conditions, argoappv1.ApplicationCondition{ Type: argoappv1.ApplicationConditionInvalidSpecError, Message: fmt.Sprintf("application destination {%s %s} is not permitted in project '%s'", spec.Destination.Server, spec.Destination.Namespace, spec.Project), From 531824a7dc020e79c7e28428f03366571e1c2629 Mon Sep 17 00:00:00 2001 From: pashavictorovich Date: Tue, 28 Jun 2022 22:48:59 +0300 Subject: [PATCH 2/4] report condition errors --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index ca7de51a302bd..fd204e974c55f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.3.4-cap-CR-empty-desired-state +2.3.4-cap-CR-report-condition-errors From e799fdaddcd142bd0ddddba706523be9bd707a60 Mon Sep 17 00:00:00 2001 From: pashavictorovich Date: Tue, 28 Jun 2022 23:04:57 +0300 Subject: [PATCH 3/4] fix linter --- server/application/application_errors_parser.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/application/application_errors_parser.go b/server/application/application_errors_parser.go index b59c16103bf49..6101f5042c8eb 100644 --- a/server/application/application_errors_parser.go +++ b/server/application/application_errors_parser.go @@ -1,9 +1,10 @@ package application import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "strings" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/argoproj/gitops-engine/pkg/health" "github.com/argoproj/gitops-engine/pkg/sync/common" From 0d72784ab254423a6eacd0be4675e494e2efbeaf Mon Sep 17 00:00:00 2001 From: pashavictorovich Date: Tue, 28 Jun 2022 23:22:18 +0300 Subject: [PATCH 4/4] non fail if cant get desired manifest, just leave it empty --- .../v1alpha1/app_project_types_test.go | 66 ------------------- 1 file changed, 66 deletions(-) delete mode 100644 pkg/apis/application/v1alpha1/app_project_types_test.go diff --git a/pkg/apis/application/v1alpha1/app_project_types_test.go b/pkg/apis/application/v1alpha1/app_project_types_test.go deleted file mode 100644 index 7807e6659af38..0000000000000 --- a/pkg/apis/application/v1alpha1/app_project_types_test.go +++ /dev/null @@ -1,66 +0,0 @@ -package v1alpha1 - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestIsDestinationPermitted(t *testing.T) { - t.Run("allowed", func(t *testing.T) { - pr := AppProject{ - Spec: AppProjectSpec{ - Destinations: []ApplicationDestination{ - { - Server: KubernetesInternalAPIServerAddr, - }, - }, - }, - } - - rs := pr.IsDestinationPermitted(ApplicationDestination{Server: KubernetesInternalAPIServerAddr}) - assert.True(t, rs) - }) - t.Run("rejected", func(t *testing.T) { - pr := AppProject{ - Spec: AppProjectSpec{ - Destinations: []ApplicationDestination{ - { - Server: KubernetesInternalAPIServerAddr, - }, - }, - }, - } - - rs := pr.IsDestinationPermitted(ApplicationDestination{Server: KubernetesInternalAPIServerAddr}) - assert.False(t, rs) - }) - t.Run("allowed-name", func(t *testing.T) { - pr := AppProject{ - Spec: AppProjectSpec{ - Destinations: []ApplicationDestination{ - { - Name: "in-cluster", - }, - }, - }, - } - - rs := pr.IsDestinationPermitted(ApplicationDestination{Name: "in-cluster"}) - assert.True(t, rs) - }) - t.Run("rejected-name", func(t *testing.T) { - pr := AppProject{ - Spec: AppProjectSpec{ - Destinations: []ApplicationDestination{ - { - Name: "in-cluster", - }, - }, - }, - } - - rs := pr.IsDestinationPermitted(ApplicationDestination{Name: "in-cluster"}) - assert.False(t, rs) - }) -}