Add prometheus metrics for dockerregistry #12711

legionus · 2017-01-30T13:54:11Z

Example:


# HELP http_request_duration_microseconds The HTTP request latencies in microseconds.
# TYPE http_request_duration_microseconds summary
http_request_duration_microseconds{handler="openshift",quantile="0.5"} 12060.892
http_request_duration_microseconds{handler="openshift",quantile="0.9"} 26831.134
http_request_duration_microseconds{handler="openshift",quantile="0.99"} 92603.647
http_request_duration_microseconds_sum{handler="openshift"} 817404.5250000001
http_request_duration_microseconds_count{handler="openshift"} 52

# HELP http_request_size_bytes The HTTP request sizes in bytes.
# TYPE http_request_size_bytes summary
http_request_size_bytes{handler="openshift",quantile="0.5"} 345
http_request_size_bytes{handler="openshift",quantile="0.9"} 583
http_request_size_bytes{handler="openshift",quantile="0.99"} 3114
http_request_size_bytes_sum{handler="openshift"} 25907
http_request_size_bytes_count{handler="openshift"} 52

# HELP http_response_size_bytes The HTTP response sizes in bytes.
# TYPE http_response_size_bytes summary
http_response_size_bytes{handler="openshift",quantile="0.5"} 87
http_response_size_bytes{handler="openshift",quantile="0.9"} 2739
http_response_size_bytes{handler="openshift",quantile="0.99"} 2740
http_response_size_bytes_sum{handler="openshift"} 25128
http_response_size_bytes_count{handler="openshift"} 52

# HELP http_requests_total Total number of HTTP requests made.
# TYPE http_requests_total counter
http_requests_total{code="200",handler="openshift",method="get"} 19
http_requests_total{code="200",handler="openshift",method="head"} 6
http_requests_total{code="201",handler="openshift",method="post"} 1
http_requests_total{code="201",handler="openshift",method="put"} 4
http_requests_total{code="202",handler="openshift",method="patch"} 2
http_requests_total{code="202",handler="openshift",method="post"} 2
http_requests_total{code="400",handler="openshift",method="put"} 2
http_requests_total{code="401",handler="openshift",method="get"} 14
http_requests_total{code="404",handler="openshift",method="head"} 2

# HELP openshift_registry_request_duration_microseconds Request latency summary in microseconds for each operation
# TYPE openshift_registry_request_duration_microseconds summary
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="blobstore.serveblob",quantile="0.5"} 316
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="blobstore.serveblob",quantile="0.9"} 385
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="blobstore.serveblob",quantile="0.99"} 385
openshift_registry_request_duration_microseconds_sum{name="tmp/busybox",operation="blobstore.serveblob"} 1180
openshift_registry_request_duration_microseconds_count{name="tmp/busybox",operation="blobstore.serveblob"} 3
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="blobstore.stat",quantile="0.5"} 466
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="blobstore.stat",quantile="0.9"} 632
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="blobstore.stat",quantile="0.99"} 632
openshift_registry_request_duration_microseconds_sum{name="tmp/busybox",operation="blobstore.stat"} 6633
openshift_registry_request_duration_microseconds_count{name="tmp/busybox",operation="blobstore.stat"} 7
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="manifestservice.get",quantile="0.5"} 10427
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="manifestservice.get",quantile="0.9"} 10427
openshift_registry_request_duration_microseconds{name="tmp/busybox",operation="manifestservice.get",quantile="0.99"} 10427
openshift_registry_request_duration_microseconds_sum{name="tmp/busybox",operation="manifestservice.get"} 22958
openshift_registry_request_duration_microseconds_count{name="tmp/busybox",operation="manifestservice.get"} 2

This is only one part of implemented metrics. Currently collected metrics about the process state, memstats, the garbage collector metrics.

legionus · 2017-01-30T14:01:46Z

@mfojtik @miminar @smarterclayton PTAL

mfojtik · 2017-01-30T14:44:56Z

@legionus I assume the metrics will only be visible for the cluster admin user, right?

@jcantrill FYI (this might be interesting for metrics)

legionus · 2017-01-30T14:48:58Z

I assume the metrics will only be visible for the cluster admin user, right?

@mfojtik No. Right now /extensions/v2/metrics open for anonymous access. But we can restrict it.

mfojtik · 2017-01-30T14:50:27Z

@legionus you probably don't want to expose these data to anonymous users. also if we are going to include per-repo metrics (num of pulls), you want to make that info available only for admin (or the user that has access to the repo?).

legionus · 2017-01-30T14:59:34Z

[registry] Registry Metrics

deads2k · 2017-01-30T16:04:06Z

Metrics about response time and registry performance should not be exposed to end users. They can and should be restricted to cluster-admins or another privileged ops kind of role.

Where do you envision per-repository metrics going, what would you expose (and how), and how do you plan to restrict access?

legionus · 2017-01-30T16:13:20Z

@mfojtik @deads2k Sure. After discussion with @enj I planning to make some virtual object (registrymetrics for example) and use SAR to check access to it. I don't want to give any access in addition to receiving metrics.

legionus · 2017-01-30T16:25:12Z

@deads2k I know it's ugly, but I don't know other way how to control access without giving any access to other resources.

jcantrill · 2017-01-30T16:50:14Z

cc @mwringe

smarterclayton · 2017-01-30T22:44:54Z

The approach established for app metrics is probably what we should follow - something associated with the registry container definition that is a shared secret that can be accessed by the metrics endpoint.

smarterclayton · 2017-01-30T22:47:32Z

Prometheus recommends being careful on high cardinality metrics. A good read is here: https://prometheus.io/docs/practices/instrumentation/

Will look more

smarterclayton · 2017-01-30T22:52:02Z

Some metrics I would expect to be able to tell end users about:

How many times has anyone downloaded this image (we need to correlate that to one of the operations trivially)
How long did it take to download this image
How many requests to openshift API did we make over this period
Which openshift API calls took the most time to complete for the registry
Do we have high latency between the registry and etcd
When downloading blobs, what was the aggregate rate
How many bytes of blob data was served per unit time

mfojtik · 2017-02-01T15:00:47Z

So we agreed we are not going to expose any data to end-users, but this endpoint will be for operator kind of users.
We have to talk to operations team what kind of metrics they would like to have and pick ones that are cheap to provide.
Then talk to @mwringe about advertising the prometheus endpoint to hawkular agent (pod annotations?).

The auth mechanism will be shared secret I assume, so think endpoint won't use the auth middleware.

dmage · 2017-03-10T12:44:03Z

pkg/dockerregistry/server/metrics/metrics.go

