process config control messages during check and discover
#20894
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…onous-job-control-msg
pedroslopez
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pedroslopez
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pedroslopez
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pedroslopez
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pedroslopez
force-pushed
the
pedroslopez/synchronous-job-control-msg
branch
from
60987e2 to
b526aac
Compare
octavia-squidington-iv
removed
area/worker
octavia-squidington-iv
added
area/api
pedroslopez
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pedroslopez
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
jbfbell
pushed a commit
that referenced
this pull request
Jan 13, 2023
* track latest config message * pass new config as part of outputs * persist new config * persist config as the messages come through, dont set output * clean up old implementation * accept control messages for destinations * get api client from micronaut * mask instance-wide oauth params when updating configs * defaultreplicationworker tests * formatting * tests for source/destination handlers * rm todo * refactor test a bit to fix pmd * fix pmd * fix test * add PersistConfigHelperTest * update message tracker comment * fix pmd * format * move ApiClientBeanFactory to commons-worker, use in container-orchestrator * pull out config updating to separate methods * add jitter * rename PersistConfigHelper -> UpdateConnectorConfigHelper, docs * fix exception type * fmt * move message type check into runnable * formatting * pass api client env vars to container orchestrator * pass micronaut envs to container orchestrator * print stacktrace for debugging * different api host for container orchestrator * fix default env var * format * fix errors after merge * set source and destination actor id as part of the sync input * fix: get destination definition * fix null ptr * remove "actor" from naming * fix missing change from rename * revert ContainerOrchestratorConfigBeanFactory changes * inject sourceapi/destinationapi directly rather than airbyteapiclient * UpdateConnectorConfigHelper -> ConnectorConfigUpdater * rm log * fix test * dont fail on config update error * process control messages for discover jobs * process control messages for CHECK * persist config updates on check_connection_for_update * get last config message rather than first * fix pmd * fix failing tests * add tests * source id not required for check connection (create case) * suppress pmd warning for BusyWait literal * source id not required for checkc onnection (create case) (p2) * pass id, not full config to runnables/accept control message * add new config required for api client * add test file * remove debugging logs * rename method (getLast -> getMostRecent) * rm version check (re-added this in by mistake on merge) * fix test compatibility * simplify
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Process config control messages in
checkanddiscovercalls to support connectors with short-lived/single-use refresh tokens.closes #20145
How
Call ConnectorConfigUpdater introduced in feat(Platform): update actor configuration when receiving control messages from connectors during sync #19811 from
DefaultCheckConnectionWorker/DefaultDiscoverCatalogWorkerto update the connector config if a config control message has been found.discover, use the existingsourceIdfrom the input.check, addactorTypeandactorIdto the input so we can call updateSource/updateDestination accordingly.In order to support refreshing tokens during
check_connection_for_update,sourceIdanddestinationIdare now being passed through. (see caviat in "User Impact" section)🚨 User Impact / Known Issues 🚨
Whenever a connector emits a config control message during
checkordiscover, the airbyte platform will now persist the updated configuration.Configuration updates via UI
There's an interesting side effect here that can happen when users are updating configuration via the UI. Before saving the new configuration, we perform a
checkon the configuration. If thecheckfails, we do not persist the user's changes. After this PR, if the connector issues a token refresh during that preliminarycheck, we will persist the new configuration. This means that a failed check may now cause your config to be updated with invalid values.The following diagram illustrates the happy path of CHECK succeeding:
The above scenario works as expected and the updated config + new tokens emitted during
CHECKare saved. Note that we are assuming that the connector updatedsecretfields here that have not been modified by the user in the form. Non-secret field values would be overwritten by the user's provided values in the laterupdatecall (see #20913)Here's what happens in the case of CHECK failing or throwing an error:
Because we want to save the new token values even on a failed check (since old tokens are invalidated), this creates a side effect of updating the configuration with the values that led to the failed check. This means that if you had a working connector before but started making some changes to the configuration that rendered it invalid, now your connector isn't properly configured. Before this PR, changes leading to a failed check would just not be persisted and your previously working configuration would still be saved. However, if we didn't do this then every failed check would need a manual re-authentication.
Connector creation with eager token refreshes
When first setting up a connector, the
checkcall happens before the connector has been saved in the database. This means that we can't update the configuration at this phase. If the connector performs a token refresh and invalidates the old tokens, we have no way of saving these currently. See https://github.com/airbytehq/airbyte-internal-issues/issues/1260 for more details.