Skip to content
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
20 changes: 20 additions & 0 deletions assets/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -6544,6 +6544,23 @@
}
}
},
"v1alpha1ManagedNamespaceMetadata": {
"type": "object",
"properties": {
"annotations": {
"type": "object",
"additionalProperties": {
"type": "string"
}
},
"labels": {
"type": "object",
"additionalProperties": {
"type": "string"
}
}
}
},
"v1alpha1MatrixGenerator": {
"description": "MatrixGenerator generates the cartesian product of two sets of parameters. The parameters are defined by two nested\ngenerators.",
"type": "object",
Expand Down Expand Up @@ -7827,6 +7844,9 @@
"automated": {
"$ref": "#/definitions/v1alpha1SyncPolicyAutomated"
},
"managedNamespaceMetadata": {
"$ref": "#/definitions/v1alpha1ManagedNamespaceMetadata"
},
"retry": {
"$ref": "#/definitions/v1alpha1RetryStrategy"
},
Expand Down
33 changes: 17 additions & 16 deletions controller/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
cdcommon "github.com/argoproj/argo-cd/v2/common"
"os"
"strconv"
"sync/atomic"
Expand All @@ -20,7 +21,6 @@ import (
"k8s.io/apimachinery/pkg/util/managedfields"
"k8s.io/kubectl/pkg/util/openapi"

cdcommon "github.com/argoproj/argo-cd/v2/common"
"github.com/argoproj/argo-cd/v2/controller/metrics"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
listersv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1"
Expand Down Expand Up @@ -212,14 +212,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
}
trackingMethod := argo.GetTrackingMethod(m.settingsMgr)

syncCtx, cleanup, err := sync.NewSyncContext(
compareResult.syncStatus.Revision,
reconciliationResult,
restConfig,
rawConfig,
m.kubectl,
app.Spec.Destination.Namespace,
openAPISchema,
opts := []sync.SyncOpt{
sync.WithLogr(logutils.NewLogrusLogger(logEntry)),
sync.WithHealthOverride(lua.ResourceHealthOverrides(resourceOverrides)),
sync.WithPermissionValidator(func(un *unstructured.Unstructured, res *v1.APIResource) error {
Expand Down Expand Up @@ -249,20 +242,28 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
m.isSelfReferencedObj(live, target, app.GetName(), appLabelKey, trackingMethod)
}),
sync.WithManifestValidation(!syncOp.SyncOptions.HasOption(common.SyncOptionsDisableValidation)),
sync.WithNamespaceCreation(syncOp.SyncOptions.HasOption("CreateNamespace=true"), func(un *unstructured.Unstructured) bool {
if un != nil && kube.GetAppInstanceLabel(un, cdcommon.LabelKeyAppInstance) != "" {
kube.UnsetLabel(un, cdcommon.LabelKeyAppInstance)
return true
Comment on lines 253 to 255
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems that this logic isn't present in the syncNamespace function. Is there a reason to stop cleaning the instance label from the namespace?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Don't think there's any particular reason; think this was an artifact from when the resource tracking was always being set. I've re-added it, but what is the reason for having this? If this is significant, I'd add a test to check that this gets unset.

Copy link
Copy Markdown
Collaborator

@leoluz leoluz Oct 19, 2022

Choose a reason for hiding this comment

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

Just a bit of history (@alexmt correct me if I'm wrong :)
The tracking id is how Argo CD tracks the resources belonging to an Application. Previously Argo CD used to use a standard label for that but that causes some conflicts with other controllers. To avoid this problem, Argo CD is now gradually switching from the standard label to a proprietary annotation. This whole logic is handled in util/argo/resource_tracking.go

As far as I understand the previous code, it was cleaning up the label from a previously tracked namespace. This was only happening if the namespace already existed in k8s and it had the tracking label which means that the namespace yaml was provided in git. If we want to keep the current behaviour we should clean the label only if:

  • Application is configured to create namespace
  • Application is not configured to manage namespace metadata

Btw, cleaning only the label in the scenario above is probably a bug we currently have. I suspect that we should clean the label AND the annotation if they are present and the Application is under the 2 conditions above.

FYI, I am going to bring this to the contributors meeting this week (will happen Oct 20th 8:15AM PST)

}
return false
}),
sync.WithSyncWaveHook(delayBetweenSyncWaves),
sync.WithPruneLast(syncOp.SyncOptions.HasOption(common.SyncOptionPruneLast)),
sync.WithResourceModificationChecker(syncOp.SyncOptions.HasOption("ApplyOutOfSyncOnly=true"), compareResult.diffResultList),
sync.WithPrunePropagationPolicy(&prunePropagationPolicy),
sync.WithReplace(syncOp.SyncOptions.HasOption(common.SyncOptionReplace)),
sync.WithServerSideApply(syncOp.SyncOptions.HasOption(common.SyncOptionServerSideApply)),
sync.WithServerSideApplyManager(cdcommon.ArgoCDSSAManager),
}

if syncOp.SyncOptions.HasOption("CreateNamespace=true") {
opts = append(opts, sync.WithNamespaceModifier(syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app.Name, app.Spec.SyncPolicy)))
}

syncCtx, cleanup, err := sync.NewSyncContext(
compareResult.syncStatus.Revision,
reconciliationResult,
restConfig,
rawConfig,
m.kubectl,
app.Spec.Destination.Namespace,
openAPISchema,
opts...,
)

if err != nil {
Expand Down
51 changes: 51 additions & 0 deletions controller/sync_namespace.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package controller

import (
"fmt"
cdcommon "github.com/argoproj/argo-cd/v2/common"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/argoproj/argo-cd/v2/util/argo"
gitopscommon "github.com/argoproj/gitops-engine/pkg/sync/common"
"github.com/argoproj/gitops-engine/pkg/utils/kube"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, trackingMethod v1alpha1.TrackingMethod, appName string, syncPolicy *v1alpha1.SyncPolicy) func(un *unstructured.Unstructured) (bool, error) {
return func(liveNs *unstructured.Unstructured) (bool, error) {
if liveNs != nil && kube.GetAppInstanceLabel(liveNs, cdcommon.LabelKeyAppInstance) != "" {
kube.UnsetLabel(liveNs, cdcommon.LabelKeyAppInstance)
return true, nil
}

isNewNamespace := liveNs != nil && liveNs.GetUID() == "" && liveNs.GetResourceVersion() == ""

if liveNs != nil && syncPolicy != nil {
// managedNamespaceMetadata relies on SSA, and since the diffs are computed by the k8s control plane we
// always need to call the k8s api server, so we'll always need to return true if managedNamespaceMetadata is set.
hasManagedMetadata := syncPolicy.ManagedNamespaceMetadata != nil
if hasManagedMetadata {
managedNamespaceMetadata := syncPolicy.ManagedNamespaceMetadata
liveNs.SetLabels(managedNamespaceMetadata.Labels)
liveNs.SetAnnotations(appendSSAAnnotation(managedNamespaceMetadata.Annotations))

err := resourceTracking.SetAppInstance(liveNs, appLabelKey, appName, "", trackingMethod)
if err != nil {
return false, fmt.Errorf("failed to set app instance tracking on the namespace %s: %s", liveNs.GetName(), err)
}

return true, nil
}
}

return isNewNamespace, nil
}
}

func appendSSAAnnotation(in map[string]string) map[string]string {
r := map[string]string{}
for k, v := range in {
r[k] = v
}
r[gitopscommon.AnnotationSyncOptions] = gitopscommon.SyncOptionServerSideApply
return r
}
Loading