+var registerMetrics sync.Once
+
+// Register all metrics.
+func Register() {


There is no reason to make this function public nor to use sync.Once: it's called from init() and should not be called manually.

We actually don't want to call things from init() unless we have to. That registers metrics globally and pollutes the namespace. We should only log metrics when we're going to use them.

@smarterclayton Not a problem. I call Register() from pkg/cmd/dockerregistry.Execute() if metrics enabled.

legionus · 2017-03-28T14:58:17Z

@mfojtik @miminar @smarterclayton I added an additional section in the config.yml because this functionality is not specific to any particular middleware. I added the option to enable metrics collection and the option to specify a shared secret.
It looks better for you ?

miminar

I'm not familiar with the prometheus. I'll take a look what it is and return back.

Tests are welcome.

miminar · 2017-03-28T15:07:26Z

pkg/cmd/dockerregistry/dockerregistry.go

-		context.GetLogger(app).Infof("listening on %v", config.HTTP.Addr)
-		if err := http.ListenAndServe(config.HTTP.Addr, handler); err != nil {
+	if extraConfig.Metrics.Enabled {
+		log.Debugf("configured metrics endpoint at \"/extensions/v2/metrics\"")


context.GetLogger(app)

miminar · 2017-03-28T15:09:02Z

pkg/cmd/dockerregistry/dockerregistry.go


 	ctx := context.Background()
-	ctx, err = configureLogging(ctx, config)
+	ctx = server.WithConfiguration(ctx, extraConfig)


Why does it need to be set on the context? Can it be passed to RegisterMetricHandler instead?

Resolved eye-to-eye. If we're going with the top-level openshift section, this is the probably the only sensible way of passing the configuration to the registry/repository middleware.

Nevertheless, I'd still want to avoid using the configuration from the context wherever possible. Let's use server.AccessControllerParams to configure the auth handler and pass the other configuration options directly to any other object we initialize here.

@miminar Please no. Otherwise we have to add that to the repository middleware options as well. It already contains many parameters. I don't want to adв internal options to repository middleware.
Please take a look at my new changes.

I don't want to adв internal options to repository middleware.

@legionus It's fine to use the config object in repository middleware since we don't have a better way to pass the config there. On the other hand, there's no sense to have auth module differentiate between parameters passed via configuration file, parameters loaded from environment variables and parameters created on the fly. Those are assumptions that may easily become obsolete. Moreover, it makes unit tests hard to follow.

I still think we should pre-process the config and set server.AccessControllerParams accordingly. And do similar for all the other wrappers but repository. I won't block the review because of this though.
@dmage What is your opinion?

I think passing variables through a context is a code smell, like a having global variables. It's very convenient to write this way (you can get data from anywhere), but it really complicates the code.

But in this case I don't care, I want to try to refactor the repository struct, after which there can emerge a place to pass the config. But it's too scary to do that without the good test coverage.

Let's keep it as is. We can further tune it in a follow-up while fixing #13568. Let's focus on the prometheus here.

miminar · 2017-03-28T15:15:39Z

pkg/dockerregistry/server/auditblobstore.go

 	return err
 }

 type blobWriter struct {
 	distribution.BlobWriter
+
+	repo *repository


Aren't we avoiding passing the repository everywhere? This could be just the Named.

miminar · 2017-03-28T15:16:14Z

pkg/dockerregistry/server/auditblobstore.go

+	if audit.LoggerExists(ctx) {
+		audit.GetLogger(ctx).LogResult(err, "BlobStore.Delete")
+	}
+
 	return err
 }

 type blobWriter struct {


nit: Could this be renamed to auditBlobWriter?

miminar · 2017-03-28T15:20:03Z

pkg/dockerregistry/server/auditmanifestservice.go

 )

 // auditManifestService wraps a distribution.ManifestService to track operation result and
 // write it in the audit log.
 type auditManifestService struct {
 	manifests distribution.ManifestService
+	repo      *repository


Just the name please.

miminar · 2017-03-28T15:28:25Z

pkg/dockerregistry/server/metrichandler.go

+			},
+		}
+	}
+	extensionsRouter := app.NewRoute().PathPrefix("/extensions/v2/").Subrouter()


Could you turn this into a constant and use it on all the other places as well?

miminar · 2017-03-28T15:29:56Z

pkg/dockerregistry/server/metrics/dispatcher.go

+
+	"github.com/docker/distribution/registry/handlers"
+
+	gorillahandlers "github.com/gorilla/handlers"


These could be squashed.

miminar · 2017-03-28T15:32:53Z

pkg/dockerregistry/server/auditblobstore.go

-	audit.GetLogger(ctx).Log("BlobStore.Stat")
+	defer metrics.NewTimer(metrics.RegistryAPIRequests, []string{"blobstore.stat", b.repo.Named().Name()}).Stop()
+
+	if audit.LoggerExists(ctx) {


I'd rather have audit and metric wrappers separated.

miminar · 2017-03-28T15:36:48Z

images/dockerregistry/config.yml

@@ -35,3 +35,7 @@ middleware:
        blobrepositorycachettl: 10m
  storage:
    - name: openshift
+openshift:


Upstream has a configuration section called reporting. This could be probably nested there - similarly to the auth.

Resolved eye to eye. @legionus convinced me that this is a preferred place for our current and future configuration. As long as it's versioned properly. In the future, we'd like to move middleware section here completely. The openshift section can co-exist with the middleware entries for now since they have separate areas of interest. The middleware section gets out of hand already so I'd prefer to do the configuration refactoring sooner rather than later. I'll open an issue to track this.

miminar · 2017-03-28T15:38:10Z

pkg/dockerregistry/server/auditblobstore.go

 	err := b.store.Delete(ctx, dgst)
-	audit.GetLogger(ctx).LogResult(err, "BlobStore.Delete")
+
+	if audit.LoggerExists(ctx) {


Two many ifs. I think separated metricBlobStore would really make it more pleasant to the eye.

legionus · 2017-03-29T21:43:29Z

@miminar Most of your commens addressed.

miminar · 2017-04-03T14:15:47Z

pkg/cmd/dockerregistry/dockerregistry.go

 	}

 	// TODO add https scheme
-	adminRouter := app.NewRoute().PathPrefix("/admin/").Subrouter()
+	adminRouter := app.NewRoute().PathPrefix(api.AdminPrefix).Subrouter()


This is missing the terminating slash. Quoting PathPrefix's GoDoc:

// Note that it does not treat slashes specially ("/foobar/" will be matched by // the prefix "/foo") so you may want to use a trailing slash here.

@miminar Fixed in dockerregistry/server/api

miminar · 2017-04-03T14:16:18Z

pkg/dockerregistry/server/metrichandler.go

+			},
+		}
+	}
+	extensionsRouter := app.NewRoute().PathPrefix(api.ExtensionsPrefix).Subrouter()


Add terminating slash.

@miminar Fixed in dockerregistry/server/api

miminar · 2017-04-03T14:18:54Z

pkg/dockerregistry/server/metrics/metricsblobstore.go

+	"github.com/docker/distribution/digest"
+)
+
+// BlobStore wraps a distribution.BlobStore to collect statistic


miminar · 2017-04-03T14:23:09Z

pkg/dockerregistry/server/api/routes.go

+
+	AdminPath      = "/blobs/{digest:" + reference.DigestRegexp.String() + "}"
+	SignaturesPath = "/{name:" + reference.NameRegexp.String() + "}/signatures/{digest:" + reference.DigestRegexp.String() + "}"
+	MetricsPath    = "/metrics"


The leading slashes should be probably moved to the prefixes above.

miminar · 2017-04-03T15:03:45Z

pkg/dockerregistry/server/metrics/metrics.go

+}
+
+func (m *metricTimer) Stop() {
+	m.collector.WithLabelValues(m.labels...).Observe(float64(time.Since(m.startTime) / time.Microsecond))


WithLabelValues can panic. Could GetMetricWithLabelValues be used instead?

@miminar and what I gona do in this function if err != nil ?

This function is used as a defer and I have at this moment no other possibility as to panic.

I was thinking about logging the error. If the probability of panicking is close to zero, this is ok.

miminar · 2017-04-03T15:29:54Z

pkg/dockerregistry/server/metrics/dispatcher.go

+	gorillahandlers "github.com/gorilla/handlers"
+)
+
+func Dispatcher(ctx *handlers.Context, r *http.Request) http.Handler {


miminar · 2017-04-03T15:32:14Z

pkg/dockerregistry/server/configuration/configuration.go

+	Secret  string `yaml:"secret"`
+}
+
+func Parse(rd io.Reader) (*configuration.Configuration, *Configuration, error) {


miminar · 2017-04-03T15:36:45Z

pkg/dockerregistry/server/configuration/configuration.go

+		return nil, nil, err
+	}
+
+	return dockerConfig, &config.Openshift, nil


What happens if this encounters newer version than supported?

@miminar I'm not sure I understand the question but I will try to answer. The docker developers always return the configuration.Configuration structure. They can convert old config to this structure, but they can't return something else.

My last remark to the configuration concerns the version. We should define our own and panic if the configuration file defines any other.

miminar · 2017-04-03T15:43:06Z

pkg/dockerregistry/server/configuration/configuration.go

+
+	p := configuration.NewParser("registry", []configuration.VersionedParseInfo{
+		{
+			Version: configuration.CurrentVersion,


Shouldn't this be different from upstream version?

@miminar No. This is required to parse config file. Without it you will get Unsupport version error. Here, the entire upstream configuration is not important to us at all because we want to parse our own part of config file.

@miminar But you are right and here we must specify the current version of config file.

miminar · 2017-04-03T15:57:47Z

pkg/cmd/dockerregistry/dockerregistry.go


 	ctx := context.Background()
-	ctx, err = configureLogging(ctx, config)
+	ctx = server.WithConfiguration(ctx, extraConfig)


I don't want to adв internal options to repository middleware.

@legionus It's fine to use the config object in repository middleware since we don't have a better way to pass the config there. On the other hand, there's no sense to have auth module differentiate between parameters passed via configuration file, parameters loaded from environment variables and parameters created on the fly. Those are assumptions that may easily become obsolete. Moreover, it makes unit tests hard to follow.

I still think we should pre-process the config and set server.AccessControllerParams accordingly. And do similar for all the other wrappers but repository. I won't block the review because of this though.
@dmage What is your opinion?

legionus · 2017-04-05T14:35:31Z

@smarterclayton since @mfojtik asked me to add this PR to the 3.6, please take a look again.

legionus · 2017-04-05T18:54:39Z

[test] #13644

smarterclayton · 2017-04-05T21:09:48Z

Will try and take a look tomorrow.

…

On Wed, Apr 5, 2017 at 2:02 PM, Alexey Gladkov ***@***.***> wrote: [test] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p3ZyU_bCApFIqnbqIqBRQsHKtXUmks5rs4LfgaJpZM4LxbRp> .

mfojtik · 2017-04-06T09:43:30Z

@smarterclayton my reason this should be in 3.6 is basically the ops team being able to monitor health of the registry and make scaling decisions. we don't have to make this perfect right now and we can always add more metrics to this as we go.

legionus · 2017-04-10T09:31:13Z

@mfojtik @smarterclayton ping

mfojtik · 2017-04-10T09:32:11Z

LGTM

Will wait for Clayton to have final word.

smarterclayton · 2017-04-12T05:26:46Z

pkg/dockerregistry/server/auth.go

+	config := ConfigurationFrom(ctx)
+	if config.Metrics.Enabled {
+		return config.Metrics.Secret == token
+	}


If we're already doing auth from the registry, we should use the SAR check instead of a secret. That allows us to then use the same policy the apiservers do.

@smarterclayton In the beginning, we discussed this already and came to the decision that we would use the shared secret for metrics endpoint because router doing it in same way.

#12711 (comment)
#12711 (comment)

I don't agree with that. If you are already doing auth via the master, we want metrics to use that behavior. Static secret is only for things that aren't integrated with master auth (which is a limited set, but registry is under)

Actually, does the registry support cert auth (is it using the correct filter for that)? If not, I'm ok with static secret.

If you are already doing auth via the master, we want metrics to use that behavior. Static secret is only for things that aren't integrated with master auth (which is a limited set, but registry is under)

We have already discussed this. Registry metrics are not part of the master API. The metrics belongs to the registry server only. You propose to make some virtual object (registrymetrics for example) and use SAR to check access to it ?

Actually, does the registry support cert auth (is it using the correct filter for that)? If not, I'm ok with static secret.

The registry does not have a ready solution for it. Additionally, cert authentication will work only if HTTPS enabled. It means that it will be impossible to restrict access to metrics in an insecure registry.

@mfojtik @smarterclayton I propose to leave the possibility of shared secret authentication and add cert authentication later (in 3.7) because we don't have time to add it for 3.6. ok?

i'm oking with it move to 3.7, make sure there is an issue tracking it.

smarterclayton · 2017-04-12T05:28:11Z

pkg/dockerregistry/server/metrics/metrics.go

+			Namespace: registryNamespace,
+			Subsystem: registrySubsystem,
+			Name:      "request_duration_microseconds",
+			Help:      "Request latency summary in microseconds for each operation",


Standard Prometheus convention is for everythjng to be in seconds now. See the Prometheus docs - there's a section that recommends naming and pattern

@smarterclayton Changed it to seconds. But http_request_duration_microseconds will remain in place. We have really old version of github.com/prometheus/client_golang/prometheus in the vendor and InstrumentHandler uses microseconds as unit, which is deprecated in new version and should be replaced by seconds.

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

smarterclayton · 2017-04-12T20:21:22Z

I'm ok with shared secret for now. Just to confirm - it can be set via environment variable using normal Docker registry configuration override rules?

…

On Wed, Apr 12, 2017 at 3:56 PM, Alexey Gladkov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/dockerregistry/server/auth.go <#12711 (comment)>: > @@ -535,3 +545,11 @@ func verifyPruneAccess(ctx context.Context, client client.SubjectAccessReviews) } return nil } + +func isMetricsBearerToken(ctx context.Context, token string) bool { + config := ConfigurationFrom(ctx) + if config.Metrics.Enabled { + return config.Metrics.Secret == token + } If you are already doing auth via the master, we want metrics to use that behavior. Static secret is only for things that aren't integrated with master auth (which is a limited set, but registry is under) We have already discussed this <#12711 (comment)>. Registry metrics are not part of the master API. The metrics belongs to the registry server only. You propose to make some virtual object ( registrymetrics for example) and use SAR to check access to it ? Actually, does the registry support cert auth (is it using the correct filter for that)? If not, I'm ok with static secret. The registry does not have a ready solution for it. Additionally, cert authentication will work only if HTTPS enabled. It means that it will be impossible to restrict access to metrics in an insecure registry. @mfojtik <https://github.com/mfojtik> @smarterclayton <https://github.com/smarterclayton> I propose to leave the possibility of shared secret authentication and add cert authentication later (in 3.7) because we don't have time to add it for 3.6. ok? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p2elg5j-UacRy9nX0fikdWMjKbcgks5rvSx6gaJpZM4LxbRp> .

legionus · 2017-04-12T20:30:12Z

I'm ok with shared secret for now. Just to confirm - it can be set via environment variable using normal Docker registry configuration override rules?

@smarterclayton Yes.

mfojtik · 2017-04-19T13:47:51Z

@smarterclayton [merge] -ing this as we need this for perf testing. i think it will be easy to add more metrics as we go.

legionus · 2017-04-20T09:28:34Z

Installer failed [merge]

legionus · 2017-04-20T12:41:20Z

Ansible Task Failed [merge]

legionus · 2017-04-20T16:17:08Z

flake #13271 [merge]

legionus · 2017-04-20T20:41:20Z

Installer failed [merge]

legionus · 2017-04-23T18:53:22Z

[merge]

legionus · 2017-04-24T07:25:25Z

AWS EC2 problems [merge]

legionus · 2017-04-24T10:24:13Z

[merge]

smarterclayton · 2017-04-24T15:05:03Z

[merge]

…

On Mon, Apr 24, 2017 at 6:45 AM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/merge FAILURE ( https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/456/) (Base Commit: 53a4d1b <53a4d1b> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p6hzjkoyoiBSLGJvboZ10TCVD289ks5rzH1DgaJpZM4LxbRp> .

mfojtik · 2017-04-24T15:05:59Z

[test] in meanwhile... if we can get the test green, we can pretest-merge this...

openshift-bot · 2017-04-24T15:13:28Z

Evaluated for origin test up to 021cf0c

openshift-bot · 2017-04-24T16:44:41Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/903/) (Base Commit: d41996f)

openshift-bot · 2017-04-24T17:29:35Z

Evaluated for origin merge up to 021cf0c

openshift-bot · 2017-04-24T17:29:44Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/461/) (Base Commit: 576f2c5) (Image: devenv-rhel7_6174)

smarterclayton · 2017-04-24T22:59:37Z

images/dockerregistry/config.yml

+  version: 1.0
+  metrics:
+    enabled: false
+    secret: TopSecretToken


This is incredibly unsafe and needs to be fixed :)

We should not check in default secrets - instead this should be empty, and the user MUST set both secret and enabled in order to turn it on. That should be done by oc adm registry

It should be impossible for a user to accidentally turn this on and have the default secret in place.

agree, this is abad practice

@legionus FYI

More examples:

storage: azure: accountname: accountname accountkey: base64encodedaccountkey s3: accesskey: awsaccesskey secretkey: awssecretkey swift: username: username password: password oss: accesskeyid: accesskeyid accesskeysecret: accesskeysecret health: http: headers: Authorization: [Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==] proxy: username: username password: password

@smarterclayton @mfojtik This config file is full of plaintext passwords even if we can fix this. We have already discussed the possibility of using secrets for this file and came to the conclusion that if this is done, then the entire configuration will have to be put there.

agree, this is abad practice

Don't tell me. In the beginning, I had not even considered this decision. You asked for a shared secret.

@legionus the problem is the fact that 'secret: TopSecretToken' is hardcoded and looks like nobody will change it by default, so we will end up with tons of clusters with this secret :-)

@mfojtik Now I see :) How about:

secret: Place-real-secret-here

?

same problem :-) how about not having it enabled by default and update oc registry to allow to set this up?

legionus added the component/imageregistry label Jan 30, 2017

legionus self-assigned this Jan 30, 2017

legionus force-pushed the dockerregistry-metrics-prometheus branch from cce6b7d to 1cc1c07 Compare January 30, 2017 13:56

legionus force-pushed the dockerregistry-metrics-prometheus branch from 1cc1c07 to 6a148e1 Compare February 1, 2017 16:03

dmage reviewed Mar 10, 2017

View reviewed changes

legionus force-pushed the dockerregistry-metrics-prometheus branch from fca31a5 to 967600d Compare March 10, 2017 13:13

legionus force-pushed the dockerregistry-metrics-prometheus branch from 967600d to 2cbfe94 Compare March 28, 2017 14:50

miminar suggested changes Mar 28, 2017

View reviewed changes

miminar mentioned this pull request Mar 29, 2017

Refactor registry configuration file #13568

Closed

legionus force-pushed the dockerregistry-metrics-prometheus branch from 2cbfe94 to 8809b42 Compare March 29, 2017 21:36

miminar suggested changes Apr 3, 2017

View reviewed changes

legionus force-pushed the dockerregistry-metrics-prometheus branch 4 times, most recently from 52e4106 to d116155 Compare April 4, 2017 13:51

