-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Vault DON: e2e testing with auth enabled #19543
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
Conversation
… into vault_e2e_auth
… into vault_e2e_auth
|
I see you updated files related to
|
|
| getPeerID: getPeerID, | ||
| srvs: srvcs, | ||
| workflowRegistrySyncer: workflowRegistrySyncer, | ||
| orgResolver: orgResolver, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this line removed intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, seems like caused during merge conflicts resolution. Will fix it asap
| } else if req.Method == vaulttypes.MethodSecretsGet { | ||
| ar := h.newActiveRequest(req, callback) | ||
| return h.handleSecretsGet(ctx, ar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? Could use a comment for why Get differs from other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not sure I understand this bit either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get is not exposed to users. It is only there on dev builds for e2e testing. So no point doing auth for this method.
Once we write different tests for workflows fetching and decrypting secrets, we can remove Get entirely from gateway.
I will add a comment here.
| // Async initialization of contract reader because there is an on-chain | ||
| // call dependency. Blocking on initialization results in a | ||
| // deadlock. Instead wait until the node has identified it's DON | ||
| // as a proxy for a DON and on-chain ready state . | ||
| reader, err := w.newWorkflowRegistryContractReader(ctx) | ||
| if err != nil { | ||
| w.lggr.Errorw("contract reader unavailable", "error", err.Error()) | ||
| break | ||
| } | ||
| w.contractReader = reader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this comment correctly, then the error might occur even when everything is fine (we are waiting for the on-chain state to be ready). In that case, the log should be a debug or info log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, error level seems too much. Will update it.
| require.NoError(t, err, "failed to allowlist request") | ||
|
|
||
| framework.L.Info().Msgf("Allowlisting request digest at contract %s, for owner: %s, digestHexStr: %s", wfRegistryContract.Address().Hex(), owner, hex.EncodeToString(digest[:])) | ||
| time.Sleep(5 * time.Second) // wait a bit to ensure the allowlist is propagated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to mine/advance the block instead of sleeping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So some more context is that the sleep is also there to allow the vault don and gateway to sync this list using workflow registry syncer.
If we quickly send a request after allowlisting, it might fail because vault don/gateway haven't yet fetched the latest list.
|
Enabled merge to unblock other PRs |




Uh oh!
There was an error while loading. Please reload this page.