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.

Sign up for GitHub

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

Jump to bottom

add OCIS_ENABLE_OCM env var #9784

Merged
merged 2 commits into from
Aug 12, 2024
Merged

add OCIS_ENABLE_OCM env var #9784

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f960ae4
add OCIS_ENABLE_OCM env var
butonic Aug 12, 2024
d61e2c1
remove runtime changes for OCIS_ENABLE_OCM
butonic Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .drone.star
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,7 @@ config = {
"skip": False,
"federationServer": True,
"extraServerEnvironment": {
"OCIS_ADD_RUN_SERVICES": "ocm",
"GRAPH_INCLUDE_OCM_SHAREES": True,
"OCIS_ENABLE_OCM": True,
"OCM_OCM_INVITE_MANAGER_INSECURE": True,
"OCM_OCM_SHARE_PROVIDER_INSECURE": True,
"OCM_OCM_STORAGE_PROVIDER_INSECURE": True,
Expand Down
7 changes: 1 addition & 6 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,13 @@
"OCM_OCM_INVITE_MANAGER_INSECURE": "true",
"OCM_OCM_SHARE_PROVIDER_INSECURE": "true",
"OCM_OCM_STORAGE_PROVIDER_INSECURE": "true",
"FRONTEND_OCS_INCLUDE_OCM_SHAREES": "true",
"OCIS_BASE_DATA_PATH": "${env:HOME}/.ocis-10200",
"OCIS_CONFIG_DIR": "${env:HOME}/.ocis-10200/config",
"OCIS_EVENTS_ENDPOINT": "127.0.0.1:10233",
"OCIS_LDAP_URI": "ldaps://localhost:10235",
"OCIS_RUNTIME_PORT": "10250",
"OCIS_URL": "https://federation-ocis-server:10200",
"FRONTEND_OCS_LIST_OCM_SHARES": "true",
"FRONTEND_ENABLE_FEDERATED_SHARING_INCOMING": "true",
"FRONTEND_ENABLE_FEDERATED_SHARING_OUTGOING": "true",
"OCIS_ADD_RUN_SERVICES": "ocm",
"GRAPH_INCLUDE_OCM_SHAREES": "true",
"OCIS_ENABLE_OCM": "true",
"APP_PROVIDER_DEBUG_ADDR": "127.0.0.1:10165",
"APP_PROVIDER_GRPC_ADDR": "127.0.0.1:10164",
"APP_REGISTRY_DEBUG_ADDR": "127.0.0.1:10243",
Expand Down
5 changes: 5 additions & 0 deletions changelog/unreleased/add-enable-ocm-variable.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Add OCIS_ENABLE_OCM env var

We added a new `OCIS_ENABLE_OCM` env var that will enable all ocm flags and add `ocm` to the services started by the runtime.

https://github.com/owncloud/ocis/pull/9784
14 changes: 2 additions & 12 deletions docs/ocis/development/testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,12 +556,8 @@ in the `/etc/hosts` file
# run oCIS
OCIS_URL="https://ocis-server:9200" \
PROXY_ENABLE_BASIC_AUTH=true \
GRAPH_INCLUDE_OCM_SHAREES=true \
OCM_OCM_INVITE_MANAGER_INSECURE=true \
OCM_OCM_SHARE_PROVIDER_INSECURE=true \
OCM_OCM_STORAGE_PROVIDER_INSECURE=true \
OCIS_ENABLE_OCM=true \
OCM_OCM_PROVIDER_AUTHORIZER_PROVIDERS_FILE="${workspaceFolder}/tests/config/drone/providers.json" \
OCIS_ADD_RUN_SERVICES="ocm" \
ocis/bin/ocis server
```

Expand All @@ -583,13 +579,7 @@ ocis/bin/ocis server
The second oCIS instance should be available at: https://federation-ocis-server:10200/

{{< hint info >}}
To enable ocm in the web interface, you need to set the following envs:
`FRONTEND_OCS_INCLUDE_OCM_SHAREES=true` \
`FRONTEND_OCS_LIST_OCM_SHARES=true` \
`FRONTEND_ENABLE_FEDERATED_SHARING_INCOMING=true` \
`FRONTEND_ENABLE_FEDERATED_SHARING_OUTGOING=true` \

and put `ocm` to apps https://github.com/owncloud/ocis/blob/master/services/web/pkg/config/defaults/defaultconfig.go#L101
To enable ocm in the web interface, you need to set `OCIS_ENABLE_OCM=true`. It will enable all ocm flags and start the ocm service when running `ocis server`.
{{< /hint>}}

#### Run the Acceptance Test
Expand Down
1 change: 1 addition & 0 deletions ocis-pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type Runtime struct {
Services []string `yaml:"services" env:"OCIS_RUN_EXTENSIONS;OCIS_RUN_SERVICES" desc:"A comma-separated list of service names. Will start only the listed services." introductionVersion:"pre5.0"`
Disabled []string `yaml:"disabled_services" env:"OCIS_EXCLUDE_RUN_SERVICES" desc:"A comma-separated list of service names. Will start all default services except of the ones listed. Has no effect when OCIS_RUN_SERVICES is set." introductionVersion:"pre5.0"`
Additional []string `yaml:"add_services" env:"OCIS_ADD_RUN_SERVICES" desc:"A comma-separated list of service names. Will add the listed services to the default configuration. Has no effect when OCIS_RUN_SERVICES is set. Note that one can add services not started by the default list and exclude services from the default list by using both envvars at the same time." introductionVersion:"pre5.0"`
EnableOCM bool `yaml:"enable_ocm" env:"OCIS_ENABLE_OCM" desc:"Enable all OCM config flags and start the ocm service." introductionVersion:"%%NEXT%%"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need an extra envvar for this? Whats wrong with OCIS_ADD_RUN_SERVICES=ocm?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAICT pm wanted a single env var to enable ocm. that means we have to also make the runtime pick up the env var to start the service.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please let's NOT do this. We have too many envvars anyways. We cannot add envvars for specific deployment scenarios. Next week there'll be OCIS_MY_CUSTOM_DEPLOYMENT which start antivirus and webfinger but not search.

Tell PO OCIS_ADD_RUN_SERVICES IS a single envvar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I was trying to argue that the helm chart is the layer that uses feature flags like this. @micbar was against it. And the whole point of this PR is to add a feature flag env var OCIS_ENABLE_OCM that sets all the other env vars. Making it also enable the service at least makes it not a half baked solution ...

all or nothing IMO

Copy link
Collaborator

@kobergj kobergj Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was trying to argue that the helm chart is the layer that uses feature flags like this

I'm 100% with you here. Please let's not do things we both know are wrong.

And the whole point of this PR is to add a feature flag env var OCIS_ENABLE_OCM that sets all the other env vars

Why don't we just default them all to true? Then OCIS_ADD_RUN_SERVICES would suffice the requirements. What is the point in starting ocm service if you don't want to use it anyways?

Copy link
Member Author

Choose a reason for hiding this comment

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

setting them to true results in various error messages in the log because the ocm service cannot be contacted.

Looking at the list item New variable like OCIS_ENABLE_OCM should summarize most of the variable to simplify enablement of ocm we could just add OCIS_ENABLE_OCM to the existing boolean env vars and not start ocm ... but that seems to just be a half assed solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the list item New variable like OCIS_ENABLE_OCM should summarize most of the variable to simplify enablement of ocm we could just add OCIS_ENABLE_OCM to the existing boolean env vars and not start ocm

I'd like that more. The runtime already listens to: OCIS_ADD_RUN_SERVICES, OCIS_EXCLUDE_RUN_SERVICES and OCIS_RUN_SERVICES. Listening to a fourth envvar will only make it more complicated and produces edge cases that need to be considered. (e.g. OCIS_EXCLUDE_RUN_SERVICES=ocm and OCIS_ENABLE_OCM=true)

OCIS_ENABLE_OCM activates all service specific envvars, ocm service needs to be started separately. That sounds nice and simple to me.

}

// Config combines all available configuration parts.
Expand Down
5 changes: 5 additions & 0 deletions ocis/pkg/runtime/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,11 @@ func (s *Service) generateRunSet(cfg *ociscfg.Config) {
runset[name] = struct{}{}
}

// add ocm service if explicitly enabled by config
if cfg.Runtime.EnableOCM {
runset[cfg.OCM.Service.Name] = struct{}{}
}

// remove services if explicitly excluded by config
for _, name := range cfg.Runtime.Disabled {
delete(runset, name)
Expand Down
8 changes: 4 additions & 4 deletions services/frontend/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ type Config struct {
UploadMaxChunkSize int `yaml:"upload_max_chunk_size" env:"FRONTEND_UPLOAD_MAX_CHUNK_SIZE" desc:"Sets the max chunk sizes in bytes for uploads via the clients." introductionVersion:"pre5.0"`
UploadHTTPMethodOverride string `yaml:"upload_http_method_override" env:"FRONTEND_UPLOAD_HTTP_METHOD_OVERRIDE" desc:"Advise TUS to replace PATCH requests by POST requests." introductionVersion:"pre5.0"`
DefaultUploadProtocol string `yaml:"default_upload_protocol" env:"FRONTEND_DEFAULT_UPLOAD_PROTOCOL" desc:"The default upload protocol to use in clients. Currently only 'tus' is available. See the developer API documentation for more details about TUS." introductionVersion:"pre5.0"`
EnableFederatedSharingIncoming bool `yaml:"enable_federated_sharing_incoming" env:"FRONTEND_ENABLE_FEDERATED_SHARING_INCOMING" desc:"Changing this value is NOT supported. Enables support for incoming federated sharing for clients. The backend behaviour is not changed." introductionVersion:"pre5.0"`
EnableFederatedSharingOutgoing bool `yaml:"enable_federated_sharing_outgoing" env:"FRONTEND_ENABLE_FEDERATED_SHARING_OUTGOING" desc:"Changing this value is NOT supported. Enables support for outgoing federated sharing for clients. The backend behaviour is not changed." introductionVersion:"pre5.0"`
EnableFederatedSharingIncoming bool `yaml:"enable_federated_sharing_incoming" env:"OCIS_ENABLE_OCM;FRONTEND_ENABLE_FEDERATED_SHARING_INCOMING" desc:"Changing this value is NOT supported. Enables support for incoming federated sharing for clients. The backend behaviour is not changed." introductionVersion:"pre5.0"`
EnableFederatedSharingOutgoing bool `yaml:"enable_federated_sharing_outgoing" env:"OCIS_ENABLE_OCM;FRONTEND_ENABLE_FEDERATED_SHARING_OUTGOING" desc:"Changing this value is NOT supported. Enables support for outgoing federated sharing for clients. The backend behaviour is not changed." introductionVersion:"pre5.0"`
SearchMinLength int `yaml:"search_min_length" env:"FRONTEND_SEARCH_MIN_LENGTH" desc:"Minimum number of characters to enter before a client should start a search for Share receivers. This setting can be used to customize the user experience if e.g too many results are displayed." introductionVersion:"pre5.0"`
Edition string `yaml:"edition" env:"OCIS_EDITION;FRONTEND_EDITION" desc:"Edition of oCIS. Used for branding purposes." introductionVersion:"pre5.0"`
DisableSSE bool `yaml:"disable_sse" env:"OCIS_DISABLE_SSE;FRONTEND_DISABLE_SSE" desc:"When set to true, clients are informed that the Server-Sent Events endpoint is not accessible." introductionVersion:"pre5.0"`
Expand Down Expand Up @@ -141,10 +141,10 @@ type OCS struct {
CacheWarmupDriver string `yaml:"cache_warmup_driver,omitempty"` // not supported by the oCIS product, therefore not part of docs
CacheWarmupDrivers CacheWarmupDrivers `yaml:"cache_warmup_drivers,omitempty"` // not supported by the oCIS product, therefore not part of docs
EnableDenials bool `yaml:"enable_denials" env:"FRONTEND_OCS_ENABLE_DENIALS" desc:"EXPERIMENTAL: enable the feature to deny access on folders." introductionVersion:"pre5.0"`
ListOCMShares bool `yaml:"list_ocm_shares" env:"FRONTEND_OCS_LIST_OCM_SHARES" desc:"Include OCM shares when listing shares. See the OCM service documentation for more details." introductionVersion:"5.0"`
ListOCMShares bool `yaml:"list_ocm_shares" env:"OCIS_ENABLE_OCM;FRONTEND_OCS_LIST_OCM_SHARES" desc:"Include OCM shares when listing shares. See the OCM service documentation for more details." introductionVersion:"5.0"`
IncludeOCMSharees bool `yaml:"include_ocm_sharees" env:"OCIS_ENABLE_OCM;FRONTEND_OCS_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing sharees." introductionVersion:"5.0"`
PublicShareMustHavePassword bool `yaml:"public_sharing_share_must_have_password" env:"OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD;FRONTEND_OCS_PUBLIC_SHARE_MUST_HAVE_PASSWORD" desc:"Set this to true if you want to enforce passwords on all public shares." introductionVersion:"5.0"`
WriteablePublicShareMustHavePassword bool `yaml:"public_sharing_writeableshare_must_have_password" env:"OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD;FRONTEND_OCS_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD" desc:"Set this to true if you want to enforce passwords for writable shares. Only effective if the setting for 'passwords on all public shares' is set to false." introductionVersion:"5.0"`
IncludeOCMSharees bool `yaml:"include_ocm_sharees" env:"FRONTEND_OCS_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing sharees." introductionVersion:"5.0"`
ShowUserEmailInResults bool `yaml:"show_email_in_results" env:"OCIS_SHOW_USER_EMAIL_IN_RESULTS" desc:"Include user email addresses in responses. If absent or set to false emails will be omitted from results. Please note that admin users can always see all email addresses." introductionVersion:"6.0.0"`
}

Expand Down
2 changes: 1 addition & 1 deletion services/graph/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ type Config struct {
Application Application `yaml:"application"`
Spaces Spaces `yaml:"spaces"`
Identity Identity `yaml:"identity"`
IncludeOCMSharees bool `yaml:"include_ocm_sharees" env:"GRAPH_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing users." introductionVersion:"5.0"`
IncludeOCMSharees bool `yaml:"include_ocm_sharees" env:"OCIS_ENABLE_OCM;GRAPH_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing users." introductionVersion:"5.0"`
Events Events `yaml:"events"`

Keycloak Keycloak `yaml:"keycloak"`
Expand Down
7 changes: 1 addition & 6 deletions tests/config/drone/.env-federation
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,13 @@ export OCM_OCM_PROVIDER_AUTHORIZER_PROVIDERS_FILE=tests/config/drone/providers.j
export OCM_OCM_INVITE_MANAGER_INSECURE=true
export OCM_OCM_SHARE_PROVIDER_INSECURE=true
export OCM_OCM_STORAGE_PROVIDER_INSECURE=true
export FRONTEND_OCS_INCLUDE_OCM_SHAREES=true
export OCIS_BASE_DATA_PATH=${HOME}/.ocis-10200
export OCIS_CONFIG_DIR=${HOME}/.ocis-10200/config
export OCIS_EVENTS_ENDPOINT=127.0.0.1:10233
export OCIS_LDAP_URI=ldaps://localhost:10235
export OCIS_RUNTIME_PORT=10250
export OCIS_URL=https://federation-ocis-server:10200
export FRONTEND_OCS_LIST_OCM_SHARES=true
export FRONTEND_ENABLE_FEDERATED_SHARING_INCOMING=true
export FRONTEND_ENABLE_FEDERATED_SHARING_OUTGOING=true
export OCIS_ADD_RUN_SERVICES=ocm
export GRAPH_INCLUDE_OCM_SHAREES=true
export OCIS_ENABLE_OCM=true
export APP_PROVIDER_DEBUG_ADDR=127.0.0.1:10165
export APP_PROVIDER_GRPC_ADDR=127.0.0.1:10164
export APP_REGISTRY_DEBUG_ADDR=127.0.0.1:10243
Expand Down