Skip to content

Commit

Permalink
fix: Apply the creator labels about the user who resubmitted a Workfl…
Browse files Browse the repository at this point in the history 
…ow (#11415)
  • Loading branch information
@umi0410 @terrytangyuan
umi0410 authored and terrytangyuan committed Sep 5, 2023
1 parent b0909c6 commit edfde16
Show file tree
Hide file tree
Showing 7 changed files with 164 additions and 12 deletions.
2 changes: 1 addition & 1 deletion server/workflow/workflow_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ func (s *workflowServer) ResubmitWorkflow(ctx context.Context, req *workflowpkg.
return nil, sutils.ToStatusError(err, codes.InvalidArgument)
}

newWF, err := util.FormulateResubmitWorkflow(wf, req.Memoized, req.Parameters)
newWF, err := util.FormulateResubmitWorkflow(ctx, wf, req.Memoized, req.Parameters)
if err != nil {
return nil, sutils.ToStatusError(err, codes.Internal)
}
Expand Down
2 changes: 1 addition & 1 deletion server/workflowarchive/archived_workflow_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ func (w *archivedWorkflowServer) ResubmitArchivedWorkflow(ctx context.Context, r
return nil, sutils.ToStatusError(err, codes.Internal)
}

newWF, err := util.FormulateResubmitWorkflow(wf, req.Memoized, req.Parameters)
newWF, err := util.FormulateResubmitWorkflow(ctx, wf, req.Memoized, req.Parameters)
if err != nil {
return nil, sutils.ToStatusError(err, codes.Internal)
}
Expand Down
8 changes: 4 additions & 4 deletions workflow/controller/operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5502,12 +5502,12 @@ status:
name: my-wf
phase: Failed
`)
wf, err := util.FormulateResubmitWorkflow(wf, true, nil)
ctx := context.Background()
wf, err := util.FormulateResubmitWorkflow(ctx, wf, true, nil)
if assert.NoError(t, err) {
cancel, controller := newController(wf)
defer cancel()

ctx := context.Background()
woc := newWorkflowOperationCtx(wf, controller)
woc.operate(ctx)
assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase)
Expand Down Expand Up @@ -5551,12 +5551,12 @@ status:
name: my-wf
phase: Failed
`)
wf, err := util.FormulateResubmitWorkflow(wf, true, []string{"message=modified"})
ctx := context.Background()
wf, err := util.FormulateResubmitWorkflow(ctx, wf, true, []string{"message=modified"})
if assert.NoError(t, err) {
cancel, controller := newController(wf)
defer cancel()

ctx := context.Background()
woc := newWorkflowOperationCtx(wf, controller)
woc.operate(ctx)
assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase)
Expand Down
16 changes: 15 additions & 1 deletion workflow/creator/creator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,27 @@ import (
func Label(ctx context.Context, obj metav1.Object) {
claims := auth.GetClaims(ctx)
if claims != nil {
labels.Label(obj, common.LabelKeyCreator, dnsFriendly(claims.Subject))
if claims.Subject != "" {
labels.Label(obj, common.LabelKeyCreator, dnsFriendly(claims.Subject))
} else {
labels.UnLabel(obj, common.LabelKeyCreator)
}
if claims.Email != "" {
labels.Label(obj, common.LabelKeyCreatorEmail, dnsFriendly(strings.Replace(claims.Email, "@", ".at.", 1)))
} else {
labels.UnLabel(obj, common.LabelKeyCreatorEmail)
}
if claims.PreferredUsername != "" {
labels.Label(obj, common.LabelKeyCreatorPreferredUsername, dnsFriendly(claims.PreferredUsername))
} else {
labels.UnLabel(obj, common.LabelKeyCreatorPreferredUsername)
}
} else {
// If the object already has creator-related labels, but the actual request lacks auth information,
// remove the creator-related labels from the object.
labels.UnLabel(obj, common.LabelKeyCreator)
labels.UnLabel(obj, common.LabelKeyCreatorEmail)
labels.UnLabel(obj, common.LabelKeyCreatorPreferredUsername)
}
}

Expand Down
77 changes: 77 additions & 0 deletions workflow/creator/creator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/go-jose/go-jose/v3/jwt"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/argoproj/argo-workflows/v3/server/auth"
Expand Down Expand Up @@ -52,4 +53,80 @@ func TestLabel(t *testing.T) {
assert.Equal(t, strings.Repeat("x", 20), wf.Labels[common.LabelKeyCreator])
}
})
t.Run("DifferentUsersFromCreatorLabels", func(t *testing.T) {
type input struct {
claims *types.Claims
wf *wfv1.Workflow
}
type output struct {
creatorLabelsToHave map[string]string
creatorLabelsNotToHave []string
}
for _, testCase := range []struct {
name string
input *input
output *output
}{
{
name: "when claims are empty",
input: &input{
claims: &types.Claims{Claims: jwt.Claims{}},
wf: &wfv1.Workflow{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{
common.LabelKeyCreator: "xxxx-xxxx-xxxx-xxxx",
common.LabelKeyCreatorEmail: "foo.at.example.com",
common.LabelKeyCreatorPreferredUsername: "foo",
}}},
},
output: &output{
creatorLabelsToHave: nil,
creatorLabelsNotToHave: []string{common.LabelKeyCreator, common.LabelKeyCreatorEmail, common.LabelKeyCreatorPreferredUsername},
},
}, {
name: "when claims are nil",
input: &input{
claims: nil,
wf: &wfv1.Workflow{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{
common.LabelKeyCreator: "xxxx-xxxx-xxxx-xxxx",
common.LabelKeyCreatorEmail: "foo.at.example.com",
common.LabelKeyCreatorPreferredUsername: "foo",
}}},
},
output: &output{
creatorLabelsToHave: nil,
creatorLabelsNotToHave: []string{common.LabelKeyCreator, common.LabelKeyCreatorEmail, common.LabelKeyCreatorPreferredUsername},
},
}, {
name: "when user information in claim is different from the existing labels of a Workflow",
input: &input{
claims: &types.Claims{Claims: jwt.Claims{Subject: "yyyy-yyyy-yyyy-yyyy"}, Email: "bar.at.example.com", PreferredUsername: "bar"},
wf: &wfv1.Workflow{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{
common.LabelKeyCreator: "xxxx-xxxx-xxxx-xxxx",
common.LabelKeyCreatorEmail: "foo.at.example.com",
common.LabelKeyCreatorPreferredUsername: "foo",
}}},
},
output: &output{
creatorLabelsToHave: map[string]string{
common.LabelKeyCreator: "yyyy-yyyy-yyyy-yyyy",
common.LabelKeyCreatorEmail: "bar.at.example.com",
common.LabelKeyCreatorPreferredUsername: "bar",
},
creatorLabelsNotToHave: nil,
},
},
} {
t.Run(testCase.name, func(t *testing.T) {
Label(context.WithValue(context.TODO(), auth.ClaimsKey, testCase.input.claims), testCase.input.wf)
labels := testCase.input.wf.GetLabels()
for k, expectedValue := range testCase.output.creatorLabelsToHave {
assert.Equal(t, expectedValue, labels[k])
}
for _, creatorLabelKey := range testCase.output.creatorLabelsNotToHave {
_, ok := labels[creatorLabelKey]
assert.Falsef(t, ok, "should not have the creator label, \"%s\"", creatorLabelKey)
}
})

}
})
}
10 changes: 8 additions & 2 deletions workflow/util/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import (
"k8s.io/utils/pointer"
"sigs.k8s.io/yaml"

"github.com/argoproj/argo-workflows/v3/workflow/creator"

"github.com/argoproj/argo-workflows/v3/errors"
"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow"
wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
Expand Down Expand Up @@ -606,7 +608,7 @@ func RandSuffix() string {
}

// FormulateResubmitWorkflow formulate a new workflow from a previous workflow, optionally re-using successful nodes
func FormulateResubmitWorkflow(wf *wfv1.Workflow, memoized bool, parameters []string) (*wfv1.Workflow, error) {
func FormulateResubmitWorkflow(ctx context.Context, wf *wfv1.Workflow, memoized bool, parameters []string) (*wfv1.Workflow, error) {
newWF := wfv1.Workflow{}
newWF.TypeMeta = wf.TypeMeta

Expand Down Expand Up @@ -644,12 +646,16 @@ func FormulateResubmitWorkflow(wf *wfv1.Workflow, memoized bool, parameters []st
}
for key, val := range wf.ObjectMeta.Labels {
switch key {
case common.LabelKeyCreator, common.LabelKeyPhase, common.LabelKeyCompleted, common.LabelKeyWorkflowArchivingStatus:
case common.LabelKeyCreator, common.LabelKeyCreatorEmail, common.LabelKeyCreatorPreferredUsername,
common.LabelKeyPhase, common.LabelKeyCompleted, common.LabelKeyWorkflowArchivingStatus:
// ignore
default:
newWF.ObjectMeta.Labels[key] = val
}
}
// Apply creator labels based on the authentication information of the current request,
// regardless of the creator labels of the original Workflow.
creator.Label(ctx, &newWF)
// Append an additional label so it's easy for user to see the
// name of the original workflow that has been resubmitted.
newWF.ObjectMeta.Labels[common.LabelKeyPreviousWorkflowName] = wf.ObjectMeta.Name
Expand Down
61 changes: 58 additions & 3 deletions workflow/util/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ import (
"testing"
"time"

"github.com/go-jose/go-jose/v3/jwt"
"github.com/stretchr/testify/assert"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/fields"
"sigs.k8s.io/yaml"

"github.com/argoproj/argo-workflows/v3/server/auth"
"github.com/argoproj/argo-workflows/v3/server/auth/types"

"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow"
wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
argofake "github.com/argoproj/argo-workflows/v3/pkg/client/clientset/versioned/fake"
Expand Down Expand Up @@ -67,7 +71,7 @@ func TestResubmitWorkflowWithOnExit(t *testing.T) {
Name: onExitName,
Phase: wfv1.NodeSucceeded,
}
newWF, err := FormulateResubmitWorkflow(&wf, true, nil)
newWF, err := FormulateResubmitWorkflow(context.Background(), &wf, true, nil)
assert.NoError(t, err)
newWFOnExitName := newWF.ObjectMeta.Name + ".onExit"
newWFOneExitID := newWF.NodeID(newWFOnExitName)
Expand Down Expand Up @@ -640,7 +644,7 @@ func TestFormulateResubmitWorkflow(t *testing.T) {
},
},
}
wf, err := FormulateResubmitWorkflow(wf, false, nil)
wf, err := FormulateResubmitWorkflow(context.Background(), wf, false, nil)
if assert.NoError(t, err) {
assert.Contains(t, wf.GetLabels(), common.LabelKeyControllerInstanceID)
assert.Contains(t, wf.GetLabels(), common.LabelKeyClusterWorkflowTemplate)
Expand All @@ -656,6 +660,57 @@ func TestFormulateResubmitWorkflow(t *testing.T) {
assert.Equal(t, "testObj", wf.OwnerReferences[0].Name)
}
})
t.Run("OverrideCreatorLabels", func(t *testing.T) {
wf := &wfv1.Workflow{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
common.LabelKeyCreator: "xxxx-xxxx-xxxx",
common.LabelKeyCreatorEmail: "foo.at.example.com",
common.LabelKeyCreatorPreferredUsername: "foo",
},
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "test",
Name: "testObj",
},
},
},
}
ctx := context.WithValue(context.Background(), auth.ClaimsKey, &types.Claims{
Claims: jwt.Claims{Subject: "yyyy-yyyy-yyyy-yyyy"},
Email: "bar.at.example.com",
PreferredUsername: "bar",
})
wf, err := FormulateResubmitWorkflow(ctx, wf, false, nil)
if assert.NoError(t, err) {
assert.Equal(t, "yyyy-yyyy-yyyy-yyyy", wf.Labels[common.LabelKeyCreator])
assert.Equal(t, "bar.at.example.com", wf.Labels[common.LabelKeyCreatorEmail])
assert.Equal(t, "bar", wf.Labels[common.LabelKeyCreatorPreferredUsername])
}
})
t.Run("UnlabelCreatorLabels", func(t *testing.T) {
wf := &wfv1.Workflow{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
common.LabelKeyCreator: "xxxx-xxxx-xxxx",
common.LabelKeyCreatorEmail: "foo.at.example.com",
common.LabelKeyCreatorPreferredUsername: "foo",
},
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "test",
Name: "testObj",
},
},
},
}
wf, err := FormulateResubmitWorkflow(context.Background(), wf, false, nil)
if assert.NoError(t, err) {
assert.Emptyf(t, wf.Labels[common.LabelKeyCreator], "should not %s label when a workflow is resubmitted by an unauthenticated request", common.LabelKeyCreator)
assert.Emptyf(t, wf.Labels[common.LabelKeyCreatorEmail], "should not %s label when a workflow is resubmitted by an unauthenticated request", common.LabelKeyCreatorEmail)
assert.Emptyf(t, wf.Labels[common.LabelKeyCreatorPreferredUsername], "should not %s label when a workflow is resubmitted by an unauthenticated request", common.LabelKeyCreatorPreferredUsername)
}
})
t.Run("OverrideParams", func(t *testing.T) {
wf := &wfv1.Workflow{
Spec: wfv1.WorkflowSpec{Arguments: wfv1.Arguments{
Expand All @@ -664,7 +719,7 @@ func TestFormulateResubmitWorkflow(t *testing.T) {
},
}},
}
wf, err := FormulateResubmitWorkflow(wf, false, []string{"message=modified"})
wf, err := FormulateResubmitWorkflow(context.Background(), wf, false, []string{"message=modified"})
if assert.NoError(t, err) {
assert.Equal(t, "modified", wf.Spec.Arguments.Parameters[0].Value.String())
}
Expand Down

0 comments on commit edfde16

Please sign in to comment.