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.

Sign up for GitHub

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

Support config sub-values for allowedHosts #21950

Merged
merged 3 commits into from
Jan 27, 2023
Merged

Support config sub-values for allowedHosts #21950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b0abfa
Support config sub-values for `allowedHosts`
evantahler Jan 26, 2023
d239507
Support numeric values
evantahler Jan 27, 2023
147ac5d
smarter support for empty allowedHosts
evantahler Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions airbyte-config/init/src/main/resources/seed/source_definitions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,7 @@
allowedHosts:
hosts:
- "${host}"
- "${tunnel_method.tunnel_host}"
Comment on lines 1349 to +1352
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @bleonard / @prateekmukhedkar - this should be the complete set of needs for source-postgres!

Copy link
Contributor

Choose a reason for hiding this comment

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

@evantahler I am working on adding allowedHosts to the remaining database sources(link). Reading through this PR, it appears that ${tunnel_method.tunnel_host} is only required for those sources that support connection via SSH tunnels. (for example as of now we don't support connecting via ssh for source-redshift). And for all such sources I will only include ${host} sub-value under allowedHosts. Does that sound correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

- name: Postmark App
sourceDefinitionId: cde75ca1-1e28-4a0f-85bb-90c546de9f1f
dockerRepository: airbyte/source-postmarkapp
Expand Down
41 changes: 34 additions & 7 deletions airbyte-workers/src/main/java/io/airbyte/workers/utils/ConfigReplacer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,35 @@ public AllowedHosts getAllowedHosts(AllowedHosts allowedHosts, JsonNode config)
final List<String> resolvedHosts = new ArrayList<>();
final Map<String, String> valuesMap = new HashMap<>();
final JsonParser jsonParser = config.traverse();

List<String> prefixes = new ArrayList<>();
while (!jsonParser.isClosed()) {
if (jsonParser.nextToken() == JsonToken.FIELD_NAME) {
final JsonToken type = jsonParser.nextToken();
if (type == JsonToken.FIELD_NAME) {
final String key = jsonParser.getCurrentName();
if (config.get(key) != null) {
valuesMap.put(key, config.get(key).textValue());
// the interface for allowedHosts is dot notation, e.g. `"${tunnel_method.tunnel_host}"`
final String fullKey = (prefixes.isEmpty() ? "" : String.join(".", prefixes) + ".") + key;
// the search path for JSON nodes is slash notation, e.g. `"/tunnel_method/tunnel_host"`
final String lookupKey = "/" + (prefixes.isEmpty() ? "" : String.join("/", prefixes) + "/") + key;

String value = config.at(lookupKey).textValue();
if (value == null) {
final Number numberValue = config.at(lookupKey).numberValue();
if (numberValue != null) {
value = numberValue.toString();
}
}

if (value != null) {
valuesMap.put(fullKey, value);
}
} else if (type == JsonToken.START_OBJECT) {
if (jsonParser.getCurrentName() != null) {
prefixes.add(jsonParser.getCurrentName());
}
} else if (type == JsonToken.END_OBJECT) {
if (!prefixes.isEmpty()) {
prefixes.remove(prefixes.size() - 1);
}
}
}
Expand All @@ -55,11 +79,14 @@ public AllowedHosts getAllowedHosts(AllowedHosts allowedHosts, JsonNode config)
final List<String> hosts = allowedHosts.getHosts();
for (String host : hosts) {
final String replacedString = sub.replace(host);
if (replacedString.contains("${")) {
this.logger.error(
"The allowedHost value, '" + host + "', is expecting an interpolation value from the connector's configuration, but none is present");
if (!replacedString.contains("${")) {
resolvedHosts.add(replacedString);
}
resolvedHosts.add(replacedString);
}

if (resolvedHosts.isEmpty() && !hosts.isEmpty()) {
this.logger.error(
"All allowedHosts values are un-replaced. Check this connector's configuration or actor definition - " + allowedHosts.getHosts());
}

final AllowedHosts resolvedAllowedHosts = new AllowedHosts();
Expand Down
38 changes: 37 additions & 1 deletion airbyte-workers/src/test/java/io/airbyte/workers/utils/ConfigReplacerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,58 @@ class ConfigReplacerTest {
final ObjectMapper mapper = new ObjectMapper();

@Test
@SuppressWarnings("PMD.AvoidUsingHardCodedIP")
void getAllowedHostsGeneralTest() throws IOException {
final AllowedHosts allowedHosts = new AllowedHosts();
final List<String> hosts = new ArrayList();
hosts.add("localhost");
hosts.add("static-site.com");
hosts.add("${host}");
hosts.add("${number}");
hosts.add("${subdomain}.vendor.com");
hosts.add("${tunnel_method.tunnel_host}");
allowedHosts.setHosts(hosts);

final List<String> expected = new ArrayList<>();
expected.add("localhost");
expected.add("static-site.com");
expected.add("foo.com");
expected.add("123");
expected.add("account.vendor.com");
expected.add("1.2.3.4");

final String configJson = "{\"host\": \"foo.com\", \"subdomain\": \"account\", \"password\": \"abc123\"}";
final String configJson =
"{\"host\": \"foo.com\", \"number\": 123, \"subdomain\": \"account\", \"password\": \"abc123\", \"tunnel_method\": {\"tunnel_host\": \"1.2.3.4\"}}";
final JsonNode config = mapper.readValue(configJson, JsonNode.class);
final AllowedHosts response = replacer.getAllowedHosts(allowedHosts, config);

assertThat(response.getHosts()).isEqualTo(expected);
}

@Test
void getAllowedHostsNestingTest() throws IOException {
final AllowedHosts allowedHosts = new AllowedHosts();
final List<String> hosts = new ArrayList();
hosts.add("value-${a.b.c.d}");
allowedHosts.setHosts(hosts);

final List<String> expected = new ArrayList<>();
expected.add("value-100");

final String configJson = "{\"a\": {\"b\": {\"c\": {\"d\": 100 }}}, \"array\": [1,2,3]}";
final JsonNode config = mapper.readValue(configJson, JsonNode.class);
final AllowedHosts response = replacer.getAllowedHosts(allowedHosts, config);

assertThat(response.getHosts()).isEqualTo(expected);
}

@Test
void ensureEmptyArrayRemains() throws IOException {
final AllowedHosts allowedHosts = new AllowedHosts();
allowedHosts.setHosts(new ArrayList());
final List<String> expected = new ArrayList<>();

final String configJson = "{}";
final JsonNode config = mapper.readValue(configJson, JsonNode.class);
final AllowedHosts response = replacer.getAllowedHosts(allowedHosts, config);

Expand Down