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

add OCIS_URL env var #1148

Merged
merged 10 commits into from
Dec 23, 2020
Merged

add OCIS_URL env var #1148

merged 10 commits into from
Dec 23, 2020

Conversation

butonic
Copy link
Member

@butonic butonic commented Dec 21, 2020

We introduced a new environment variable OCIS_URL that exects a URL including protocol, host and optionally port to configure all the different services. These existing environment variables still take precedence, but will also fall back to OCIS_URL: STORAGE_LDAP_IDP, STORAGE_OIDC_ISSUER, PROXY_OIDC_ISSUER, STORAGE_FRONTEND_PUBLIC_URL, KONNECTD_ISS, WEB_OIDC_AUTHORITY, WEB_UI_CONFIG_SERVER.

Some environment variables are now built dynamically if they are not set:

  • STORAGE_DATAGATEWAY_PUBLIC_URL defaults to <STORAGE_FRONTEND_PUBLIC_URL>/data, also falling back to OCIS_URL
  • WEB_OIDC_METADATA_URL defaults to <WEB_OIDC_AUTHORITY>/.well-known/openid-configuration, also falling back to OCIS_URL

Furthermore, the built in konnectd will generate an identifier-registration.yaml that uses the KONNECTD_ISS in the allowed redirect_uris and origins. It simplifies the default https:/localhost:9200 and remote deployment with OCIS_URL which is evaluated as a fallback if KONNECTD_ISS is not set.

An OCIS server can now be started on a remote machine as easy as OCIS_URL=https://cloud.ocis.test PROXY_HTTP_ADDR=0.0.0.0:443 ocis server.

Note that the OCIS_DOMAIN environment variable is not used by ocis, but by the docker containers.

TODO

  • set PROXY_HTTP_ADDR based on OCIS_URL? for now running OCIS_URL="https://cloud.ocis.test" PROXY_HTTP_ADDR=0.0.0.0:443 ocis server should start the proxy on the https port and configure all URLs to use it 🤔 hmmm no .. could be done in a subsequent PR. keeping OCIS_URL and PROXY_HTTP_ADDR separate allows putting a traefik in between...
  • use konnectd identifier-registration.yaml asset as a template and update the domain after generating it for the first time.

