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.

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

Validation for required attributes #865

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Validation for required attributes #865

wants to merge 3 commits into from

Conversation

nanchano
Copy link

Rules:

  1. CSA key can't be empty
  2. CSA Signature can't be empty
  3. Data Schema can't be empty
  4. Domain can't be empty
  5. Set up Timestamp automatically

}
}
return m
}

func NewMetadata(attrs Attributes) *Metadata {
m := &Metadata{}
m := &Metadata{Timestamp: time.Now().UTC()}
Copy link
Contributor

Choose a reason for hiding this comment

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

@nanchano, We should try not to use attributes with unbounded cardinality.
It can cause various problems see this thread for details
https://chainlink-core.slack.com/archives/C06TEMFRPQD/p1729082258236499

Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong, but the unbounded cardinality issue is a problem for metrics, not for custom events, right?

Copy link
Author

Choose a reason for hiding this comment

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

How would we wanna represent a timestamp then? As a string and let clients parse it?

@@ -22,9 +23,9 @@ type Metadata struct {
// The version of the CL node.
NodeVersion string
// mTLS public key for the node operator. This is used as an identity key but with the added benefit of being able to provide signatures.
NodeCsaKey string
NodeCsaKey string `validate:"required"`
Copy link
Contributor

@pkcll pkcll Oct 18, 2024

Choose a reason for hiding this comment

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

@4of9, are these going to be added as resource attributes to beholder config (since they are static) or we want to pass them as regular attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinkin NodeCsaKey would be included as a resource attribute so users don't have to add it to every message

"beholder_data_schema": "/schemas/ids/1001", // Required field, URI
"beholder_data_schema": "/schemas/ids/1001", // Required field, URI
"node_csa_key": "node_csa_val", // Required
"node_csa_signature": "mode_csa_signature_val", // Required
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this would be an attribute, the signature will be coming through the authentication headers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants