Feature #15000

[controller] publish safe config

Added by Peter Amstutz 4 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
06/07/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
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
15000-docs.png (37.5 KB) 15000-docs.png Tom Clegg, 06/14/2019 06:20 PM

Subtasks

Task #15330: Review 15000-config-apiResolvedTom Clegg


Related issues

Related to Arvados - Story #13648: [Epic] Use one cluster configuration file for all componentsNew

Blocks Arvados - Story #14813: [Workbench2] Use cluster configNew

Associated revisions

Revision 5c1c5e34
Added by Tom Clegg about 1 month ago

Merge branch '15000-config-api'

refs #15000

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Peter Amstutz 4 months ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz 4 months ago

  • Status changed from In Progress to New

#3 Updated by Peter Amstutz 4 months ago

  • Blocks Story #14813: [Workbench2] Use cluster config added

#4 Updated by Peter Amstutz 4 months ago

  • Description updated (diff)

#5 Updated by Tom Morris 4 months ago

  • Story points set to 2.0

#6 Updated by Tom Morris 4 months ago

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

#7 Updated by Ward Vandewege about 2 months ago

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

#8 Updated by Peter Amstutz about 2 months ago

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

#9 Updated by Tom Clegg about 2 months ago

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

#10 Updated by Tom Clegg about 2 months 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.

#12 Updated by Ward Vandewege about 1 month ago

  • Release set to 22

#13 Updated by Peter Amstutz about 1 month 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.

#14 Updated by Tom Clegg about 1 month 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.

#15 Updated by Tom Clegg about 1 month 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/

#16 Updated by Peter Amstutz about 1 month 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.

#17 Updated by Tom Clegg about 1 month 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.)

#18 Updated by Peter Amstutz about 1 month 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.

#19 Updated by Tom Clegg about 1 month 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/

#20 Updated by Tom Clegg about 1 month ago

  • Description updated (diff)

#21 Updated by Tom Clegg about 1 month ago

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

#22 Updated by Peter Amstutz about 1 month 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?

#23 Updated by Tom Clegg about 1 month ago

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

#24 Updated by Peter Amstutz about 1 month ago

Tom Clegg wrote:

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

Thanks, LGTM.

#25 Updated by Tom Clegg about 1 month ago

  • Status changed from New to Resolved

Also available in: Atom PDF