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

MON-2967: CMO deploys monitoring console-plugin #1890

Merged

Conversation

sthaha
Copy link
Contributor

@sthaha sthaha commented Feb 14, 2023

This PR deploy the new monitoring console plugin

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2023
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2023
@sthaha
Copy link
Contributor Author

sthaha commented Feb 14, 2023

Testing it locally

  go run ./cmd/operator/... \
    -images=prometheus-operator=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1d704524e203614a92f903ed50247546dcf854a6e983edb1f2c7e9268a6b3247 \
    -images=prometheus-config-reloader=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:0b682f7e323cafc28bd09fa60903f3ae9e073aeb3c04b194f7401432e50a6c31 \
    -images=prometheus-operator-admission-webhook=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:cb86ba5d99371e93eced2f86bfb20d041668cabb7d09501d24fd8ea0617402e4 \
    -images=configmap-reloader=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:7ded11abcea8ed136acf36d6a5bacd218dffadac564ef0f5bce150620c992879 \
    -images=prometheus=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:b10ab06cd40456afe5f928c2642832ea92a8ad05b0636b9286b82d39a66eb2d4 \
    -images=alertmanager=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:aaa094ab3806e1688b381459841ace8bf528b4965f8791cb0353546a518ba543 \
    -images=oauth-proxy=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:5925f28a7682ef5cb91d4a3e3c9f6877ccc339ddb3533ad6376771314a79cb5a \
    -images=node-exporter=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:3576c4bdb825087fc187266bfc13ac84c644ce18779fe864adc03303fab67063 \
    -images=kube-state-metrics=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:7d236b37ff6ef642a63a15ecce4951705e434c340ee6914f432eddc551ca66b8 \
    -images=openshift-state-metrics=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:8a254827cbaf147d18fdd55e381dcec8abb4ea622a1d5a52b23b3cd71f16715d \
    -images=kube-rbac-proxy=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:ec61024e2c37e5aba93593220b12ba91a3ba4f547b2562a36539ff1d9df74a23 \
    -images=telemeter-client=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:570c1c5b42268585f572478e5d22b85c7c23e42d96f71c0af95c31d54104cd67 \
    -images=prom-label-proxy=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:9a2f06206b2a3ff6fdd4d4d3a9b4b6ad43ec595fa8e765c47f5565633ba27ecf \
    -images=k8s-prometheus-adapter=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:a61b6f8fab1abbe03db8cd221179dd01c4eb622e981b93b76ca58a8fb77158c0 \
    -images=thanos=quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:2327c5c575e389003c7b3d3ffb0a95a7ecaccac6403cf82d50c35eb57716fba1 \
    -images=console-plugin=quay.io/anpicker/monitoring-plugin:4-13-test  # 👈 this is new \
    -assets assets/ \
    -telemetry-config /tmp/telemetry-config.yaml \
    -kubeconfig <path>/to/kubeconfig # 👈 replace this\ 
    -namespace=openshift-monitoring \
    -configmap=cluster-monitoring-config \
    -logtostderr=true \
    -v=5 2>&1 | tee tmp/$(git branch --show-current)/cbot.log

@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch from 91ef19f to 09fc64f Compare February 14, 2023 07:04
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

2 questions/remarks from me:

  • for consistency we should aim at generating the assets from jsonnet.
  • is there any authn/authz for the console plugin deployment?

pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/manifests/manifests.go Outdated Show resolved Hide resolved
pkg/tasks/console_plugin.go Outdated Show resolved Hide resolved
assets/console-plugin/console-plugin.yaml Outdated Show resolved Hide resolved
assets/console-plugin/configmap.yaml Outdated Show resolved Hide resolved
assets/console-plugin/deployment.yaml Outdated Show resolved Hide resolved
assets/console-plugin/deployment.yaml Outdated Show resolved Hide resolved
@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch 2 times, most recently from 21ffc8a to a9b319f Compare February 15, 2023 04:12
manifests/image-references Outdated Show resolved Hide resolved
@sthaha sthaha changed the title WIP: CMO deploys monitoring console-plugin WIP: [MON-2967]: CMO deploys monitoring console-plugin Feb 16, 2023
@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch from 7ad14ef to f7940f0 Compare February 20, 2023 07:51
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

I think this looks good.

@simonpasquier raised a good point in a chat earlier:
We have several options that can apply to most CMO components. It might be worth discussing which we should add intitally or which could be added in a follow-up PR.

  1. nodeSelector
    Most components can be moved to specific nodes. This should probably apply to the console plugin deployment as well?
  2. Same for tolerations.
  3. TopologySpreadConstraints are worth considering for high availability purposes.
  4. Setting logging levels doesn't apply iiuc.

I mostly just want to bring this up so we can discuss. I'd prefer to merge this PR and add the above in follow-ups. If some of those make it into 4.13, great! If not I could live with a later addition.

jsonnet/versions.yaml Outdated Show resolved Hide resolved
pkg/manifests/manifests.go Outdated Show resolved Hide resolved
jsonnet/main.jsonnet Outdated Show resolved Hide resolved
jsonnet/components/console-plugin.libsonnet Outdated Show resolved Hide resolved
@simonpasquier
Copy link
Contributor

reiterating my question from earlier: is there any authn/authz for the console plugin service? If not, does it mean that any user who's able to run a pod in the cluster can get access to the monitoring APIs?

@simonpasquier
Copy link
Contributor

simonpasquier commented Feb 20, 2023

From #1890 (review)

I mostly just want to bring this up so we can discuss. I'd prefer to merge this PR and add the above in follow-ups. If some of those make it into 4.13, great! If not I could live with a later addition.

I'd consider tolerations & node selector to be a must because some users have a requirement to separate infrastructure workloads from user workloads.

@sthaha
Copy link
Contributor Author

sthaha commented Feb 20, 2023

@kyoto could you please help answer the concern that Simon raised?

reiterating my question from earlier: is there any authn/authz for the console plugin service? If not, does it mean that any user who's able to run a pod in the cluster can get access to the monitoring APIs?

@kyoto
Copy link
Member

kyoto commented Feb 21, 2023

reiterating my question from earlier: is there any authn/authz for the console plugin service? If not, does it mean that any user who's able to run a pod in the cluster can get access to the monitoring APIs?

@simonpasquier The Service we're adding is just for serving the plugin's resources (JavaScript, CSS) as described in https://github.com/openshift/enhancements/blob/master/enhancements/console/dynamic-plugins.md#delivering-plugins

Once the new plugin frontend code is installed and running, it accesses Thanos and Alertmanager in the same way as the old Console code.

Does that answer your question?

@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch 3 times, most recently from ba80b46 to 1be089f Compare February 21, 2023 07:24
@sthaha
Copy link
Contributor Author

sthaha commented Feb 21, 2023

I think this looks good.

@simonpasquier raised a good point in a chat earlier: We have several options that can apply to most CMO components. It might be worth discussing which we should add intitally or which could be added in a follow-up PR.

  1. nodeSelector
    Most components can be moved to specific nodes. This should probably apply to the console plugin deployment as well?
  2. Same for tolerations.
  3. TopologySpreadConstraints are worth considering for high availability purposes.
  4. Setting logging levels doesn't apply iiuc.

I mostly just want to bring this up so we can discuss. I'd prefer to merge this PR and add the above in follow-ups. If some of those make it into 4.13, great! If not I could live with a later addition.

I didn't see a log-level that we can set since the plugin iiuc, is only an nginx that serves static content (js+css)
I have added all configs mentioned above + resource limits, could you please take a look again?

@jan--f
Copy link
Contributor

jan--f commented Feb 22, 2023

/retitle [WIP] MON-2967: CMO deploys monitoring console-plugin

@openshift-ci openshift-ci bot changed the title WIP: [MON-2967]: CMO deploys monitoring console-plugin [WIP] MON-2967: CMO deploys monitoring console-plugin Feb 22, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 22, 2023

@sthaha: This pull request references MON-2967 which is a valid jira issue.

In response to this:

DRAFT submitted for early feedback

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label May 31, 2023
@jan--f
Copy link
Contributor

jan--f commented May 31, 2023

/retest-required

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2023
@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch from 240e67c to 0214352 Compare June 7, 2023 23:27
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2023
@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch 2 times, most recently from 61e83e9 to c9550fa Compare June 8, 2023 04:51
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 8, 2023

@sthaha: This pull request references MON-2967 which is a valid jira issue.

In response to this:

DRAFT submitted for early feedback

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 8, 2023

@sthaha: This pull request references MON-2967 which is a valid jira issue.

In response to this:

DRAFT submitted for early feedback

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch from c9550fa to ee9defa Compare June 8, 2023 07:30
@simonpasquier
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023
@sthaha sthaha force-pushed the feat-deploy-monitoring-plugin branch from ee9defa to b9174c5 Compare June 8, 2023 07:47
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 8, 2023

@sthaha: This pull request references MON-2967 which is a valid jira issue.

In response to this:

This PR deploy the new monitoring console plugin

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 8, 2023

@sthaha: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-single-node 2e17e7d link false /test e2e-aws-single-node
ci/prow/e2e-agnostic 2e17e7d link true /test e2e-agnostic
ci/prow/e2e-agnostic-upgrade 2e17e7d link true /test e2e-agnostic-upgrade
ci/prow/versions b9174c5 link false /test versions

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jan--f
Copy link
Contributor

jan--f commented Jun 8, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, simonpasquier, sthaha

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 [jan--f,simonpasquier,sthaha]

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

@simonpasquier
Copy link
Contributor

/skip

@deepsm007
Copy link

/label jira/valid-bug

@openshift-ci openshift-ci bot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jun 12, 2023
@openshift-merge-robot openshift-merge-robot merged commit 1530a71 into openshift:master Jun 12, 2023
16 checks passed
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. docs-approved Signifies that Docs has signed off on this PR jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants