Project

General

Profile

Actions

Feature #15000

closed

[controller] publish safe config

Added by Peter Amstutz about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Story points:
2.0
Release relationship:
Auto

Description

Controller should publish at a well-known API endpoint a subset of the configuration which is not sensitive, which can be accessed without authorization (similar to the discovery doc). It should have the same shape as the full configuration, but only include whitelisted keys/sections.

Sections that should be public:

  • "ExternalURL" for "Services"
  • Collections
    • DefaultReplication
    • DefaultTrashLifetime
    • CollectionVersioning
    • BlobSigningTTL
  • Containers
    • SupportedDockerImageFormats
    • DefaultKeepCacheRAM
    • MaxDispatchAttempts
    • MaxRetryAttempts
    • UsePreemptibleInstances
    • Logging (all)
  • RemoteClusters
  • Workbench

Files

15000-docs.png (37.5 KB) 15000-docs.png Tom Clegg, 06/14/2019 06:20 PM

Subtasks 1 (0 open1 closed)

Task #15330: Review 15000-config-apiResolvedTom Clegg06/07/2019Actions

Related issues

Related to Arvados - Idea #13648: [Epic] Use one cluster configuration file for all componentsResolvedActions
Blocks Arvados - Idea #14813: [Workbench2] Use cluster configResolvedEric Biagiotti07/31/2019Actions
Actions #1

Updated by Peter Amstutz about 5 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz about 5 years ago

  • Status changed from In Progress to New
Actions #3

Updated by Peter Amstutz about 5 years ago

  • Blocks Idea #14813: [Workbench2] Use cluster config added
Actions #4

Updated by Peter Amstutz about 5 years ago

  • Description updated (diff)
Actions #5

Updated by Tom Morris about 5 years ago

  • Story points set to 2.0
Actions #6

Updated by Tom Morris about 5 years ago

  • Target version changed from To Be Groomed to Arvados Future Sprints
Actions #7

Updated by Ward Vandewege almost 5 years ago

  • Related to Idea #13648: [Epic] Use one cluster configuration file for all components added
Actions #8

Updated by Peter Amstutz almost 5 years ago

  • Blocks Feature #14812: [Workbench1] Load configuration from cluster config file added
Actions #9

Updated by Tom Clegg almost 5 years ago

  • Category set to API
  • Assigned To set to Tom Clegg
  • Target version changed from Arvados Future Sprints to 2019-06-19 Sprint
Actions #10

Updated by Tom Clegg almost 5 years ago

15000-config-api @ 68211a7ead5f3cbe1a90b1b7769118a2b3543211 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1292/
  • added Workbench section to config.default.yml
  • everything in config.default.yml is now represented in the Go config struct, too (and there's a test to prevent us from adding other stuff without updating the struct)
  • controller exports the safe parts of the config struct at /arvados/v1/config
  • every config has to be explicitly classified as safe/unsafe in export.go (there's a test that spits out some helpful Go code if anything is missing)

We still need to figure out how Workbench will get its secrets in #14812, since they can't be included in this unauthenticated endpoint. I don't think that needs to hold up this branch, though.

The gofmt test fails because I saved this with gofmt 1.12. I could use a useless-comment hack like we've done in other places, but I'm hoping Jenkins gets updated to 1.12 soon and makes that unnecessary. Jenkins is on Go 1.12. Make sure you have Go 1.12 to pass gofmt tests.

Actions #12

Updated by Ward Vandewege almost 5 years ago

  • Release set to 22
Actions #13

Updated by Peter Amstutz almost 5 years ago

It appears that a duration without a suffix is now a fatal error?

2019-06-12_20:38:29.42932 {"error":"transcoding config data: duration must be given as a string like \"600s\" or \"1h30m\"","level":"info","msg":"exiting","time":"2019-06-12T20:38:29.429230820Z"}

Unfortunately it didn't tell me what key has the problem.

Actions #14

Updated by Tom Clegg almost 5 years ago

Peter Amstutz wrote:

It appears that a duration without a suffix is now a fatal error?

s/now/still/; cluster config has always required units for durations.

Actions #15

Updated by Tom Clegg almost 5 years ago

Updated error to include the bad value, since that's easy. Including the key/line/column from the original YAML would be ideal, but beyond scope here.

15000-config-api @ eaf26ebd285952ec6695270f0eda04ea7bf7e6c6 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1303/

Actions #16

Updated by Peter Amstutz almost 5 years ago

Tom Clegg wrote:

Peter Amstutz wrote:

It appears that a duration without a suffix is now a fatal error?

s/now/still/; cluster config has always required units for durations.

Got it. Then probably this configuration key wasn't previously read by the Go code.

Actions #17

Updated by Tom Clegg almost 5 years ago

Peter Amstutz wrote:

Got it. Then probably this configuration key wasn't previously read by the Go code.

Ah, yes.

(Aside -- perhaps add to #14812? -- now that the keys are all there, we should make RailsAPI call config-dump instead of loading the cluster config file directly, so it gets the same checks/munges as everything else.)

Actions #18

Updated by Peter Amstutz almost 5 years ago

Noticed this in config.default.yml

        ImageID: ami-01234567890abcdef

I don't think it is a good idea to have bogus values in the defaults file.

Also the VM image id feels like an internal implementation detail that maybe shouldn't be exposed without a management token.

I don't think the public config should include Containers.CloudVMs, Containers.JobsAPI or Containers.SLURM.

Git.Repositories and Workbench.RepositoryCache both have filesystem-local paths that don't need to be public.

I see Containers.DispatchPrivateKey ummmmmm

I don't think the "Users" and "Mail" sections should be published.

PostgreSQL includes "ConnectionPool"

TLS.Certificate I'm not sure what is supposed to go there, but if it is a path to a file it shouldn't be published.

Actions #19

Updated by Tom Clegg almost 5 years ago

Peter Amstutz wrote:

I don't think it is a good idea to have bogus values in the defaults file.

Agreed. Changed to "".

Now that we've established this won't be used by system components at all, I've moved the threshold from "non-secret configs needed by components" to "configs needed by clients".

15000-config-api @ 3f45203069cda0b906ceeaf64d1ac8146a085895 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1306/

Actions #20

Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)
Actions #21

Updated by Tom Clegg almost 5 years ago

  • Blocks deleted (Feature #14812: [Workbench1] Load configuration from cluster config file)
Actions #22

Updated by Peter Amstutz almost 5 years ago

I noticed that some services have {ExternalURL: "-"} which I assume means it is an internal service that isn't supposed to have an external URL (as opposed to the services with {ExternalURL: ""} where the external URL is missing from the configuration...), how hard would it be to just filter those out? What if an admin fills in ExternalURL by accident?

Actions #23

Updated by Tom Clegg almost 5 years ago

15000-config-api @ d768d4e5b5b61949aeb2bb2e473c4ceec93957f1 adds a step to the install docs to check for accidentally-published stuff:

Actions #24

Updated by Peter Amstutz almost 5 years ago

Tom Clegg wrote:

15000-config-api @ d768d4e5b5b61949aeb2bb2e473c4ceec93957f1 adds a step to the install docs to check for accidentally-published stuff:

Thanks, LGTM.

Actions #25

Updated by Tom Clegg almost 5 years ago

  • Status changed from New to Resolved
Actions

Also available in: Atom PDF