Add integration test for GetAuthorizedEntries RPC #5356
Conversation
valverdethiago
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
valverdethiago
changed the title
…n and alias entry creation Signed-off-by: Thiago Valverde de Souza <[email protected]> d Please enter the commit message for your changes. Lines starting
valverdethiago
force-pushed
the
integration-test-for-GetAuthroizedEntries-RPC
branch
from
e684e4b
to
8a8168b
Compare
| srv.EnableEventDrivenCacheValidation() | ||
|
|
||
| // Wait for the cache to update | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
time.Sleep(2 * time.Second)
I don't know maintainers' tolerance for potential flake in tests :)
do we have another mechanism/preference vs sleep in tests?
There was a problem hiding this comment.
I think we should opt for exponential retry here which should be more reliable.
valverdethiago
force-pushed
the
integration-test-for-GetAuthroizedEntries-RPC
branch
from
8389b28
to
ae62b45
Compare
valverdethiago
force-pushed
the
integration-test-for-GetAuthroizedEntries-RPC
branch
from
da2cc4d
to
bced4a6
Compare
| exit 1 | ||
| else | ||
| echo "Entry with SPIFFE ID ${SPIFFE_ID} exists." | ||
| fi |
There was a problem hiding this comment.
- this test only ends with making sure that spire-server has the entries, but doesn't test GetAuthorizedEntries
- how is this test intended to be different from, for example, https://github.com/spiffe/spire/blob/main/test/integration/suites/fetch-x509-svids/04-create-registration-entries#L17 where we verify an agent receives its entries? are we planning to call GetAuthorizedEntries ourselves, eliminating spire-agent code from the test equation?
|
|
||
| log-debug "bootstrapping agent..." | ||
| docker compose exec -T spire-server \ | ||
| /opt/spire/bin/spire-server bundle show > conf/agent/bootstrap.crt |
There was a problem hiding this comment.
I didn't see other CI needing these setups you have here since shared creds already are checked in
|
|
||
| for ((m=1;m<=$SIZE;m++)); do | ||
| check-synced-entry "spire-agent" "spiffe://domain.test/workload-$m" | ||
| done |
There was a problem hiding this comment.
let's simplify CI and make each step distinct; checking in agent that entries are synced is separate from their creation
however, that probably also could just be a separate integration test (if doesn't already exist)? do we need the agent to come up at all for us to make the later GetAuthorizedEntries call?
| verify_registration_entry() { | ||
| local response | ||
|
|
||
| response=$(list_entries) |
There was a problem hiding this comment.
why do we need to verify the full list of entries to test GetAuthorizedEntries? that could be a separate integration test (if it doesn't already exist)
| # Prepare the test data | ||
| echo "Setting up test data..." | ||
| # (Include any commands to set up test data here) |
There was a problem hiding this comment.
| # Prepare the test data | |
| echo "Setting up test data..." | |
| # (Include any commands to set up test data here) |
we're not setting up test data here
| - Creates necessary registration entries for testing. | ||
|
|
||
| 3. **Assert Entities Creation (`03-assert-entities-created.sh`)** | ||
| - Creates necessary registration entries for testing. |
There was a problem hiding this comment.
| - Creates necessary registration entries for testing. | |
| - Checks necessary registration entries are retrieveable. |
|
please stop using force push :) it clears all comments out on the PR |
|
Based on previous contribute sync discussion, I think the approach is being revisited? I'll go ahead and close this out for now. Please feel free to re-open after revision. |
Pull Request check list
Affected functionality
N/A
Description of change
Add an integration test that ensures continued operation of the GetAuthorizedEntires RPC. This is scoped to:
Which issue this PR fixes
Fixes #4811