feat(metrics-operator): introduce insecureSkipTlsVerify parameter for prometheus metrics fetch #3742

Bharadwajshivam28 · 2024-10-05T00:33:28Z

Hey @odubajDT @mowies
I have implemented TLS round tripper to handle HTTP request.

codecov · 2024-10-07T06:25:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.57%. Comparing base (8eb878f) to head (6d639d1).
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3742      +/-   ##
==========================================
- Coverage   74.72%   74.57%   -0.15%     
==========================================
  Files         225      225              
  Lines       10160    12205    +2045     
==========================================
+ Hits         7592     9102    +1510     
- Misses       2198     2732     +534     
- Partials      370      371       +1

Files with missing lines	Coverage Δ
.../controllers/common/providers/prometheus/common.go	`90.00% <100.00%> (-0.48%)`	⬇️

... and 158 files with indirect coverage changes

Flag	Coverage Δ
certificate-operator	`47.44% <ø> (+0.24%)`	⬆️
component-tests	`56.61% <ø> (-1.43%)`	⬇️
lifecycle-operator	`76.07% <ø> (-0.17%)`	⬇️
metrics-operator	`75.95% <100.00%> (-0.36%)`	⬇️
scheduler	`36.09% <ø> (+1.49%)`	⬆️

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

Bharadwajshivam28 · 2024-10-09T11:43:39Z

Hey @odubajDT I have modified the GetRoundTripper function to use a custom http.Transport with TLSClientConfig.

metrics-operator/controllers/common/providers/prometheus/common.go

Signed-off-by: Bharadwajshivam28 <[email protected]>

odubajDT · 2024-10-14T11:11:09Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

@@ -119,7 +119,8 @@ func Test_GetRoundtripper(t *testing.T) {
 		name      string
 		provider  metricsapi.KeptnMetricsProvider
 		k8sClient client.Client
-		want      http.RoundTripper
+		wantUser  string


what is the reasoning behind this change? the function returns a roundtripper, therefore I would expect that instead of user and password

can you please elaborate on this? @odubajDT

my question is why you are expecting a user and password if the function you are testing is returning a roundtripper, it should in my opinion stay as it is, unless there is a reason for the change. This change seems to add significant complexity to the test, which is not needed

The thing is for the unit test without user and password it was failing so when I used user and password also it passed so I thought this way works when using user and password

yes the test is passing because now you are not checking the roundTripper correctly, please revert this and try to stick the the concept that is present unless there is a technical reason why it should be done differently

Hey @odubajDT
In this method Test_GetRoundtripper in common_test.go according to the current unit test it fails-
getRoundtripper() got = &{myuser mytoken 0x140005afb00}, want &{myuser mytoken 0x140001d7980} because the comparison of RoundTripper instances is not working with reflect.DeepEqual.

so instead of comparing entire RoundTripper structs I check specific properties like wantUser, wantPass, and wantTLS instead of a complete want RoundTripper.

@odubajDT one way is that by creating mock HTTP server to capture and verify the actual request sent by the RoundTripper. Instead of checking the request directly. It covers the main scenarios of secret retrieval and the creation of an authenticated RoundTripper

I modified the test case and it passes

so can we move with this unit test approach?

I guess you need to adapt the expected outcome of the function, since you just added a parameter to the config, which has an impact on the resulting object

can I create a new function in the common_test file?

Yes I understand that I need to adapt the changes in the test function but the thing without the mock method the test doesn't get adapted

what you should do is to set/unset the parameter in the metricProvider and expect it to be set/unset in the resulting roundtripper in the test. Not many changes are needed and I don't think another test function is needed here. You just need to duplicate and slightly adapt the test cases that are present

odubajDT · 2024-10-15T12:54:39Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

+				},
+			},
+			k8sClient: fake.NewClient(goodsecret),
+			want:      config.NewBasicAuthRoundTripper("myuser", "mytoken", "", "", promapi.DefaultRoundTripper),


why are you expecting defaultRoundTripper if you have modified it in the code?

odubajDT · 2024-10-23T06:30:22Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

+			// if !reflect.DeepEqual(got, tt.want) {
+			// 	t.Errorf("getRoundtripper() got = %v, want %v", got, tt.want)
+			// }
+			if tr, ok := got.(*http.Transport); ok {


the insecureSkipVerify is not the only parameter that should be tested here, please test also the user and the password of the round tripper

@odubajDT I have made the changes please verify it

still only a single parameter is verified here, what about user/password and other parameters set in the struct?

odubajDT · 2024-10-23T06:30:37Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

+			if !tt.wantErr && got == nil {
+				t.Errorf("getRoundtripper() returned nil, expected a RoundTripper")
+			}
+			// if !reflect.DeepEqual(got, tt.want) {


please remove this commented part if it's not needed

odubajDT · 2024-10-23T06:30:47Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

@@ -4,7 +4,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
-	"reflect"
+	// "reflect"


please remove

odubajDT

Please singoff your commits to pass the DCO check in the CI

Signed-off-by: Bharadwajshivam28 <[email protected]>

…ent method Signed-off-by: Bharadwajshivam28 <[email protected]>

Signed-off-by: Bharadwajshivam28 <[email protected]>

sonarcloud · 2024-10-23T13:37:40Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

odubajDT · 2024-10-23T13:46:23Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

+		wantUser         string
+		wantPassword     string


you are not asserting these parameters, can you explain why?

odubajDT · 2024-10-23T13:47:44Z

metrics-operator/controllers/common/providers/prometheus/common_test.go

+		k8sClient        client.Client
+		wantUser         string
+		wantPassword     string
+		wantRoundTripper http.RoundTripper


where is the roundtripper asserted?

mowies · 2024-11-11T07:35:41Z

@Bharadwajshivam28 any updates on this?

Bharadwajshivam28 requested a review from a team as a code owner October 5, 2024 00:33

github-actions bot added the metrics-operator label Oct 5, 2024

mowies changed the title ~~TLS method to Handle HTTP request~~ feat(metrics-operator): TLS method to Handle HTTP request Oct 7, 2024

mowies changed the title ~~feat(metrics-operator): TLS method to Handle HTTP request~~ feat(metrics-operator): use TLS to fetch metrics from prometheus Oct 7, 2024

Bharadwajshivam28 force-pushed the feat/client branch from a8909c5 to 2c0c512 Compare October 8, 2024 10:21

Bharadwajshivam28 force-pushed the feat/client branch from 911ac5e to 97eef27 Compare October 9, 2024 11:48

odubajDT reviewed Oct 9, 2024

View reviewed changes

metrics-operator/controllers/common/providers/prometheus/common.go Show resolved Hide resolved

Bharadwajshivam28 force-pushed the feat/client branch from fb7ae3b to 75b8a9f Compare October 10, 2024 04:14

Bharadwajshivam28 mentioned this pull request Oct 10, 2024

REQUEST: Membership Member for @Bharadwajshivam28 keptn/community#359

Closed

7 tasks

Bharadwajshivam28 force-pushed the feat/client branch from d11663c to 9f9211a Compare October 14, 2024 09:06

feat: Integrating ELK provider

e10b14d

Signed-off-by: Bharadwajshivam28 <[email protected]>

Bharadwajshivam28 force-pushed the feat/client branch from 94876c1 to 3a83d92 Compare October 14, 2024 10:54

odubajDT reviewed Oct 14, 2024

View reviewed changes

odubajDT reviewed Oct 15, 2024

View reviewed changes

Bharadwajshivam28 force-pushed the feat/client branch from 06a7d81 to 100e353 Compare October 16, 2024 11:06

odubajDT changed the title ~~feat(metrics-operator): use TLS to fetch metrics from prometheus~~ feat(metrics-operator): introduce insecureSkipTlsVerify parameters for prometheus metrics fetch Oct 22, 2024

odubajDT changed the title ~~feat(metrics-operator): introduce insecureSkipTlsVerify parameters for prometheus metrics fetch~~ feat(metrics-operator): introduce insecureSkipTlsVerify parameter for prometheus metrics fetch Oct 22, 2024

odubajDT reviewed Oct 23, 2024

View reviewed changes

odubajDT requested changes Oct 23, 2024

View reviewed changes

Bharadwajshivam28 added 7 commits October 23, 2024 19:06

feat: Implemented TLS round tripper

ba03ab3

Signed-off-by: Bharadwajshivam28 <[email protected]>

Removed unwanted folders

68f2271

Signed-off-by: Bharadwajshivam28 <[email protected]>

fix: removed unwanted lines form go.mod and go.sum files

a3d0d35

Signed-off-by: Bharadwajshivam28 <[email protected]>

modified GetRoundTripper function

4f019d3

Signed-off-by: Bharadwajshivam28 <[email protected]>

modified the common.go file

f6b8797

Signed-off-by: Bharadwajshivam28 <[email protected]>

feat: modified tls method to use exisiting behavior and allow tls skip

c42cf6f

Signed-off-by: Bharadwajshivam28 <[email protected]>

maintaining original DefaultRoundTripper behavior

9840512

Signed-off-by: Bharadwajshivam28 <[email protected]>

Bharadwajshivam28 added 14 commits October 23, 2024 19:06

modification in the logic and alignment of test according to the curr…

bf7a379

…ent method Signed-off-by: Bharadwajshivam28 <[email protected]>

fix: fixed failure of test_case

6150139

Signed-off-by: Bharadwajshivam28 <[email protected]>

modifing test file

f9df3af

Signed-off-by: Bharadwajshivam28 <[email protected]>

changes in test file

c3e3962

Signed-off-by: Bharadwajshivam28 <[email protected]>

feat: modifing unit test

c4ecd7e

Signed-off-by: Bharadwajshivam28 <[email protected]>

feat: code

4672e23

Signed-off-by: Bharadwajshivam28 <[email protected]>

fix: removed clone() from the logic

1dd3f1d

Signed-off-by: Bharadwajshivam28 <[email protected]>

feat: modifying unit test

aef152e

Signed-off-by: Bharadwajshivam28 <[email protected]>

feat: Added InsecureSkipTlsVerify flag

f46b6a2

Signed-off-by: Bharadwajshivam28 <[email protected]>

adding tests in the common_test.go

0024f6f

Signed-off-by: Bharadwajshivam28 <[email protected]>

formatted files

eea2d14

Signed-off-by: Bharadwajshivam28 <[email protected]>

Improvde test case

6cc8224

Signed-off-by: Bharadwajshivam28 <[email protected]>

Added username and password parameter

0013e90

Signed-off-by: Bharadwajshivam28 <[email protected]>

formatted file

6d639d1

Signed-off-by: Bharadwajshivam28 <[email protected]>

Bharadwajshivam28 force-pushed the feat/client branch from f4b9698 to 6d639d1 Compare October 23, 2024 13:37

odubajDT reviewed Oct 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics-operator): introduce insecureSkipTlsVerify parameter for prometheus metrics fetch #3742

