Support remote zilla configuration with change detection by attilakreiner · Pull Request #1071 · aklivity/zilla

attilakreiner · 2024-05-31T15:48:27Z

Description

This change moves away from URL to Path abstraction and introduces the http file system, so the config and the resources can be watched over file and http protocol and opens up the potential for further extension.

Fixes #1061

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/EngineConfigReader.java

...rc/main/java/io/aklivity/zilla/runtime/vault/filesystem/internal/FileSystemVaultHandler.java

...g-kafka/src/main/java/io/aklivity/zilla/runtime/binding/kafka/config/KafkaOptionsConfig.java

...ine/src/main/java/io/aklivity/zilla/runtime/engine/internal/watcher/EngineConfigWatcher.java

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/ResourceResolver.java

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

...e/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/registry/EngineManager.java

This reverts commit a370a7b.

jfallows · 2024-06-18T14:38:45Z

...ity/zilla/runtime/binding/openapi/asyncapi/internal/config/OpenapiAsyncapiBindingConfig.java


        this.composite = namespaceGenerator.generate(binding, openapis, asyncapis, openapiSchemaIdsByApiId::get);
-        this.composite.readURL = binding.readURL;
+        this.composite.readLocation = binding.readLocation;


Is it possible to get rid of composite.readURL / composite.readLocation entirely?

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/EngineConfiguration.java

jfallows · 2024-06-18T14:47:32Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/BindingConfig.java

    public transient long entryId;
    public transient ToLongFunction<String> resolveId;
-    public transient Function<String, String> readURL;
+    public transient Function<String, String> readLocation;


Is it possible to remove this completely?

