Skip to content

Dynamic federator remotes#3260

Merged
fisx merged 186 commits intowireapp:developfrom
lepsa:FS-1115
Jun 27, 2023
Merged

Dynamic federator remotes#3260
fisx merged 186 commits intowireapp:developfrom
lepsa:FS-1115

Conversation

@fisx
Copy link
Contributor

@fisx fisx commented May 1, 2023

https://wearezeta.atlassian.net/browse/FS-1115

internal docs:

http://localhost:8082/api-internal/swagger-ui/brig/

2023-05-19-113146_1920x1080_scrot

docs for devs:

2023-06-04-143744_1600x900_scrot

(slightly outdated) docs for operators:

2023-06-04-143310_1600x900_scrot
2023-06-04-143317_1600x900_scrot
2023-06-04-143320_1600x900_scrot
2023-06-04-143325_1600x900_scrot
2023-06-04-143335_1600x900_scrot

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

TODO

  • double-check what our default search policy in (a) the case where we allow all domains and a new one is not listed, and (b) the case where we have an allow entry in the cassandra table, but it has no search policy. also check if we're getting it right in the docs. (i think (a) should be no_search and (b) should be no NULL allowed in database field, just pick something, it's not that hard.) maybe write integration tests?
  • log 'F' are sometimes too harsh. 'I' or 'D' would do. eg., for "couldn't pull data from brig yet, retrying".
  • yeah, galley integration tests where hanging for me as well, but the last 3 rounds everything went green ok.
  • Dynamic federator remotes #3260 (review)

@fisx fisx mentioned this pull request May 1, 2023
2 tasks
@battermann
Copy link
Contributor

This is just a comment, and there is no need to take action. Just to document what I have tested.

setFederationStrategy in brig.integration.yaml should be set to allowAll which matches the old setting in federator before this PR.

If we change setFederationStrategy to allowDynamic and add example.com and b.example.com to the allow list, a couple of integration tests fail. The reason is that some tests that expect 400 federation-denied now get 422 invalid-domain. Since the exact same thing happens when changing the old setting in federator to allowDomains this is expected and absolutely fine.

The errors are not listed in the specific endpoint descriptions in the swagger, but they are all listed as federation errors in the overall API descriptions. So clients should be aware and prepared to handle them all.

All looking good 👍

# Disable one ore more API versions. Please make sure the configuration value is the same in all these charts:
# brig, cannon, cargohold, galley, gundeck, proxy, spar.
# setDisabledAPIVersions: [ v3 ]
setFederationStrategy: allowNone
Copy link
Contributor

Choose a reason for hiding this comment

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

The old setting for cloud was:

    federationStrategy:
      allowedDomains: []

Should we set it to allowDynamic with an empty list here, too, to exactly match the old behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the two are different in behavior if the database table is non-empty. so i'd argue allowNone is keeping behavior as unchanged as can be.

with other services) we hope to get away with the simple solution and
always read from cassandra directly.

(More details to be added?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add details or remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair point. but i'm not going to spend another day beating concourse for this change :-P

@fisx
Copy link
Contributor Author

fisx commented Jun 26, 2023

ready to merge! :shipit:

@fisx fisx merged commit d47d274 into wireapp:develop Jun 27, 2023
@fisx fisx mentioned this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants