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

Structured Configuration and context values overrides #2024

Closed
wants to merge 9 commits into from

Conversation

refs
Copy link
Member

@refs refs commented May 6, 2021

Configuring oCIS

Use Cases

Supervised

Something to keep in mind is that commands sent to the runtime to control started extensions neither accept cli flags, nor environment variables. The only way to configure an extension this way is via global config files or individual config files.

Let's have a look at some scenarios where we run ocis server:

  • global ocis.yaml config
    • will configure the entire set of extensions, setting sane defaults for those extensions left without configuration
  • single extension.yaml config file, no global ocis.yaml config
    • if there is a file configuring a single extension, values are loaded onto the config.
  • single extension.yaml config file and a global ocis.yaml config
    • values from the extension config file override globals

Unsupervised

  • ocis proxy
    • if a global ocis.yaml is present it will get loaded and configure the proxy.
    • if a global ocis.yaml + proxy.yaml are both present in expected locations, ocis.yaml will override values from proxy.yaml, however, values will be merged. Let's see an example:

ocis.yaml:

proxy:
  debug:
    addr: "localhost:5678"

proxy.yaml

http:
  addr: "localhost:1234"

running ocis proxy will result in the proxy service running with the following configuration:

end config

http:
  addr: "localhost:1234"
debug:
  addr: "localhost:5678"

ocis.yaml takes precedence and overrides proxy.yaml values. This is an artificial limitation due to the cli framework we're using is not flexible enough for this use case, and hence we have to merge values. We do it this way to ensure that for extensions running supervised, their configuration file overrides values from a global ocis.yaml.

This command still behaves as expected when combined with environment variables or cli arguments. Let's consider the following commands:

> ocis proxy --config-file /etc/ocis/proxy.json
> PROXY_CONFIG_FILE=/etc/ocis/proxy.json ocis proxy

In both cases the outcome is the same, in which values from proxy.yaml override globals. However when CLI flags are present, its values will override those from the config files (as they are more explicit). Let's see an example with the contents from the above scenario.

> ocis proxy --http-addr=1111`

The resulting service will run with the following configuration:

http:
  addr: "localhost:1111"
debug:
  addr: "localhost:5678"

It works similarly with its counterpart environment variable:

> PROXY_HTTP_ADDR=1111 ocis proxy`

The end result is the same.

Given these limitations, for an easier to reason about deployment, it is encouraged to use environment variables as opposed to structured configuration from files.

Apply to

  • proxy
  • accounts
  • idp
  • glauth
  • graph
  • graph-explorer
  • settings
  • storage
  • store
  • thumbnails
  • webdav
  • ocs
  • onlyoffice
  • web

@update-docs
Copy link

update-docs bot commented May 6, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic
Copy link
Member

butonic commented May 6, 2021

waiting for a comment from upstream ....

I get why the order of merging config gets twisted around when running ocis proxy vs ocis server.

The only other solution I see would be to implement our own merging algorithm, that keeps track of the differen types of config sources and can differentiate between the global.yaml config and the extension.yaml file. We should try and help get this solved upstream, IMO.

@refs
Copy link
Member Author

refs commented Jul 8, 2021

changes to the extensions should be limited to blocks like the one seeing in the proxy command:

Before: func(ctx *cli.Context) error {
	  return ParseConfig(ctx, cfg)
	  beforeOverride := config.Config{}
	  if err := copier.Copy(&beforeOverride, cfg); err != nil {
		  return err
	  }
	  
	  if err := ParseConfig(ctx, cfg); err != nil {
		  return err
	  }
	  
	  if err := mergo.Merge(cfg, beforeOverride, mergo.WithOverride); err != nil {
		  return err
	  }
	  
	  return nil
},

@refs refs changed the title [experimental] Structured Configuration and context values overrides Structured Configuration and context values overrides Jul 8, 2021
@refs
Copy link
Member Author

refs commented Jul 8, 2021

Configuration Concept

command
ocis.yaml
extension.yaml
effect
bin/ocis server


proxy http runs on default address.
PROXY_HTTP_ADDR=0.0.0.0:6666 bin/ocis server


HTTP listening on 0.0.0.0:6666
bin/ocis server


proxy load values from ocis.yaml: HTTP server listening on 0.0.0.0:1111
PROXY_HTTP_ADDR=0.0.0.0:6666 bin/ocis server


most explicit value overrides config file: HTTP listening on 0.0.0.0:6666
bin/ocis server


HTTP listening on 0.0.0.0:3333
PROXY_HTTP_ADDR=0.0.0.0:6666 bin/ocis server


HTTP listening on 0.0.0.0:6666
bin/ocis proxy


only values from proxy.yaml config file are read: HTTP listening on 0.0.0.0:3333
PROXY_HTTP_ADDR=0.0.0.0:9999 bin/ocis proxy


