feat: Core Keeper implementation #10

jackchenjc · 2024-01-30T08:26:55Z

closes: #9

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-examples/blob/master/.github/CONTRIBUTING.md.

What is the current behavior?

Issue Number:

#9

What is the new behavior?

Core Keeper is a lightweight configuration and registry service that is aimed to replace Consul in the EdgeX architecture

Does this PR introduce a breaking change?

Yes
No

New Imports

github.com/spf13/cast to safely and easily casting from one type to another in Go

Specific Instructions

Common configuration pushing

Start a docker container to run redis with docker run -d --rm --name redis -p 6379:6379 redis
Run make keeper common-config to ensure the binary builds
Go to the core keeper folder and run export EDGEX_SECURITY_SECRET_STORE=false; ./core-keeper
Make sure there is no error log
Go to the core common folder and run export EDGEX_SECURITY_SECRET_STORE=false; ./core-common-config-bootstrapper -cp=keeper.http://localhost:59890 which specifies using core keeper as configuration provider
Verify logs contain the following:

level=INFO ts=2024-01-30T07:20:01.885887Z app=core-common-config-bootstrapper source=config.go:744 msg="Using Configuration provider (keeper) from: http://localhost:59890 with base path of edgex/v3/core-common-config-bootstrapper"
level=INFO ts=2024-01-30T07:20:01.892918Z app=core-common-config-bootstrapper source=main.go:122 msg="Pushing common configuration. It doesn't exists or overwrite flag is set"
level=INFO ts=2024-01-30T07:20:01.895441Z app=core-common-config-bootstrapper source=main.go:167 msg="Using common configuration from res/configuration.yaml"
level=INFO ts=2024-01-30T07:20:01.897215Z app=core-common-config-bootstrapper source=main.go:250 msg="Common configuration loaded from file with 0 overrides applied"
level=INFO ts=2024-01-30T07:20:01.979008Z app=core-common-config-bootstrapper source=main.go:213 msg="Common configuration has been pushed to into Configuration Provider with overrides applied"
level=INFO ts=2024-01-30T07:20:01.979051Z app=core-common-config-bootstrapper source=main.go:134 msg="Core Common Config exiting"

Service configuration consuming, pushing and watching for change

Continue with the above services running
Take core-metadata for testing. Run make metadata to ensure the binary builds
Go to the core metadata folder and run export EDGEX_SECURITY_SECRET_STORE=false; ./core-metadata -cp=keeper.http://localhost:59890 which specifies using core keeper as configuration provider
Run the following curl command to verify the common configs have been applied curl localhost:59881/api/v3/config
Run the following curl command to test the WatchForChange feature using Core Keeper

curl -X 'PUT' \
              'http://localhost:59890/api/v3/kvs/key/edgex/v3/core-metadata/Writable/LogLevel' \
              -H 'accept: application/json' \
              -d '{ "value": "DEBUG"  }'

Verify logs contain the following:

level=INFO ts=2024-01-30T07:41:39.397226Z app=core-metadata source=config.go:1031 msg="Logging level changed to DEBUG"

Registration

Go to cmd/core-common-config-bootstrapper/res and modify the Registry section in configuration.yaml as

  Registry:
    Host: "localhost"
    Port: 59890
    Type: "keeper"

Follow the steps described in Common configuration pushing again to start a new redis container and get the new binary builds
Take core-metadata for testing. Run make metadata to ensure the binary builds
Go to the core metadata folder and run export EDGEX_SECURITY_SECRET_STORE=false; ./core-metadata -cp=keeper.http://localhost:59890/ -r which specifies using core keeper as configuration provider and using registry
Verify logs of core keeper contain the following:

level=INFO ts=2024-01-30T08:02:00.856666Z app=core-keeper source=registry.go:50 msg="Registered service: core-metadata"

Other information

Related go modules issues and new branches:
go-mod-core-contracts
issue: edgexfoundry/go-mod-core-contracts#873
branch: https://github.com/jackchenjc/go-mod-core-contracts/tree/issue-873

go-mod-configuration
issue: edgexfoundry/go-mod-configuration#109
branch: https://github.com/jackchenjc/go-mod-configuration/tree/issue-109

go-mod-bootstrap
issue: edgexfoundry/go-mod-bootstrap#649
branch: https://github.com/jackchenjc/go-mod-bootstrap/tree/issue-649

go-mod-registry
issue: edgexfoundry/go-mod-registry#134
branch: https://github.com/jackchenjc/go-mod-registry/tree/issue-134

closes: edgexfoundry-holding#9 Signed-off-by: Jack Chen <[email protected]>

jackchenjc · 2024-01-30T09:35:33Z

Hi @lenny-intel,

This PR contains the initial implementation of Core Keeper, which has not been tested in secure mode yet.
And the Registry service need to be replaced to Core Keeper manually in the configuration.yaml file of core-common-config-bootstrapper for now.

Would you please help reviewing it when you have time? Thanks.

lenny-goodell · 2024-02-08T16:42:50Z

@cloudxxx8 , @jackchenjc , the TSC approved adding this service to EdgeX in Odessa. I don't see the need to do the review in holding and again when it is moved to main org. We never did this with any of the new security services and common config service.

That said I'll do a high level review here and more detailed when moved to main org.

lenny-goodell · 2024-02-08T16:52:03Z

internal/core/keeper/config/config.go

