deployment/cre/jobs: update VerifyCRESettings to check for prefixes & lowercase #20702
Conversation
product-security-plaid-production
bot
requested review from
agparadiso,
ajaskolski,
george-dorin and
gustavogama-cll
jmank88
force-pushed
the
CRE-1612-validate-cre-settings-keys
branch
from
2ab6ebc to
68802b3
Compare
CORA - Pending ReviewersAll codeowners have approved! ✅ Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
I see you updated files related to
|
deployment/cre/jobs/settings.go
Outdated
| // TODO to be enforced after upgrading deployed nodes and settings | ||
| //if strings.HasPrefix(id, "org_") { | ||
| // errs = errors.Join(errs, fmt.Errorf("invalid org id %s: must not be prefixed org_", id)) | ||
| //} else |
There was a problem hiding this comment.
We have to allow org prefix (and configure duplicate settings) until we are only running the new code.
| } else if strings.HasPrefix(id, "0x") { | ||
| errs = errors.Join(errs, fmt.Errorf("invalid owner id %s: must not be prefixed 0x", id)) | ||
| } else if strings.ToLower(id) != id { | ||
| errs = errors.Join(errs, fmt.Errorf("invalid owner id %s: must be lower case", id)) |
There was a problem hiding this comment.
Can't we simply require N hex characters? (where N is different for org, workflow and owner)
There was a problem hiding this comment.
The context values are being normalized to lowercase, so we need to ensure that we only use lowercase here too, since they are just treated as string keys for settings/limits, not decoded to raw bytes.
![trunk-io[bot]](https://avatars.githubusercontent.com/in/120106?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jmank88
force-pushed
the
CRE-1612-validate-cre-settings-keys
branch
from
68802b3 to
a7fa630
Compare
jmank88
force-pushed
the
CRE-1612-validate-cre-settings-keys
branch
from
a7fa630 to
1540618
Compare
|
There was a problem hiding this comment.
🔴 Test Results: Unrelated Failure
Affected failures:
- Workflow Run: Integration Tests
What Broke
This failure appears to be unrelated to the changes in this PR. The CI failure is caused by newly introduced validation logic in deployment/cre/jobs/settings.go or an unexpected interaction with the updated test case in deployment/cre/jobs/propose_job_spec_test.go. The PR introduces new validation rules in deployment/cre/jobs/settings.go for CRE settings IDs, requiring them to be lowercase and without specific prefixes. Although the test deployment/cre/jobs/propose_job_spec_test.go was updated to align with some of these changes, the generic 'exit 1' error in the CI logs strongly suggests an unhandled validation failure or an incompatibility within the updated test case.
Autofix Options
You can use our MCP server to get AI assistance with debugging and fixing these failures.
- Use MCP in your IDE to debug the issue. Try
Help me fix CI failures from rhHp74eTto get started.
3 participants
https://smartcontract-it.atlassian.net/browse/CRE-1612
Requires:
Supports: