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

add public-viewer support to MemberClusterService #450

Merged
merged 11 commits into from
Jul 31, 2024
Merged

add public-viewer support to MemberClusterService #450

merged 11 commits into from
Jul 31, 2024

Conversation

filariow
Copy link
Contributor

This PR is part of the effort of splitting PR #443

Signed-off-by: Francesco Ilario filario@redhat.com

@filariow
add public-viewer support to MemberClusterService
7a31ebd 
this commit is part of the effort of splitting PR codeready-toolchain#443

Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow filariow mentioned this pull request Jul 25, 2024
Merged
7 tasks
@codecov Codecov
Copy link

codecov bot commented Jul 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.56%. Comparing base (0eb4c1f) to head (0625dc8).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
+ Coverage   78.40%   78.56%   +0.15%     
==========================================
  Files          42       42              
  Lines        2677     2696      +19     
==========================================
+ Hits         2099     2118      +19     
  Misses        487      487              
  Partials       91       91
Flag Coverage Δ
unittests 78.56% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alexeykazakov
alexeykazakov reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

Overall looks good (I haven't look at the tests yet)!

pkg/proxy/service/cluster_service.go Outdated Show resolved Hide resolved
pkg/proxy/service/cluster_service.go Outdated Show resolved Hide resolved
pkg/proxy/service/cluster_service.go Outdated Show resolved Hide resolved
mfrancisc
mfrancisc reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Overall looks good! Great job 👍

pkg/proxy/service/cluster_service_test.go Show resolved Hide resolved
pkg/proxy/service/cluster_service_test.go Outdated Show resolved Hide resolved
pkg/proxy/service/cluster_service_test.go Show resolved Hide resolved
@mfrancisc
Copy link
Contributor

One comment I have is that the unit tests are becoming more complicated to follow now, I wonder if we really need to check all the cases with the additional layers , or if we could just add new test cases targeting the public viewer scenarios and configure the existing tests with publicViewer set to false, but maybe that will leave with some untested scenarios.

filariow added 5 commits July 26, 2024 10:37
@filariow
rename variable for complaintUserName
e66c362 
Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow
just check username
b711d62 
Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow
improve public-viewer unit tests
a260aa3 
Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow
improve public-viewer unit tests
7379e53 
Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow
fix linter
685672b 
Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow
Copy link
Contributor Author

One comment I have is that the unit tests are becoming more complicated to follow now, I wonder if we really need to check all the cases with the additional layers , or if we could just add new test cases targeting the public viewer scenarios and configure the existing tests with publicViewer set to false, but maybe that will leave with some untested scenarios.

When public-viewer is enabled we expect the same behavior we have when public-viewer is disabled with a few exception for the KubesawAuthenticated user. So IMO to test it correctly we need to test the the same tests are passing with either public-viewer enabled and disabled. Unfortunately this complicates tests.

alexeykazakov
alexeykazakov reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@alexeykazakov alexeykazakov 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.

Great job! And good test coverage. However, if I not mistaken, there is one test case missing: the public viewer access is disabled but the public viewer authenticated username is used.

@filariow
Copy link
Contributor Author

Great job! And good test coverage. However, if I not mistaken, there is one test case missing: the public viewer access is disabled but the public viewer authenticated username is used.

I looked into it and eventually decided to not add this test case because

  1. kube is in the list of forbidden prefixes so it should never happen as kubesaw-authenticated matches it
  2. even if it existed, it would be treated as a normal user (scenario already tested)

Btw, it should not be neither difficult nor harmful to have this test case. WDYT, should we add it anyway?

alexeykazakov
alexeykazakov reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

kube is in the list of forbidden prefixes so it should never happen as kubesaw-authenticated matches it
even if it existed, it would be treated as a normal user (scenario already tested)
Btw, it should not be neither difficult nor harmful to have this test case. WDYT, should we add it anyway?

OK. It makes sense. The only reason for such test would be to make sure that using kubesaw-authenticated would still require the public viewer feature to be enabled. See my comment below.

pkg/proxy/service/cluster_service.go Show resolved Hide resolved
filariow added 2 commits July 29, 2024 11:03
@filariow
add public-viewer disabled test
d9ba99c 
the public viewer access is disabled but the public viewer authenticated username is used

Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow
enhance tests
589d5f9 
Signed-off-by: Francesco Ilario <filario@redhat.com>
alexeykazakov
alexeykazakov approved these changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks. Just one minor comment.

pkg/proxy/service/cluster_service_test.go Outdated Show resolved Hide resolved
mfrancisc
mfrancisc approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Great Job 👍

Thanks for addressing my comments!

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jul 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, filariow, mfrancisc

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 [alexeykazakov,mfrancisc]

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

filariow and others added 2 commits July 30, 2024 10:39
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
5.7% Duplication on New Code

See analysis details on SonarCloud

@filariow
Copy link
Contributor Author

/test e2e

@filariow
Copy link
Contributor Author

/retest

@filariow
Copy link
Contributor Author

/retest

worker setup is failing when downloading Go SDK

 go1.20.11.linux-amd64.tar.gz: FAILED
sha256sum: WARNING: 1 computed checksum did NOT match

@filariow
Copy link
Contributor Author

/retest

@filariow filariow merged commit 007cba6 into codeready-toolchain:master Jul 31, 2024
10 of 11 checks passed
@filariow filariow deleted the pv-532-memberclusterservice branch July 31, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfrancisc mfrancisc mfrancisc approved these changes

@alexeykazakov alexeykazakov alexeykazakov approved these changes

@MatousJobanek MatousJobanek Awaiting requested review from MatousJobanek MatousJobanek is a code owner

@xcoulon xcoulon Awaiting requested review from xcoulon xcoulon is a code owner

@rajivnathan rajivnathan Awaiting requested review from rajivnathan rajivnathan is a code owner

@ranakan19 ranakan19 Awaiting requested review from ranakan19 ranakan19 is a code owner

@sbryzak sbryzak Awaiting requested review from sbryzak

Assignees
No one assigned
Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@filariow @mfrancisc @alexeykazakov