most explicit wins: HTTP listening on 0.0.0.0:9999

Some Axioms

  • Extensions running unsupervised do not care about ocis.yaml config file only about their config files (and the usual CLI flags and env variables)
  • ocis run extension will only use the extension config file NOT the global ocis config file. No CLI flags exist for run subcommand.

Config File Contents

ocis.yaml:

proxy:
  http:
    addr: "0.0.0.0:1111"
accounts:
  http:
    addr: "0.0.0.0:2222"

proxy.yaml

http:
  addr: "0.0.0.0:3333"
http:
  addr: "0.0.0.0:4444"

@refs
Copy link
Member Author

refs commented Jul 8, 2021

@butonic since b0ee695 we can now load the proxy policies from a file, that only holds proxy policies). This is how to use it:

proxy_policies.json:

[
  {
    "name": "ocis",
    "routes": [
      {
        "endpoint": "/",
        "backend": "http://localhost:9100"
      }
    ]
  }
]
MICRO_LOG_LEVEL=error OCIS_LOG_PRETTY=true OCIS_LOG_COLOR=true OCIS_LOG_LEVEL=debug PROXY_POLICIES_FILE=proxy_policies.json bin/ocis proxy

2021-07-08T16:19:19+02:00 DBG Tracing is not enabled service=proxy
2021-07-08T16:19:19+02:00 INF loaded proxy policies service=proxy source="policies file"
2021-07-08T16:19:19+02:00 WRN policy-selector not configured. Will always use first policy: 'ocis' service=proxy
2021-07-08T16:19:19+02:00 DBG loading policy-selector selector_config={"Migration":null,"Static":{"Policy":"ocis"}} service=proxy
2021-07-08T16:19:19+02:00 DBG adding route route={"ApacheVHost":false,"Backend":"http://localhost:9100","Endpoint":"/","Type":""} service=proxy
2021-07-08T16:19:19+02:00 WRN No tls certificate provided, using a generated one service=proxy
2021-07-08T16:19:19+02:00 INF starting server addr=0.0.0.0:9200 service=proxy transport=https
2021-07-08T16:19:19+02:00 INF starting server addr=0.0.0.0:9205 service=proxy transport=debug

The log was updated to reflect the source of the policies, in this case source=policies file, and the contents are displayed as we are in debug level:

2021-07-08T16:19:19+02:00 DBG adding route route={"ApacheVHost":false,"Backend":"http://localhost:9100","Endpoint":"/","Type":""} service=proxy

@refs refs marked this pull request as ready for review July 8, 2021 14:55
@butonic
Copy link
Member

butonic commented Jul 8, 2021

Trying to run

PROXY_TLS=false go run cmd/ocis/main.go server

I need this because I run the dev container behind traefik ...

but the browseor gives me Client sent an HTTP request to an HTTPS server.

And in a debug level log I see:

{"level":"warn","service":"proxy","time":"2021-07-08T20:15:07Z","message":"No tls certificate provided, using a generated one"}
{"level":"info","service":"proxy","transport":"https","addr":"0.0.0.0:9200","time":"2021-07-08T20:15:07Z","message":"starting server"}

so it indeed seems to ignore the PROXY_TLS=true. Can you reproduce?

Also, I added a row in #2024 (comment) ... I think I know what should happen but maybe you can copy the great explanation into a dev docs section?

@refs refs force-pushed the experimental-config-parsing branch from bf96e3a to 2262e47 Compare July 9, 2021 10:08
@owncloud owncloud deleted a comment from ownclouders Jul 9, 2021
@butonic butonic marked this pull request as draft July 26, 2021 14:55

// Default are values stored in the flagset, but moved to a struct.
func DefaultConfig() Config {
return Config{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not compatible with flaex. We need to change the config parser and then make this change to every extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2021-08-01 at 23-25-32 Configuration ownCloud

@micbar micbar force-pushed the experimental-config-parsing branch 2 times, most recently from 808e2f1 to 0382624 Compare August 2, 2021 14:56
@micbar micbar force-pushed the experimental-config-parsing branch from df03ece to b9c9c89 Compare August 12, 2021 13:38
@micbar micbar force-pushed the experimental-config-parsing branch from b9c9c89 to a498d61 Compare August 12, 2021 13:58
@micbar micbar force-pushed the experimental-config-parsing branch from a498d61 to b99a464 Compare August 12, 2021 14:31
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

6.8% 6.8% Coverage
3.5% 3.5% Duplication

@wkloucek
Copy link
Contributor

can we close this PR in favor of #2708?

@wkloucek
Copy link
Contributor

@refs I'm closing this since #2708 is merged. Please reopen if needed

@wkloucek wkloucek closed this Nov 24, 2021
@wkloucek wkloucek deleted the experimental-config-parsing branch January 21, 2022 08:58
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.

4 participants