cc @dragotin

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@@ -167,7 +167,7 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag {
&cli.StringFlag{
Name: "iss",
Usage: "OIDC issuer URL",
EnvVars: []string{"KONNECTD_ISS"},
EnvVars: []string{"KONNECTD_ISS", "OCIS_URL"}, // KONNECTD_ISS takes precedence over OCIS_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs pick up only the first environment variable. What should we do about this? Document it differently or do a PR to https://github.com/owncloud/flaex?

Copy link
Member Author

Choose a reason for hiding this comment

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

PR to flaex is the correct fix IMO. Cc @IljaN

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 👍

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@@ -167,7 +167,7 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag {
&cli.StringFlag{
Name: "iss",
Usage: "OIDC issuer URL",
EnvVars: []string{"KONNECTD_ISS"},
EnvVars: []string{"KONNECTD_ISS", "OCIS_URL"}, // KONNECTD_ISS takes precedence over OCIS_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

While digging through the code I noticed, that this enables easy configuration for environment variable only. This will not picked up if you run ocis --ocis-url https://ocis.owncloud.test server.

For that to work something like for OCIS_LOG_LEVEL must be done. This is implemented like this:

func configureAccounts(cfg *config.Config) *svcconfig.Config {
cfg.Accounts.Log.Level = cfg.Log.Level
cfg.Accounts.Log.Pretty = cfg.Log.Pretty
cfg.Accounts.Log.Color = cfg.Log.Color
cfg.Accounts.Server.Version = version.String
if cfg.Tracing.Enabled {
cfg.Accounts.Tracing.Enabled = cfg.Tracing.Enabled
cfg.Accounts.Tracing.Type = cfg.Tracing.Type
cfg.Accounts.Tracing.Endpoint = cfg.Tracing.Endpoint
cfg.Accounts.Tracing.Collector = cfg.Tracing.Collector
}
if cfg.TokenManager.JWTSecret != "" {
cfg.Accounts.TokenManager.JWTSecret = cfg.TokenManager.JWTSecret
cfg.Accounts.Repo.CS3.JWTSecret = cfg.TokenManager.JWTSecret
}
return cfg.Accounts
}

I'm not sure which is the way to go because it also has downsides. In this example OCIS_LOG_LEVEL will always win, even if your provide a lower ACCOUNTS_LOG_LEVEL

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. the downside is known but accepted because the ocis binary is a poor mans orchestrator. I would argue that in eg docker compose deployments we should run the services in their own docker containers, which is why the multi repo setup was building a docker container for every service ... dunno if we still do that ...

Copy link
Member Author

Choose a reason for hiding this comment

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

we'll add the --ocis-url flag

Copy link
Member Author

Choose a reason for hiding this comment

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

scratch that ... I thought we could add a --ocis-url flag to the ocis server command and use configure<Service>() to apply it to the service config, similar to the log level and pretty flags. But that would introduce specific code that only handles this flag, which seems a little brittle to me. @IljaN mentioned that there might be something in the urfave/cli / micro/cli flags to define a flag that can be reused. So I'd postpone a --ocis-url cli flag until later.

@IljaN IljaN self-requested a review December 23, 2020 11:01
changelog/unreleased/add-ocis-url-env.md Outdated Show resolved Hide resolved
@butonic
Copy link
Member Author

butonic commented Dec 23, 2020

Tests are happy except move ... 👀

@wkloucek
Copy link
Contributor

this might be also related to owncloud/product#15

@butonic
Copy link
Member Author

butonic commented Dec 23, 2020

hm the publicstorage dies on startup:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2fc8c69]

goroutine 1 [running]:
github.com/owncloud/ocis/ocis/pkg/command.StoragePublicLinkCommand.func1(0xc000357540, 0xf, 0x14)
        /home/vscode/repositories/ocis/ocis/pkg/command/storagepubliclink.go:24 +0x149
github.com/micro/cli/v2.(*Command).Run(0xc00069dd40, 0xc000357000, 0x0, 0x0)
        /home/vscode/go/pkg/mod/github.com/micro/cli/[email protected]/command.go:161 +0x4ed
github.com/micro/cli/v2.(*App).RunContext(0xc000228780, 0x3c12f00, 0xc000052088, 0xc00000e080, 0x2, 0x2, 0x0, 0x0)
        /home/vscode/go/pkg/mod/github.com/micro/cli/[email protected]/app.go:303 +0x81f
github.com/micro/cli/v2.(*App).Run(...)
        /home/vscode/go/pkg/mod/github.com/micro/cli/[email protected]/app.go:210
github.com/owncloud/ocis/ocis/pkg/command.Execute(0xffffffff, 0xc00005e0b8)
        /home/vscode/repositories/ocis/ocis/pkg/command/root.go:70 +0x526
main.main()
        /home/vscode/repositories/ocis/ocis/cmd/ocis/main.go:10 +0x25

@butonic butonic marked this pull request as ready for review December 23, 2020 12:01
@butonic
Copy link
Member Author

butonic commented Dec 23, 2020

ok, this should turn out green 🤞

@dragotin please try a remote deployment a la OCIS_URL=https://host:1111 PROXY_HTTP_ADDR=0.0.0.0:1111 ocis server. 🎁

@sonarcloud
Copy link

sonarcloud bot commented Dec 23, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@butonic butonic merged commit 5689daf into master Dec 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the add_ocis_url_env branch December 23, 2020 16:25
ownclouders pushed a commit that referenced this pull request Dec 23, 2020
Merge: 335aa2d 652448f
Author: Jörn Friedrich Dreyer <[email protected]>
Date:   Wed Dec 23 17:25:24 2020 +0100

    Merge pull request #1148 from owncloud/add_ocis_url_env

    add OCIS_URL env var
@phil-davis phil-davis mentioned this pull request Jan 8, 2021
@kulmann kulmann mentioned this pull request Jan 22, 2021
15 tasks
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.

None yet

4 participants