jfallows · 2024-06-18T14:48:37Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/GuardConfig.java

 {
    public transient long id;
-    public transient Function<String, String> readURL;
+    public transient Function<Path, String> readPath;


Can this be removed now?

jfallows · 2024-06-18T14:48:59Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/NamespaceConfig.java

+
    public transient int id;
-    public transient Function<String, String> readURL;
+    public transient Function<String, String> readLocation;


Can we remove this one too?

Let's remove it.

jfallows · 2024-06-18T14:52:40Z

...ngine/src/main/java/io/aklivity/zilla/runtime/engine/internal/watcher/ConfigWatcherTask.java

+    public ConfigWatcherTask(
+        FileSystem fileSystem,
+        Function<String, EngineConfig> configChangeListener,
+        Function<Path, String> readPath)


Can we remove readPath and use Files.readString(path) directly instead?

jfallows · 2024-06-18T14:54:55Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/internal/watcher/WatchedItem.java

+            String scheme = watchedPath.getFileSystem().provider().getScheme();
+            if ("file".equals(scheme))
+            {
+                registerFilePath();
+            }
+            else if ("http".equals(scheme))
+            {
+                watchKeys.add(watchedPath.register(watchService));
+            }


Is this more a question of optimization than specific schemes?

There is different behaviour here.

In case of the file filesystem we want to watch files, but actually we can only watch directories. But our file can be a symlink or the directory can be a symlink or its parent etc... So Barnabas created a very sophisticated logic to cover all those cases, which means in a generic case we have to watch more than one object (directory) so we can make sure we are watching the changes of our file.

In case of http filesystem we just watch a URL and that's it. So it's a 1:1 mapping hence just one line of code here.

jfallows · 2024-06-18T14:57:17Z

...me/guard-jwt/src/main/java/io/aklivity/zilla/runtime/guard/jwt/internal/JwtGuardContext.java

    {
        JwtOptionsConfig options = (JwtOptionsConfig) guard.options;
-        JwtGuardHandler handler = new JwtGuardHandler(options, context, supplyAuthorizedId, guard.readURL);
+        JwtGuardHandler handler = new JwtGuardHandler(options, context, supplyAuthorizedId, guard.readPath);


Can JwtGuardHandler use EngineContext.readPath(...) instead of guard.readPath?

...me/guard-jwt/src/main/java/io/aklivity/zilla/runtime/guard/jwt/internal/JwtGuardHandler.java

...m/src/test/java/io/aklivity/zilla/runtime/vault/filesystem/internal/FileSystemVaultTest.java

jfallows · 2024-06-24T18:14:41Z

...ngine/src/main/java/io/aklivity/zilla/runtime/engine/internal/watcher/ConfigWatcherTask.java

+        WatchService watchService = null;
+        if (!"jar".equals(fileSystem.provider().getScheme())) // we can't watch in jar fs
+        {
+            try
+            {
+                watchService = fileSystem.newWatchService();
+            }
+            catch (Exception ex)
+            {
+                rethrowUnchecked(ex);
+            }
+        }
+        this.watchService = watchService;


The idea of using a FileSystem abstraction is to move away from implementation specific knowledge of the filesystem as much as is practical in the engine.

The FileSystem.newWatchService() method is specified as an optional method, throwing UnsupportedOperationException on implementations not supporting it.

So we should be tolerant of that both now for jar scheme (used during tests) and later for other filesystems that also may not support newWatchService(), without needing to mention jar explicitly.

Suggested change

WatchService watchService = null;

if (!"jar".equals(fileSystem.provider().getScheme())) // we can't watch in jar fs

{

try

{

watchService = fileSystem.newWatchService();

}

catch (Exception ex)

{

rethrowUnchecked(ex);

}

}

this.watchService = watchService;

this.watchService = newWatchService(fileSystem);

private static WatchService newWatchService( FileSystem fileSystem) { WatchService watcher = null; try { watcher = fileSystem.newWatchService(); } catch (UnsupportedOperationException ex) { // no watcher } catch (Exception ex) { rethrowUnchecked(ex); } return watcher; }

jfallows · 2024-06-24T18:23:23Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

+    private String readLocation(
+        String location)
+    {
+        return readPath(resolvePath(location));


Let's remove readLocation everywhere, and use Files.readString(path) instead at the call sites.

We should no longer need BindingConfig.readLocation directly given that EngineWorker.resolvePath(location) is available to all handlers, so BindingConfig.readLocation can be removed as well.

jfallows · 2024-06-24T18:25:05Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

+    public Path resolvePath(
+        String location)
+    {
+        return configPath.resolveSibling(location);
+    }


This method implementation should move to EngineWorker instead of passing a lambda from Engine.

Note: EngineWorker already has EngineConfig and can call config.configPath() in EngineWorker constructor instead of passing as an explicit constructor parameter.

jfallows · 2024-06-24T18:27:14Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

+            this.configPath,
+            this::readPath,
+            this::readLocation);


None of these need to be passed to EngineManager constructor.

Only configPath is needed and can be obtained from EngineConfiguration (already passed).

jfallows · 2024-06-24T18:28:40Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

+        catch (Exception ex)
        {
-            output = "";
+            result = "";


I think we don't want to default to empty string here.

After moving Files.readString(path) to each call site, handling an IOException will let the caller decide the best remedy instead of assuming empty string is always appropriate.

jfallows · 2024-06-24T19:02:31Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

        {
            EngineWorker worker =
-                new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, bindings, exporters,
+                new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, this::resolvePath, bindings, exporters,


Suggested change

new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, this::resolvePath, bindings, exporters,

new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, bindings, exporters,

Move resolvePath to EngineWorker implementation instead.

jfallows · 2024-06-24T19:03:22Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/Engine.java

        final Map<String, Guard> guardsByType = guards.stream()
            .collect(Collectors.toMap(g -> g.name(), g -> g));

+        this.configPath = config.configPath();


Looks like configPath is no longer needed as Engine field after resovlePath(...) method is moved to EngineWorker implementation directly.

jfallows · 2024-06-24T19:12:48Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/EngineConfiguration.java

+    public Path configPath()
+    {
+        return ENGINE_CONFIG_PATH.get(this);
+    }
+


This configPath() method looks good.

Let's keep the ENGINE_CONFIG_URL constant in place for now, to support zilla.properties defining zilla.engine.config.url property, but remove all calls to EngineConfiguration.configURL() from the code, and replace with corresponding EngineConfiguration.configPath() method calls instead.

jfallows · 2024-06-24T19:13:21Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/NamespaceConfig.java

+
    public transient int id;
-    public transient Function<String, String> readURL;
+    public transient Function<String, String> readLocation;


Let's remove it.

jfallows · 2024-06-24T19:21:28Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/NamespaceConfig.java

+    private List<String> resolveResources()
+    {
+        List<String> resources = new LinkedList<>();
+        for (CatalogConfig catalog : catalogs)
+        {
+            if (FILESYSTEM.equals(catalog.type))
+            {
+                resources.addAll(catalog.options.resources);
+            }
+        }
+        for (VaultConfig vault : vaults)
+        {
+            if (FILESYSTEM.equals(vault.type))
+            {
+                resources.addAll(vault.options.resources);
+            }
+        }
+        return resources;


This approach assumes global knowledge of all possible implementations and assumes their behavior, such that a catalog or vault other than filesystem cannot contribute resources to be watched.

We need a more general strategy instead.

Please include all resources from any binding, guard, vault, catalog or telemetry exporter.

Note: instead of calling a non-static method from the constructor that relies on constructor field assignment, please make this method static and pass all required state as parameters, such as bindings, guards, vaults, catalogs, and telemetry.

Remove GuardedConfig.readPath Rename ConfigAdapterContext.readLocation to ConfigAdapterContext.readResource Deprecate ConfigAdapterContext for removal Add EngineConfiguration.configURI() to resolve absolute file path Remove EngineConfiguration.configURL() but use to default EngineConfiguration.configURI() Simplify EngineManager Gather resources on NamespaceConfig from member configs Consolidate config and resource watcher as EngineConfigWatcher Resolve watched paths based on path filesystem provider scheme Configure HttpFileSystem poll internval duration via Map<String,?> env Simplify ReconfigureHttpIT scripts and include /zilla.yaml in request path HttpFileSystem per origin (root path) not per individual path Track HttpPath change count vs read count to implement simple caching

...em-http/src/main/java/io/aklivity/zilla/runtime/filesystem/http/internal/HttpFileSystem.java

…wait

attilakreiner force-pushed the config-reload branch from b6efc03 to d9601d8 Compare May 31, 2024 15:50

github-advanced-security bot found potential problems May 31, 2024

View reviewed changes

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/config/EngineConfigReader.java Fixed Show fixed Hide fixed

WIP reconfigure only changed namespaces

a370a7b

attilakreiner force-pushed the config-reload branch 2 times, most recently from 348e69f to e1d5642 Compare June 5, 2024 07:46

attilakreiner added 2 commits June 5, 2024 14:51

WIP watch resources

3757300

WIP refactoring watch resources

c02e5c4

attilakreiner force-pushed the config-reload branch from e1d5642 to c02e5c4 Compare June 5, 2024 15:34

attilakreiner changed the title ~~WIP~~ WIP Support remote zilla configuration with change detection Jun 5, 2024

WIP refactoring 2

8fcae70

jfallows requested changes Jun 5, 2024

View reviewed changes

...rc/main/java/io/aklivity/zilla/runtime/vault/filesystem/internal/FileSystemVaultHandler.java Outdated Show resolved Hide resolved

attilakreiner added 4 commits June 6, 2024 13:55

WIP fix

96f070e

WIP EngineConfigWatcher

df0b998

WIP ResourceResolver

c0a554f

WIP fix ResourceResolver

b726267

jfallows requested changes Jun 7, 2024

View reviewed changes

attilakreiner added 10 commits June 10, 2024 11:01

fix indentation

9c2cbbe

fix EngineManager method names

abb923f

rm ResourceResolver

b9355c6

WIP EngineManager addResources

c58ebc9

Revert "WIP reconfigure only changed namespaces"

2285123

This reverts commit a370a7b.

fix EngineConfigWatcher addResources

39f34d0

Merge branch 'develop' into config-reload

b482fac

Merge branch 'develop' into config-reload

fd17488

Merge branch 'develop' into config-reload

b6c2ff2

WIP readPath resolvePath readLocation

2da3603

jfallows requested changes Jun 18, 2024

View reviewed changes

attilakreiner added 3 commits June 18, 2024 19:55

WIP filesystem-http

c85bdc1

WIP

ab94dac

fix

22a2d03

attilakreiner added 18 commits June 20, 2024 11:34

WIP hfs uri

2a217f8

WIP hfs resolveSibling

4f5737a

WIP hfs rm timeout

6395780

WIP hfs revert send

54a2b87

WIP hfs HP readBody resolveBody

a5d0a39

WIP hfs HP readBody etag

f19e623

WIP hfs HP

a8a4015

WIP hfs ignore AppIT

c2efe21

WIP hfs fix AppIT

873b5e1

WIP hfs watchBody 1

c166431

WIP hfs watchBody 2

0c0f7f4

WIP hfs watchBody 3

31b420c

WIP hfs watchBody 4

d3109f7

WIP hfs watchBody 5

3198b10

WIP hfs watchBody 6

ef2d377

WIP hfs getPath

8f982fe

WIP hfs watchBody 7

50c8792

WIP fix

71231a9

jfallows requested changes Jun 24, 2024

View reviewed changes

github-advanced-security bot found potential problems Jun 26, 2024

View reviewed changes

...em-http/src/main/java/io/aklivity/zilla/runtime/filesystem/http/internal/HttpFileSystem.java Fixed Show fixed Hide fixed

No watch event needed for identical response body

e7c6c56

jfallows changed the title ~~WIP Support remote zilla configuration with change detection~~ Support remote zilla configuration with change detection Jun 26, 2024

jfallows marked this pull request as ready for review June 26, 2024 04:41

jfallows previously approved these changes Jun 26, 2024

View reviewed changes

Handle status 204 with null body and infer delay for optional prefer …

5423718

…wait

jfallows dismissed their stale review via 5423718 June 26, 2024 05:35

jfallows approved these changes Jun 26, 2024

View reviewed changes

jfallows merged commit 230eec2 into aklivity:develop Jun 26, 2024

attilakreiner deleted the config-reload branch July 12, 2024 09:34

	new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, this::resolvePath, bindings, exporters,
	new EngineWorker(config, tasks, labels, errorHandler, tuning::affinity, bindings, exporters,

Conversation

attilakreiner commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attilakreiner Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

attilakreiner commented May 31, 2024 •

edited

Loading

attilakreiner Jun 18, 2024 •

edited

Loading