Skip to content

ROX-22288: Add test for operator metrics retrieval#9773

Merged
porridge merged 10 commits intomasterfrom
porridge/ROX-22288-test
Feb 8, 2024
Merged

ROX-22288: Add test for operator metrics retrieval#9773
porridge merged 10 commits intomasterfrom
porridge/ROX-22288-test

Conversation

@porridge
Copy link
Copy Markdown
Contributor

@porridge porridge commented Feb 6, 2024 •
edited
Loading

Description

Add an e2e test that makes sure that requests to the metrics endpoint are treated as expected:

  • unauthenticated requests are rejected with a 401
  • unauthorized requests (lack of get permission on /metrics) are rejected with a 403
  • authorized requests return metrics
  • requests using HTTP/2 are downgraded to HTTP/1 ("HTTP2/ Rapid Reset" attack mitigation)

This is to make sure nothing breaks when we replace rbac-proxy sidecar with builtin code.

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

No changes to production code.

Testing Performed

Here I tell how I validated my change

Ran the new test on a dev openshift and GKE cluster with --skip-delete, and inspected pods logs to make sure it's working as expected.

Reminder for reviewers

In addition to reviewing code here, reviewers must also review testing and request further testing in case the
performed one does not seem sufficient. As a reviewer, you must not approve the change until you understand the
performed testing and you are satisfied with it.

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Feb 6, 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

@porridge porridge force-pushed the porridge/ROX-22288-test branch from 7bfa06f to 795eb28 Compare February 6, 2024 08:51
@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 6, 2024

/test gke-operator-e2e-tests
/test ocp-4-11-operator-e2e-tests
/test ocp-4-14-operator-e2e-tests

@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Feb 6, 2024 •
edited
Loading

Images are ready for the commit at 4fd74d1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.3.x-1042-g4fd74d1810.

@porridge
more debugging
b70a39d
@porridge
set UID to run as
1a3e9db 
try to prevent the following error on GKE

Error: container has runAsNonRoot and image will run as root
@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 6, 2024

/test gke-operator-e2e-tests
/test ocp-4-11-operator-e2e-tests
/test ocp-4-14-operator-e2e-tests

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 6, 2024

/test gke-operator-e2e-tests
/test ocp-4-11-operator-e2e-tests
/test ocp-4-14-operator-e2e-tests

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2baa0c5) 47.63% compared to head (6583f44) 47.62%.
Report is 22 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #9773      +/-   ##
==========================================
- Coverage   47.63%   47.62%   -0.02%     
==========================================
  Files        2461     2462       +1     
  Lines      166964   167376     +412     
==========================================
+ Hits        79527    79706     +179     
- Misses      80881    81090     +209     
- Partials     6556     6580      +24
Flag Coverage Δ
go-unit-tests 47.62% <ø> (-0.02%) ⬇️

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.

@porridge porridge marked this pull request as ready for review February 6, 2024 12:57
@porridge porridge requested a review from a team February 6, 2024 12:57
@porridge porridge requested a review from johannes94 February 6, 2024 12:57
@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 7, 2024

/retest

@porridge porridge mentioned this pull request Feb 7, 2024
Merged
5 tasks
@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 7, 2024

/retest

vladbologa
vladbologa reviewed Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@vladbologa vladbologa left a comment

Choose a reason for hiding this comment

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

Could you also test curl --http2 and make sure the response does not actually use HTTP/2, to make sure it stays disabled once we drop the kube-rbac-proxy?

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 7, 2024

Could you also test curl --http2 and make sure the response does not actually use HTTP/2, to make sure it stays disabled once we drop the kube-rbac-proxy?

Good idea, let me do this.

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 7, 2024

Could you also test curl --http2 and make sure the response does not actually use HTTP/2, to make sure it stays disabled once we drop the kube-rbac-proxy?

Good idea, let me do this.

Hm, unfortunately (?) curl does a fallback to HTTP1 with --http2. Here's what I get as a result of curl -vvv --http2 --insecure --dump-header /tmp/headers "$url":

