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.

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

Prepare changes for config version 2 #147

Merged
merged 15 commits into from
Oct 27, 2023
Merged

Prepare changes for config version 2 #147

merged 15 commits into from
Oct 27, 2023

Conversation

carabasdaniel
Copy link
Contributor

No description provided.

@github-actions
Copy link

Pull Request Test Coverage Report for Build 6629312535

  • 185 of 294 (62.93%) changed or added relevant lines in 11 files are covered.
  • 10 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.4%) to 41.839%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/app/edgedir.go 2 3 66.67%
pkg/cc/config/config.go 3 4 75.0%
pkg/testing/client.go 2 3 66.67%
plugins/edge/sync.go 0 2 0.0%
pkg/app/console.go 0 3 0.0%
pkg/app/ui/handler.go 0 10 0.0%
pkg/app/authorizer.go 37 52 71.15%
pkg/app/topaz.go 127 203 62.56%
Files with Coverage Reduction New Missed Lines %
pkg/cc/config/config.go 1 75.0%
pkg/app/ui/handler.go 2 0.0%
pkg/app/topaz.go 3 61.06%
pkg/app/authorizer.go 4 68.75%
Totals Coverage Status
Change from base Build 6615738224: 0.4%
Covered Lines: 1415
Relevant Lines: 3382

💛 - Coveralls

@gertd
Copy link
Member

gertd commented Oct 24, 2023

@carabasdaniel

I took this PR together with the service-host PR (aserto-dev/service-host#6) for a spin.

A couple of observations:

  • It seems we check the config version too late, when starting topaz pointing to an old config

It first fails with:

2023/10/24 13:02:49 failed to create new config: failed to unmarshal config file: 1 error(s) decoding:

* 'api' has invalid keys: authorizer, exporter, importer, model, reader, writer

I fixed up the config, fixing the top

api:
  health:
    listen_address: "0.0.0.0:9494"
  services:

intentionally making the mistake to not remove the:

      health:
        listen_address: "0.0.0.0:9494"

section, which now fails with

2023/10/24 13:05:04 failed to create new config: failed to unmarshal config file: 6 error(s) decoding:

* 'api.services[authorizer]' has invalid keys: health
* 'api.services[exporter]' has invalid keys: health
* 'api.services[importer]' has invalid keys: health
* 'api.services[model]' has invalid keys: health
* 'api.services[reader]' has invalid keys: health
* 'api.services[writer]' has invalid keys: health

Now removed the health keys, to then fail with:

2023/10/24 13:08:08 failed to create new config: failed to validate config file: unsupported config version

The expectation is that we first check the version before attempting to unmarshal the config payload.


The cleanup, implementation yields the same results as the currently in-place workaround in terms of correctly closing the database in the correct order, so that is good!

I wonder if it would make sense that the context that is used to capture the os.Signal() should be passed down to the service implementations, so that we can respond correctly and drain connections. Right now we instantly close everything down, where I would hope we can get to the behavior that when the cancellation is called on the context we:

  1. stop accepting new requests
  2. drain the existing request using an acceptable timeout
  3. shutdown the stateful resources
  4. shutdown

I cannot envision if I can achieve this with the current cleanup function approach.

@carabasdaniel carabasdaniel force-pushed the config_v2 branch 2 times, most recently from dce0c7a to c52cefb Compare October 26, 2023 13:03
@gertd gertd merged commit aa873c6 into v3 Oct 27, 2023
5 checks passed
@gertd gertd deleted the config_v2 branch October 27, 2023 23:16
gertd added a commit that referenced this pull request Nov 4, 2023
* upd GetObject

* complete config

* Do not add to dependency map same address (#109)

* config.yaml add authorizer -> reader dependency

* GetObject revert ObjectTypeSelector validation logic

* REST API RC1

* upd assets (#112)

* OpenAPI support (#113)

* add OpenAPI support

* upd deps

* upd whitespace validation

* upd whitespace validation

* upd whitespace validation

* add model service

* config json schema

* update deps

* savepooint 20230913

* gdrive manifest and ds-load  (#139)

* Initial commit

* gdrive it is

* Fixing image paths, and updating emails

* Add console experience for topaz

* Fix linting

* Add console cli cmd

* Replace 0.0.0.0 with localhost

* Remove Println

* Get the latest commit from v3 go-edge-ds branch

* Use latest v3 console and fix handler paths

* Add model service configuration

* Only copy the files of the console build folder

* Expose default directory gateway port

* Topaz with conosole (`console06`)

* clean  pkg/app/console before build

* Bump to latest self-hosted-console

* fix migrate test

* fix migrate test
* add service cleanup

* Revert "fix migrate test"

This reverts commit 5b5dfd9.

* V3 only (#149)

* migrate all builtins to be v3 based, adds handling for v2 and v3 request payloads, v2 request return v2 results, help * is always v3 based
* migrate and reorganize tests, remove gomega/ginko dependency
* add service cleanup, the cleanup function returned by wire does not cleanup, leaving the database open

* Remove 'system' object from citadel data (#150)

* add manifest files

* updated citadel data in v3 branch

* upd v2 builtins & getUserFromIdentity

* upd topaz test command

* upd ci.yaml

* ci.yaml add CONSOLE_VERSION

* int automatic schema migration

* Latest v3 console (resizable panes)

* upd-deps (#156)

* Prepare changes for config version 2 (#147)

* Prepare changes for config version 2

* Update cli config template and exposed ports

* Update config doc and examples

* Add cleanups for topaz services

* Rename topaz service to authorizer

* Update testing configs

* Update test engine and configs

* Refactor topaz config services

* Move config version validation before unmarshal

* Defer runtime cleanup and bump service-host

* Rename authorizer and topaz structs and bump go-edge-ds

---------

Co-authored-by: Gert Drapers <gert.drapers@live.com>

* upd go-edge-ds

* upd Dockerfile (s)

* topaz configure --stdout

* add back LSP header

* Assets v3 (#158)

* assets v3 formatted

* assets v3 formatted

* go-directory-cli updates

* publish ghcr.io/aserto-dev/topaz:v3-latest-test-<platform>

* upd go-directory-cli v3

* upd ci to push v3-latest-test

* go-directory v0.30.0

* publish-test-image ci task

* add docker_manifests entry

* add docker_manifests entry

* revert migration changes

* push container

* excl configure policy msg when --stdout

* upd displayName to display_name

* fix GetObjects

* fix GetRelation(s)

* Latest console

* topaz test output fix

* upd CONSOLE_VERSION in ci.yaml

* err cleanup (#161)

* upd deps

* upd deps

* assets directory restructure (#160)

* console 0.0.0-20231102163131.0.g76203df3 (#163)

* console 0.0.0-20231102163131.0.g76203df3

* console 0.0.0-20231102163131.0.g76203df3

* v0.30.0 deps

* upd ci

---------

Co-authored-by: carabasdaniel <dani@aserto.com>
Co-authored-by: Glenn Block <glenn.block@gmail.com>
Co-authored-by: oanatmaria <oana@aserto.com>
Co-authored-by: Ronen Hilewicz <ronen@aserto.com>
Co-authored-by: Oana Tanasoiu <49147761+oanatmaria@users.noreply.github.com>
Co-authored-by: Ronen Hilewicz <github@ronen.hilewi.cz>
Co-authored-by: Omri Gazitt <ogazitt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants