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.

Sign up for GitHub

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

Jump to bottom

OCPNODE-2734: Replace gcr.io/kubebuilder/kube-rbac-proxy image #346

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

OCPNODE-2734: Replace gcr.io/kubebuilder/kube-rbac-proxy image #346

wants to merge 2 commits into from

Conversation

sairameshv
Copy link
Member

@sairameshv sairameshv commented Dec 18, 2024 •
edited
Loading

Fixes #291
Add the WithAuthenticationAndAuthorization filter to support deprecation of kube-rbac-proxy image from the kubebuilder repository

make lint, make test, make test-e2e-kind-emulated pass locally

There is no e2e test as of now to test this metrics endpoint, I would like to add the same and holding this PR until then
/hold

Added e2e test to validate the /metrics endpoint exposed by the instaslice-operator's service
/cc @rphillips @kannon92 @mamy-CS

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 18, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 18, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@sairameshv sairameshv changed the title Replace gcr.io/kubebuilder/kube-rbac-proxy image with WithAuthenticationAndAuthorization filter OCPNODE-2734: Replace gcr.io/kubebuilder/kube-rbac-proxy image Dec 18, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 18, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 18, 2024 •
edited by openshift-ci bot
Loading

@sairameshv: This pull request references OCPNODE-2734 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Fixes #291
Add the above filter to support deprecation of kube-rbac-proxy image from the kubebuilder repository

make lint, make test, make test-e2e-kind-emulated pass locally

There is no e2e test as of now to test this metrics endpoint, I would like to add the same and holding this PR until then
/hold

/cc @rphillips @kannon92 @mamy-CS

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 18, 2024 •
edited by openshift-ci bot
Loading

@sairameshv: This pull request references OCPNODE-2734 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Fixes #291
Add the WithAuthenticationAndAuthorization filter to support deprecation of kube-rbac-proxy image from the kubebuilder repository

make lint, make test, make test-e2e-kind-emulated pass locally

There is no e2e test as of now to test this metrics endpoint, I would like to add the same and holding this PR until then
/hold

/cc @rphillips @kannon92 @mamy-CS

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 18, 2024
@kannon92
Copy link
Contributor

I think you are missing a few services/role binding for this to work.

ex: kubernetes-sigs/jobset#722

I tested this by creating a metrics pod and using curl to verify that I could view the metrics if I use the right certs.

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sairameshv
Once this PR has been reviewed and has the lgtm label, please assign asm582 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@kannon92
Copy link
Contributor

We don't have any metrics yet so not sure what we would test. I guess you can try grabbing the metrics from controller-runtime?

@rphillips
Copy link
Contributor

Having the vendor update in a separate commit would be helpful for the review.

@rphillips
Copy link
Contributor

Still a lot of references to the proxy:

rg kube-rbac-proxy

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2024
sairameshv
sairameshv commented Dec 20, 2024
View reviewed changes
Copy link
Member Author

@sairameshv sairameshv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Resolved the review comments, Added the e2e test to validate the /metrics endpoint.
/hold cancel

bundle-ocp/manifests/instaslice-operator-controller-manager-metrics-service_v1_service.yaml Show resolved Hide resolved
cmd/daemonset/main.go Show resolved Hide resolved
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 20, 2024 •
edited by openshift-ci bot
Loading

@sairameshv: This pull request references OCPNODE-2734 which is a valid jira issue.

In response to this:

Fixes #291
Add the WithAuthenticationAndAuthorization filter to support deprecation of kube-rbac-proxy image from the kubebuilder repository

make lint, make test, make test-e2e-kind-emulated pass locally

There is no e2e test as of now to test this metrics endpoint, I would like to add the same and holding this PR until then
/hold

Added e2e test to validate the /metrics endpoint exposed by the instaslice-operator's service
/cc @rphillips @kannon92 @mamy-CS

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 openshift-eng/jira-lifecycle-plugin repository.

@sairameshv sairameshv marked this pull request as ready for review December 20, 2024 09:51
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2024
rphillips
rphillips reviewed Dec 20, 2024
View reviewed changes
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
rphillips
rphillips reviewed Dec 20, 2024
View reviewed changes
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 26, 2024
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 26, 2024
@sairameshv
Bump vendor packages
a6d4fc8 
Signed-off-by: Sai Ramesh Vanka <[email protected]>
@sairameshv
Replace deprecated gcr.io/kubebuilder/kube-rbac-proxy image
f7cbf35 
- Add the `WithAuthenticationAndAuthorization` filter to support deprecation of kube-rbac-proxy image
from the kubebuilder repository
- Add metrics-reader cluster role binding and the metrics service
- Add e2e test case to validate the `/metrics` endpoint

Signed-off-by: Sai Ramesh Vanka <[email protected]>
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 30, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 30, 2024

@sairameshv: all tests passed!

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@rphillips rphillips rphillips left review comments

@kannon92 kannon92 Awaiting requested review from kannon92

@mamy-CS mamy-CS Awaiting requested review from mamy-CS

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

⚠️ Action Required: Replace Deprecated gcr.io/kubebuilder/kube-rbac-proxy
6 participants
@sairameshv @openshift-ci-robot @kannon92 @rphillips @camilamacedo86 @openshift-merge-robot