Refactor HTTP Service Discovery by mviswanathsai · Pull Request #6800 · prometheus-operator/prometheus-operator

mviswanathsai · 2024-08-05T12:10:14Z

Description

This PR refactors the HTTP Service Discovery, adds new test cases for unit tests.
PR also adds OAuth2, FollowRedirects and EnableHTTP2 fields to the HTTP service discovery.

Generic API validation e2e tests are yet to be added.

Closes #6798

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
[] NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

mviswanathsai · 2024-08-05T12:12:11Z

Not sure why OAuth2, FollowRedirects and EnableHTTP2 were not added originally. I did not find any documentation stating that they are unsupported nor did I find any evidence for this in the code (hoping you could verify). Were these fields intentionally left out? was it a mistake?
CC: @slashpai @simonpasquier

pkg/prometheus/resource_selector.go

slashpai · 2024-08-07T06:35:29Z

Not sure why OAuth2, FollowRedirects and EnableHTTP2 were not added originally. I did not find any documentation stating that they are unsupported nor did I find any evidence for this in the code (hoping you could verify). Were these fields intentionally left out? was it a mistake? CC: @slashpai @simonpasquier

httpsd was one of the first SD added to scrapeconfig IIRC it must have added required fields at that moment, thanks for adding rest

mviswanathsai · 2024-08-09T17:34:13Z

I will re-base once the EC2 PR is merged and add CRD validation tests for HTTPSDConfigs.

mviswanathsai · 2024-08-16T07:40:58Z

I will re-base as soon as we are happy with the changes and the PR for subtests is merged!

simonpasquier · 2024-08-26T13:01:38Z

Another merge conflict appeared :)

mviswanathsai · 2024-08-27T17:16:07Z

Conflicts resolved. PTAL @simonpasquier

simonpasquier

LGTM, a few nits only.

pkg/apis/monitoring/v1alpha1/scrapeconfig_types.go

pkg/prometheus/resource_selector.go

simonpasquier · 2024-08-28T09:07:22Z

test/e2e/scrapeconfig_test.go

+		scrapeConfigSpec: monitoringv1alpha1.ScrapeConfigSpec{
+			HTTPSDConfigs: []monitoringv1alpha1.HTTPSDConfig{
+				{
+					URL: "http://valid-url",


(nit) a good habit is to use reserved top-level domains for tests and documentation (the existing code base doesn't apply this recommendation though!).

Suggested change

URL: "http://valid-url",

URL: "http://valid.test",

Hmm... maybe we can just log an issue for that, it would make a good first-issue IMO.

mviswanathsai · 2024-08-30T05:59:32Z

Can't quite understand the unit-tests failing.

=== RUN   TestSelectScrapeConfigs/HTTP_SD_config_with_valid_proxy_settings
time=2024-08-30T11:26:59.002+05:30 level=WARN msg="skipping scrapeconfig" error="puppetDBSDConfigs: puppetDB SD configuration is only sup
ported for Prometheus version >= 2.31.0" scrapeconfig=test/test namespace=test prometheus=test
    resource_selector_test.go:3848: 
                Error Trace:    /home/mviswanathsai/operator/pkg/prometheus/resource_selector_test.go:3848
                Error:          "map[]" should have 1 item(s), but has 0
                Test:           TestSelectScrapeConfigs/HTTP_SD_config_with_valid_proxy_settings
=== RUN   TestSelectScrapeConfigs/HTTP_SD_config_with_valid_secret_ref
time=2024-08-30T11:26:59.003+05:30 level=WARN msg="skipping scrapeconfig" error="puppetDBSDConfigs: puppetDB SD configuration is only sup
ported for Prometheus version >= 2.31.0" scrapeconfig=test/test namespace=test prometheus=test
    resource_selector_test.go:3848: 
                Error Trace:    /home/mviswanathsai/operator/pkg/prometheus/resource_selector_test.go:3848
                Error:          "map[]" should have 1 item(s), but has 0
                Test:           TestSelectScrapeConfigs/HTTP_SD_config_with_valid_secret_ref

mviswanathsai · 2024-08-30T06:02:31Z

The test case seems right to me

		{
			scenario: "HTTP SD config with valid proxy settings",
			updateSpec: func(sc *monitoringv1alpha1.ScrapeConfigSpec) {
				sc.HTTPSDConfigs = []monitoringv1alpha1.HTTPSDConfig{
					{
						URL: "http://example.com",
						ProxyConfig: monitoringv1.ProxyConfig{
							ProxyURL:             ptr.To("http://no-proxy.com"),
							NoProxy:              ptr.To("0.0.0.0"),
							ProxyFromEnvironment: ptr.To(false),
							ProxyConnectHeader: map[string][]v1.SecretKeySelector{
								"header": {
									{
										LocalObjectReference: v1.LocalObjectReference{
											Name: "secret",
										},
										Key: "key1",
									},
								},
							},
						},
					},
				}
			},
			selected: true,
            promVersion: "2.29.0",
		},

Can't see why we get logs from validatePuppetDBSDConfigs in the error.

mviswanathsai · 2024-08-30T06:25:35Z

At the resource_selector_test.go, we could also do something similar to what we did with the e2e tests for API validation and break up the large test in smaller subtests.

simonpasquier

Thanks!

mviswanathsai requested a review from a team as a code owner August 5, 2024 12:10

pull-request-size bot added the size/L label Aug 5, 2024

simonpasquier reviewed Aug 5, 2024

View reviewed changes

pkg/prometheus/resource_selector.go Show resolved Hide resolved

pull-request-size bot added size/XL and removed size/L labels Aug 16, 2024

mviswanathsai force-pushed the sc-refactor--http-sd branch from 57b6339 to 4553b7c Compare August 22, 2024 14:45

pull-request-size bot added size/L and removed size/XL labels Aug 22, 2024

mviswanathsai force-pushed the sc-refactor--http-sd branch from 4553b7c to 6e2b2d1 Compare August 22, 2024 14:51

pull-request-size bot added size/XL and removed size/L labels Aug 22, 2024

mviswanathsai added this to the [GSoC 2024] Extending the ScrapeConfig CRD milestone Aug 26, 2024

mviswanathsai force-pushed the sc-refactor--http-sd branch from 6e2b2d1 to c30c3ad Compare August 27, 2024 05:16

simonpasquier reviewed Aug 28, 2024

View reviewed changes

mviswanathsai force-pushed the sc-refactor--http-sd branch 2 times, most recently from 6d6a90c to 5e9a3c9 Compare August 29, 2024 16:30

refactor http sd

2697e50

mviswanathsai force-pushed the sc-refactor--http-sd branch from 5e9a3c9 to 2697e50 Compare August 30, 2024 08:00

simonpasquier approved these changes Aug 30, 2024

View reviewed changes

simonpasquier merged commit bc63606 into prometheus-operator:main Aug 30, 2024

mviswanathsai mentioned this pull request Oct 7, 2024

Refactor DigitalOceanSDConfig #6991

Closed

Conversation

mviswanathsai commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Verification

Changelog entry

Uh oh!

mviswanathsai commented Aug 5, 2024

Uh oh!

Uh oh!

slashpai commented Aug 7, 2024

Uh oh!

mviswanathsai commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mviswanathsai commented Aug 16, 2024

Uh oh!

simonpasquier commented Aug 26, 2024

Uh oh!

mviswanathsai commented Aug 27, 2024

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simonpasquier Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

mviswanathsai Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

mviswanathsai commented Aug 30, 2024

Uh oh!

mviswanathsai commented Aug 30, 2024

Uh oh!

mviswanathsai commented Aug 30, 2024

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mviswanathsai commented Aug 5, 2024 •

edited

Loading

mviswanathsai commented Aug 9, 2024 •

edited

Loading