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

Bump upstream #39

Merged
merged 42 commits into from Oct 10, 2019
Merged

Conversation

s-urbaniak
Copy link

As discussed in the engineering sync this bumps openshift/master to the current master state of upstream to be able to iterate on user workload monitoring.

/cc @lilic @paulfantom @brancz @pgier

mrunge and others added 30 commits September 16, 2019 16:26
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
As far as I can see, changing the ownership of the p-c-m binary should be unnessecary.
Droping the chown removes a potential attack vector as a non-privileged user
might be able alter content of the binary during runtime.

Signed-off-by: Manuel Rüger <manuel@rueg.eu>
It's bit more verbose when you see conditional above the script.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
prometheus-config-reloader: Remove unneeded Ownership change
…al-probes

Add exec probes with wget localhost:9090 to Prometheus if listenLocal
Update prometheus dependency to v2.12.0 to fix validation failure for…
scripts/jsonnet: Bump jsonnet and golangci-lint versions
…ne parameter

This adds two new command line parameters to prometheus operator:

--prometheus-instance-namespaces: Namespaces where Prometheus custom resources
and corresponding Secrets, Configmaps and StatefulSets are watched/created.
If set this takes precedence over --namespaces or --deny-namespaces
for Prometheus custom resources.

--alertmanager-instance-namespaces: Namespaces where Alertmanager custom resources
and corresponding StatefulSets are watched/created.
If set this takes precedence over --namespaces or --deny-namespaces
for Alertmanager custom resources.

This allows fine grained configuration of reconcilation for prometheus/alertmanager
instances to certain namespaces while allow- or deny-listing other namespaces for less
critical custom resources like ServiceMonitors, PodMonitors, etc.
…p-am-instances

cmd/operator: add [prometheus,alertmanager]-instance-namespaces cmdline parameter
…rometheus-operator#2728)

* Add ListenLocal option for Thanos spec to listen on local loopback

Resolves prometheus-operator#2435

* Generated files
To configure a bearer token users could only specify a file path in the
service monitor, pointing to a bearer token file in the Prometheus
container. This enables hostile users, being able to configure a service
monitor and controlling the scrape target, to retrieve arbitrary files
in the Prometheus container.

In cases where users can not be trusted, this patch adds an option to
disallow the above file path specification and replaces it by a secret
reference. This secret has to be in the same namespace as the service
monitor, shrinking the attack vector.

pkg/prometheus: Add option to deny file system access through service monitors

ArbitraryFSAccessThroughSMsConfig enables users to configure, whether
a service monitor selected by the Prometheus instance is allowed to use
arbitrary files on the file system of the Prometheus container. This is
the case when e.g. a service monitor specifies a BearerTokenFile in an
endpoint. A malicious user could create a service monitor
selecting arbitrary secret files in the Prometheus container. Those
secrets would then be send with a scrape request by Prometheus to a
malicious target. Denying the above would prevent the attack, users can
instead use the BearerTokenSecret field.

test/basic-auth-test-app: Add mTLS endpoint

pkg/prometheus: Enable users to configure tls from secret

pkg/prometheus/operator: Validate TLS configs before retrieving assets

Before retrieving TLS assets from Kubernetes secrets for a given service
monitor, make sure the user did not specify both file and secret
reference, e.g. both `CAFile` and `CASecret`.

test: Rename basic-auth-test-app to instrumented-sample-app

Given that the basic-auth-test-app not only supports basic auth, but
also bearer token as well as tls authentication, this patch renames the
app to a more generic name.

test/e2e/prometheus_test: Test ArbitraryFSAccessThroughSM option for tls

The Prometheus custom resource has the option to disable arbitrary
filesystem access configured through service monitors. This commit adds
an end-to-end test for this option in combination with the TLS
configuration via files or secret references in service monitors.

pkg/prometheus/operator: Move check for arbitrary fs access into func
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@s-urbaniak
Copy link
Author

We have a valid e2e failure :-)

I1009 15:21:38.941350       1 operator.go:322] Updating ClusterOperator status to failed. Err: running task Updating Prometheus-k8s failed: reconciling Prometheus object failed: creating Prometheus object failed: Prometheus.monitoring.coreos.com "k8s" is invalid: spec.arbitraryFSAccessThroughSMs: Required value
E1009 15:21:38.949595       1 operator.go:267] Syncing "openshift-monitoring/cluster-monitoring-config" failed
E1009 15:21:38.949689       1 operator.go:268] sync "openshift-monitoring/cluster-monitoring-config" failed: running task Updating Prometheus-k8s failed: reconciling Prometheus object failed: creating Prometheus object failed: Prometheus.monitoring.coreos.com "k8s" is invalid: spec.arbitraryFSAccessThroughSMs: Required value

@paulfantom we should default arbitraryFSAccessThroughSMs to true, so we don't break existing installations.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@paulfantom
Copy link

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2019
@paulfantom
Copy link

arbitraryFSAccessThroughSMs is by default set to false (source: https://github.com/coreos/prometheus-operator/blob/master/Documentation/api.md#arbitraryfsaccessthroughsmsconfig) which means we are allowing filesystem access, which is what had previously. The problem here is that this field is now a required one, which I will change soon.

@s-urbaniak
Copy link
Author

/hold

@s-urbaniak
Copy link
Author

@paulfantom sounds good, thanks!

@paulfantom
Copy link

Fix in prometheus-operator#2797

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2019
@s-urbaniak
Copy link
Author

@lilic @paulfantom please re-lgtm, let's also see what e2e says :)

Copy link

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm
🎉

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, LiliC, paulfantom, s-urbaniak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [LiliC,brancz,paulfantom,s-urbaniak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@s-urbaniak
Copy link
Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2019
@openshift-merge-robot openshift-merge-robot merged commit 75d8818 into openshift:master Oct 10, 2019
@s-urbaniak s-urbaniak deleted the bump-upstream branch October 10, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet