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

Define separate fallback RPC argument to work around URL escaping issues #777

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

pschork
Copy link
Contributor

@pschork pschork commented Sep 30, 2024

This change adds a optional fallback RPC as a separate argument to isolate primary from secondary RPC and avoid dealing with shell escaping issues with concatenating multiple URLs into a single argument.

The required CHAIN_RPC flag still supports multiple RPC urls via string slice, but with this change recommendation is the use single RPC URL (as primary) and specify fallback via CHAIN_RPC_FALLBACK, which gets appended to the EthClient RPC list.

Why are these changes needed?

Updating secrets needs to be high confidence and simple. Engineers should not need to worry about shell escaping or urlencoding api keys in a string slice arg.

Using github secrets with multiple RPCs concatenated into one is also problematic/error prone for updates.

Background: Our ankr enterprise RPC url includes a query param ?api=xxx which breaks helm diff when used in a string slice.

Executing helm template dataapi . --namespace dataapi --set dataapiDefaults.config.DATA_ACCESS_API_CHAIN_RPC=https://enterprise.onerpc.com/eth_holesky?apikey=xxxyz123,https://virulent-capable-flower.ethereum-holesky.quiknode.pro/xxxb0215f2a6/ --values values/eigenda-testnet/us-east-1/holesky/values.yaml --validate --is-upgrade --dry-run=client
Error: Failed to render chart: exit status 1: install.go:224: 2024-09-30 13:48:12.210677 -0700 PDT m=+0.018882085 [debug] Original chart version: ""
install.go:241: 2024-09-30 13:48:12.211159 -0700 PDT m=+0.019364918 [debug] CHART PATH: /Users/pschork/workspace/eigenda-devops/charts/dataapi

Error: failed parsing --set data: key "pro/xxxb0215f2a6/" has no value

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@pschork pschork requested review from dmanc and bxue-l2 September 30, 2024 21:20
@pschork pschork force-pushed the pschork/fallback_rpc branch 3 times, most recently from 5a3f2e2 to 11e06bf Compare October 1, 2024 00:36
@pschork pschork force-pushed the pschork/fallback_rpc branch from 11e06bf to c296a10 Compare October 1, 2024 00:50
@pschork pschork merged commit e44ba45 into master Oct 2, 2024
9 checks passed
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.

2 participants