+type ConfigurationStruct struct {
+	Writable   WritableInfo
+	MessageBus bootstrapConfig.MessageBusInfo
+	Clients    bootstrapConfig.ClientsCollection
+	Database   bootstrapConfig.Database
+	Service    bootstrapConfig.ServiceInfo
+}
+
+type WritableInfo struct {
+	LogLevel        string
+	InsecureSecrets bootstrapConfig.InsecureSecrets
+	Telemetry       bootstrapConfig.TelemetryInfo
+}


This service is NOT a standard EdgeX service when it comes to configuration since it manages the configuration. IMO, this needs to be re-thought. How would it handle Writable? i.e. how would you change this services configuration?? Security service don't have the Writable section. Example:
https://github.com/edgexfoundry/edgex-go/blob/main/cmd/security-bootstrapper/res/configuration.yaml
https://github.com/edgexfoundry/edgex-go/blob/main/internal/security/bootstrapper/config/config.go#L24

IMO, Telemetry is not needed now and isn't in your configuration.yaml. Remove until this service has some metrics that it is collecting. Or add it to your configuration with the coomm security metrics listed.
https://docs.edgexfoundry.org/3.1/microservices/general/#common-service-metrics

lenny-goodell · 2024-02-08T16:56:04Z

internal/core/keeper/container/registry.go

+	"github.com/edgexfoundry/edgex-go/internal/core/keeper/infrastructure/interfaces"
+)
+
+// RegistryInterfaceName contains the name of the interfaces.Registry implementation in the DIC.


Why is the registry implementation needed in the DIC and the kv isn't? i.e. this service implements the Registry rather than dependent on it.

lenny-goodell · 2024-02-08T17:00:47Z

cmd/core-keeper/Dockerfile

+COPY --from=builder /edgex-go/cmd/core-keeper/res/configuration.toml /res/configuration.toml
+
+ENTRYPOINT ["/core-keeper"]
+CMD ["--confdir=/res"]


Remove --confdir=/res since it is the default. This has been done removed for for all the other services

lenny-goodell · 2024-02-08T17:01:11Z

cmd/core-keeper/Dockerfile

+RUN apk add --update --no-cache dumb-init
+COPY --from=builder /edgex-go/Attribution.txt /
+COPY --from=builder /edgex-go/cmd/core-keeper/core-keeper /
+COPY --from=builder /edgex-go/cmd/core-keeper/res/configuration.toml /res/configuration.toml


Yaml file now.

lenny-goodell · 2024-02-08T17:17:52Z

go.mod

+replace github.com/edgexfoundry/go-mod-core-contracts/v3 => github.com/jackchenjc/go-mod-core-contracts/v3 v3.2.0-dev.4.0.20240130071054-92a2e48bc6ca
+
+replace github.com/edgexfoundry/go-mod-registry/v3 => github.com/jackchenjc/go-mod-registry/v3 v3.2.0-dev.1.0.20240130024331-d0f728a41496
+
+replace github.com/edgexfoundry/go-mod-configuration/v3 => github.com/jackchenjc/go-mod-configuration/v3 v3.2.0-dev.1.0.20240130054648-e25c4056b9df
+
+replace github.com/edgexfoundry/go-mod-bootstrap/v3 => github.com/jackchenjc/go-mod-bootstrap/v3 v3.2.0-dev.10.0.20240125072932-e0925013f378


Why does this Keeper Service need modified versions of the go-mods?

I expect the go-mods to be modified for the new Client, not the Service.

lenny-goodell · 2024-02-08T17:19:16Z

internal/core/keeper/application/registry.go

+	registry := container.RegistryFrom(dic.Get)
+	registry.Register(r)


lenny-goodell · 2024-02-08T17:20:51Z

internal/core/keeper/infrastructure/interfaces/mocks/Registry.go

+)
+
+// Registry is an autogenerated mock type for the Registry type
+type Registry struct {


Mocks are do dependencies. This service implements the Registry so it isn't a dependency.

lenny-goodell · 2024-02-08T17:23:11Z

internal/core/keeper/main.go

+	//      ....
+	//      flags.Parse(os.Args[1:])
+	//
+	f := flags.New()


This adds many command line flags that are meaningless to this service, like -cp/--configProvider.

feat: Core Keeper implementation

bf726ea

closes: edgexfoundry-holding#9 Signed-off-by: Jack Chen <[email protected]>

lenny-goodell requested changes Feb 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Core Keeper implementation #10

feat: Core Keeper implementation #10

jackchenjc commented Jan 30, 2024 •

edited

Loading

jackchenjc commented Jan 30, 2024 •

edited

Loading

lenny-goodell commented Feb 8, 2024 •

edited

Loading

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

lenny-goodell Feb 8, 2024

		registry := container.RegistryFrom(dic.Get)
		registry.Register(r)

feat: Core Keeper implementation #10

Are you sure you want to change the base?

feat: Core Keeper implementation #10

Conversation

jackchenjc commented Jan 30, 2024 • edited Loading

PR Checklist

What is the current behavior?

Issue Number:

What is the new behavior?

Does this PR introduce a breaking change?

New Imports

Specific Instructions

Other information

jackchenjc commented Jan 30, 2024 • edited Loading

lenny-goodell commented Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackchenjc commented Jan 30, 2024 •

edited

Loading

jackchenjc commented Jan 30, 2024 •

edited

Loading

lenny-goodell commented Feb 8, 2024 •

edited

Loading