* Connected to rhacs-operator-controller-manager-metrics-service.stackrox-operator.svc.cluster.local (10.51.34.36) port 8443 (#0)       
* ALPN, offering h2                                                                                                                     
* ALPN, offering http/1.1                                                                                                               
*  CAfile: /etc/pki/tls/certs/ca-bundle.crt                                                                                             
* TLSv1.0 (OUT), TLS header, Certificate Status (22):                                                                                   
} [5 bytes data]                                                                                                                        
* TLSv1.3 (OUT), TLS handshake, Client hello (1):                                                                                       
[.. uninteresting TLS stuff]
* TLSv1.3 (OUT), TLS handshake, Finished (20):                                                                                          
} [36 bytes data]                                                                                                                       
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256                                                                                 
* ALPN, server accepted to use http/1.1                                                                                                 
* Server certificate:                                                                                                                   
*  subject: CN=rhacs-operator-controller-manager-65cd4b7b56-gdgxz@1707301859                                                            
*  start date: Feb  7 09:30:58 2024 GMT                                                                                                 
*  expire date: Feb  6 09:30:58 2025 GMT                                                                                                
*  issuer: CN=rhacs-operator-controller-manager-65cd4b7b56-gdgxz-ca@1707301859                                                          
*  SSL certificate verify result: self-signed certificate in certificate chain (19), continuing anyway.                                 
* TLSv1.2 (OUT), TLS header, Unknown (23):                                                                                              
} [5 bytes data]                                                                                                                        
> GET /metrics HTTP/1.1                                                                                                                 
> Host: rhacs-operator-controller-manager-metrics-service.stackrox-operator.svc.cluster.local:8443                                      
> User-Agent: curl/7.76.1                                                                                                               
> Accept: */*                                                                                                                           
>                                                                                                                                       
* TLSv1.2 (IN), TLS header, Unknown (23):                                                                                               
{ [5 bytes data]                                                                                                                        
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):                                                                                   
{ [122 bytes data]                                                                                                                      
* TLSv1.2 (IN), TLS header, Unknown (23):                                                                                               
{ [5 bytes data]                                                                                                                        
* Mark bundle as not supporting multiuse                                                                                                
< HTTP/1.1 401 Unauthorized                                                                                                             
< Content-Type: text/plain; charset=utf-8                                                                                               
< X-Content-Type-Options: nosniff                                                                                                       
< Date: Wed, 07 Feb 2024 11:19:19 GMT                                                                                                   
< Content-Length: 13                                                                                                                    
<                                                                                                                                       
{ [13 bytes data]

Any idea what to check? Is the existence of HTTP/1 in the headers good enough indication that HTTP/2 was not used @vladbologa ? 🤔

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Feb 7, 2024

I guess yes, since doing the same against google.com I get:

HTTP/2 301 
location: https://www.google.com/
content-type: text/html; charset=UTF-8
content-security-policy-report-only: object-src 'none';base-uri 'self';script-src 'nonce-ilYsOjV6AAzkDIlCC6A4GQ' 'strict-dynamic' 'report-sample' 'unsafe-eval' 'unsafe-inline' https: http:;report-uri https://csp.withgoogle.com/csp/gws/other-hp
date: Wed, 07 Feb 2024 11:24:36 GMT
expires: Wed, 07 Feb 2024 11:24:36 GMT
cache-control: private, max-age=2592000
server: gws
content-length: 220
x-xss-protection: 0

@vladbologa
Copy link
Copy Markdown
Contributor

vladbologa commented Feb 7, 2024

Any idea what to check? Is the existence of HTTP/1 in the headers good enough indication that HTTP/2 was not used @vladbologa ? 🤔

Yes, that was also my impression of how this can be done.

@porridge porridge requested a review from vladbologa February 7, 2024 11:39
vladbologa
vladbologa approved these changes Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@vladbologa vladbologa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Feb 7, 2024 •
edited
Loading

@porridge: The following test 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/ocp-4-11-scanner-v4-tests 4fd74d1 link false /test ocp-4-11-scanner-v4-tests

Full PR test history. Your PR dashboard.

Details

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.

johannes94
johannes94 approved these changes Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@johannes94 johannes94 left a comment

Choose a reason for hiding this comment

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

LGTM! I have nothing to add except that the failing ci/prow/ocp-4-11-scanner-v4-tests is a known issue.

johannes94
johannes94 reviewed Feb 7, 2024
View reviewed changes
@porridge porridge enabled auto-merge (squash) February 8, 2024 05:21
@porridge porridge merged commit a53ece9 into master Feb 8, 2024
@porridge porridge deleted the porridge/ROX-22288-test branch February 8, 2024 05:59
dcaravel pushed a commit that referenced this pull request May 15, 2024
@porridge @dcaravel
ROX-22288: Add test for operator metrics retrieval (#9773)
0011de5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johannes94 johannes94 johannes94 approved these changes

@vladbologa vladbologa vladbologa approved these changes

Assignees

No one assigned

Labels

area/operator

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@porridge @rhacs-bot @vladbologa @johannes94