Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update workflow name and id validation #13424

Merged
merged 3 commits into from
Jun 5, 2024
Merged

update workflow name and id validation #13424

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e68862d
update workflow name and id validation
krehermann Jun 4, 2024
5f1cf83
fix tests
krehermann Jun 5, 2024
d63d25f
fix comments
krehermann Jun 5, 2024
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
2 changes: 1 addition & 1 deletion core/services/feeds/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ func Test_Service_ProposeJob(t *testing.T) {
// variables for workflow spec
wfID = "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef"
wfOwner = "00000000000000000000000000000000000000aa"
wfName = "my-workflow"
wfName = "myworkflow" // len 10
specYaml = `
triggers:
- id: "a-trigger"
Expand Down
33 changes: 24 additions & 9 deletions core/services/job/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package job

import (
"database/sql/driver"
"encoding/hex"
"encoding/json"
"fmt"
"strconv"
Expand Down Expand Up @@ -842,30 +843,44 @@ type LiquidityBalancerSpec struct {

type WorkflowSpec struct {
ID int32 `toml:"-"`
WorkflowID string `toml:"workflowId"`
WorkflowID string `toml:"workflowId"` // content hash
Copy link
Contributor

Choose a reason for hiding this comment

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

@krehermann If this is the content hash, can we rename it as such? I expected ID to be the fully qualified ID, i.e. containing the owner and whatever else we need to be sure it's actually unique

Also, should we be automatically generating the content hash from the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline, we're going to remove this comment and follow up later

Workflow string `toml:"workflow"`
WorkflowOwner string `toml:"workflowOwner"`
WorkflowName string `toml:"workflowName"`
WorkflowOwner string `toml:"workflowOwner"` // hex string representation of 20 bytes
WorkflowName string `toml:"workflowName"` // 10 byte plain text name
CreatedAt time.Time `toml:"-"`
UpdatedAt time.Time `toml:"-"`
}

var (
ErrInvalidWorkflowID = errors.New("invalid workflow id")
ErrInvalidWorkflowOwner = errors.New("invalid workflow owner")
ErrInvalidWorkflowName = errors.New("invalid workflow name")
)

const (
workflowIDLen = 64
workflowOwnerLen = 40
workflowIDLen = 64 // content hash
// owner and name are constrained the onchain representation i
workflowOwnerLen = 40 // hex string representation of 20 bytes
workflowNameLen = 10 // plain text name
)

// Validate checks the length of the workflow id, owner and name
// which are constrained by the onchain representation in [github.com/smartcontractkit/chainlink-common/blob/main/pkg/capabilities/consensus/ocr3/types/Metadata]
func (w *WorkflowSpec) Validate() error {
if len(w.WorkflowID) != workflowIDLen {
return fmt.Errorf("incorrect length for id %s: expected %d, got %d", w.WorkflowID, workflowIDLen, len(w.WorkflowID))
return fmt.Errorf("%w: incorrect length for id %s: expected %d, got %d", ErrInvalidWorkflowID, w.WorkflowID, workflowIDLen, len(w.WorkflowID))
}

_, err := hex.DecodeString(w.WorkflowOwner)
if err != nil {
return fmt.Errorf("%w: expected hex encoding got %s: %w", ErrInvalidWorkflowOwner, w.WorkflowOwner, err)
}
if len(w.WorkflowOwner) != workflowOwnerLen {
return fmt.Errorf("incorrect length for owner %s: expected %d, got %d", w.WorkflowOwner, workflowOwnerLen, len(w.WorkflowOwner))
return fmt.Errorf("%w: incorrect length for owner %s: expected %d, got %d", ErrInvalidWorkflowOwner, w.WorkflowOwner, workflowOwnerLen, len(w.WorkflowOwner))
}

if w.WorkflowName == "" {
return fmt.Errorf("workflow name is required")
if len(w.WorkflowName) != workflowNameLen {
return fmt.Errorf("%w: incorrect length for name %s: expected %d, got %d", ErrInvalidWorkflowName, w.WorkflowName, workflowNameLen, len(w.WorkflowName))
}

return nil
Expand Down
86 changes: 84 additions & 2 deletions core/services/job/models_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import (
"time"

"github.com/pelletier/go-toml/v2"
"github.com/stretchr/testify/require"
"gopkg.in/guregu/null.v4"

"github.com/smartcontractkit/chainlink-common/pkg/codec"
"github.com/smartcontractkit/chainlink-common/pkg/types"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gopkg.in/guregu/null.v4"

evmtypes "github.com/smartcontractkit/chainlink/v2/core/services/relay/evm/types"
"github.com/smartcontractkit/chainlink/v2/core/store/models"
)
Expand Down Expand Up @@ -268,3 +270,83 @@ func TestOCR2OracleSpec(t *testing.T) {
})
})
}

func TestWorkflowSpec_Validate(t *testing.T) {
type fields struct {
ID int32
WorkflowID string
Workflow string
WorkflowOwner string
WorkflowName string
CreatedAt time.Time
UpdatedAt time.Time
}
tests := []struct {
name string
fields fields
expectedErr error
}{
{
name: "valid",
fields: fields{
WorkflowID: "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef",
WorkflowOwner: "00000000000000000000000000000000000000aa",
WorkflowName: "ten bytes!",
},
},

{
name: "not hex owner",
fields: fields{
WorkflowID: "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef",
WorkflowOwner: "00000000000000000000000000000000000000az",
WorkflowName: "ten bytes!",
},
expectedErr: ErrInvalidWorkflowOwner,
},

{
name: "not len 40 owner",
fields: fields{
WorkflowID: "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef",
WorkflowOwner: "0000000000",
WorkflowName: "ten bytes!",
},
expectedErr: ErrInvalidWorkflowOwner,
},

{
name: "not len 10 name",
fields: fields{
WorkflowID: "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef",
WorkflowOwner: "00000000000000000000000000000000000000aa",
WorkflowName: "not ten bytes!",
},
expectedErr: ErrInvalidWorkflowName,
},
{
name: "not len 64 id",
fields: fields{
WorkflowID: "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f",
WorkflowOwner: "00000000000000000000000000000000000000aa",
WorkflowName: "ten bytes!",
},
expectedErr: ErrInvalidWorkflowID,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
w := &WorkflowSpec{
ID: tt.fields.ID,
WorkflowID: tt.fields.WorkflowID,
Workflow: tt.fields.Workflow,
WorkflowOwner: tt.fields.WorkflowOwner,
WorkflowName: tt.fields.WorkflowName,
CreatedAt: tt.fields.CreatedAt,
UpdatedAt: tt.fields.UpdatedAt,
}
err := w.Validate()
assert.ErrorIs(t, err, tt.expectedErr)
})
}
}
16 changes: 14 additions & 2 deletions core/services/workflows/delegate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type = "workflow"
schemaVersion = 1
workflowId = "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef"
workflowOwner = "00000000000000000000000000000000000000aa"
workflowName = "test"
workflowName = "ten bytes!"
`,
true,
},
Expand All @@ -43,12 +43,24 @@ schemaVersion = 1
false,
},
{
"missing name",
"invalid name length",
`
type = "workflow"
schemaVersion = 1
workflowId = "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef"
workflowOwner = "00000000000000000000000000000000000000aa"
workflowName = "not ten bytes"
`,
false,
},
{
"not a hex owner",
`
type = "workflow"
schemaVersion = 1
workflowId = "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef"
workflowOwner = "00000000000000000000000000000000000000aZ"
workflowName = "0123456789"
`,
false,
},
Expand Down
2 changes: 1 addition & 1 deletion core/web/jobs_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ func TestJobController_Create_HappyPath(t *testing.T) {
tomlTemplate: func(_ string) string {
id := "15c631d295ef5e32deb99a10ee6804bc4af1385568f9b3363f6552ac6dbb2cef"
owner := "00000000000000000000000000000000000000aa"
name := "my-test-workflow"
name := "myworkflow" // 10 bytes
workflow := `
triggers:
- id: "mercury-trigger"
Expand Down
Loading