feat(metrics-operator): introduce insecureSkipTlsVerify parameter for prometheus metrics fetch #3742

Bharadwajshivam28 commented Oct 5, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

Bharadwajshivam28 commented Oct 9, 2024

odubajDT Oct 14, 2024

Bharadwajshivam28 Oct 14, 2024

odubajDT Oct 14, 2024 •

edited

Loading

Bharadwajshivam28 Oct 14, 2024

odubajDT Oct 14, 2024

Bharadwajshivam28 Oct 14, 2024 •

edited

Loading

Bharadwajshivam28 Oct 14, 2024 •

edited

Loading

odubajDT Oct 15, 2024

Bharadwajshivam28 Oct 15, 2024 •

edited

Loading

odubajDT Oct 15, 2024

odubajDT Oct 15, 2024

odubajDT Oct 23, 2024

Bharadwajshivam28 Oct 23, 2024

odubajDT Nov 5, 2024

odubajDT Oct 23, 2024

odubajDT Oct 23, 2024

odubajDT left a comment

sonarcloud bot commented Oct 23, 2024

odubajDT Oct 23, 2024

odubajDT Oct 23, 2024

mowies commented Nov 11, 2024

feat(metrics-operator): introduce insecureSkipTlsVerify parameter for prometheus metrics fetch #3742

Are you sure you want to change the base?

feat(metrics-operator): introduce insecureSkipTlsVerify parameter for prometheus metrics fetch #3742

Conversation

Bharadwajshivam28 commented Oct 5, 2024 • edited Loading

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

Bharadwajshivam28 commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odubajDT Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharadwajshivam28 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Bharadwajshivam28 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharadwajshivam28 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odubajDT left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 23, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mowies commented Nov 11, 2024

Bharadwajshivam28 commented Oct 5, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

odubajDT Oct 14, 2024 •

edited

Loading

Bharadwajshivam28 Oct 14, 2024 •

edited

Loading

Bharadwajshivam28 Oct 14, 2024 •

edited

Loading

Bharadwajshivam28 Oct 15, 2024 •

edited

Loading