legionus force-pushed the dockerregistry-metrics-prometheus branch from 0453916 to 998f7c8 Compare April 5, 2017 15:34

smarterclayton suggested changes Apr 12, 2017

View reviewed changes

Add prometheus metrics for dockerregistry

021cf0c

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

legionus force-pushed the dockerregistry-metrics-prometheus branch from 998f7c8 to 021cf0c Compare April 12, 2017 14:52

legionus mentioned this pull request Apr 12, 2017

Dockerregistry metrics master api #13738

Closed

openshift-bot merged commit ff87ac2 into openshift:master Apr 24, 2017

smarterclayton reviewed Apr 24, 2017

View reviewed changes

pweil- mentioned this pull request Jun 26, 2017

Prometheus metrics for docker registry & haproxy #3916

Closed


		"github.com/docker/distribution/registry/handlers"

		gorillahandlers "github.com/gorilla/handlers"

Add prometheus metrics for dockerregistry #12711

Add prometheus metrics for dockerregistry #12711

Conversation

legionus commented Jan 30, 2017 • edited Loading

legionus commented Jan 30, 2017

mfojtik commented Jan 30, 2017

legionus commented Jan 30, 2017

mfojtik commented Jan 30, 2017

legionus commented Jan 30, 2017

deads2k commented Jan 30, 2017

legionus commented Jan 30, 2017 • edited Loading

legionus commented Jan 30, 2017

jcantrill commented Jan 30, 2017

smarterclayton commented Jan 30, 2017

smarterclayton commented Jan 30, 2017

smarterclayton commented Jan 30, 2017

mfojtik commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionus commented Mar 28, 2017

miminar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionus commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionus commented Apr 5, 2017

legionus commented Apr 5, 2017

smarterclayton commented Apr 5, 2017 via email

mfojtik commented Apr 6, 2017

legionus commented Apr 10, 2017

mfojtik commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Apr 12, 2017 via email

legionus commented Apr 12, 2017

mfojtik commented Apr 19, 2017 • edited Loading

legionus commented Apr 20, 2017

legionus commented Apr 20, 2017

legionus commented Jan 30, 2017 •

edited

Loading

legionus commented Jan 30, 2017 •

edited

Loading

miminar Apr 4, 2017 •

edited

Loading

mfojtik Apr 19, 2017 •

edited

Loading

mfojtik commented Apr 19, 2017 •

edited

Loading

openshift-bot commented Apr 24, 2017 •

edited

Loading