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.

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

feat: add support for Docker SD in ScrapeConfig #6421

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

mviswanathsai
Copy link
Contributor

Description

This PR aims to add Docker service discovery config support to the ScrapeConfig Alpha CRD.

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

I have written tests under pkg/prometheus/promfg_test.go for verifying the working of DockerSD config.

Changelog entry

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

Add support for Docker SD in ScrapeConfig

@slashpai
Copy link
Contributor

Thanks for addressing comments from #6386

Overall it looks good to me but let @simonpasquier also take a look before merging :)

@slashpai
Copy link
Contributor

slashpai commented Apr 1, 2024

@simonpasquier can you review too?

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Would it make sense to merge all 4 test cases into a single one?

I'm thinking that we could reduce the number of lines in this test since those configs don't conflict with each other

@ArthurSens
Copy link
Member

Ah, looks like we have many PRs open related to service discovery in scrape configs 😬

Could you also rebase the PR?

Add DockerSDConfig struct and array of DockerSDConfig to the ScrapeConfig struct

Add code block placeholder to process DockerSDConfig

Add Code-gen for the updated scrapeconfig with DockerSDConfig

Revert "Add Code-gen for the updated scrapeconfig with DockerSDConfig"

This reverts commit f7d2ff9.

Edit DockerSDConfig struct

Add Code-gen for the updated DockerSDConfig

Add processing code block for DockerSDConfig in the ScrapeConfig CRD

Update processing filters in DockerSDConfig

Add tests for DockerSDConfig

Add missing host field to DockerSDConfig struct and remove TODOs in promcfg.go

Update promcfg.go to append host field to the DockerSDConfiguration

Update autogen code and perform formatting fixes

Update DockerSD tests to include Host field

Add resource_selector validation and tests for Docker SD configs

Update tests according to host variable, tests pass

Add DockerFilter type for the filters field in DockerSDConfigs

Add code-gen for DockerFilter type update

Update promcfg test and test data for DockerFilter type

Update DockerFilter

Format code

Update pkg/apis/monitoring/v1alpha1/scrapeconfig_types.go

Co-authored-by: Jayapriya Pai <slashpai9@gmail.com>

Add validation for host field

Add relevant comments and remove unrelated debug code

Code-gen and format code

Revert "Change git mod file"

This reverts commit 232816f.

Change from pointer to ProxyConfig to variable reference

Generate Code and Format

Format code

Refactor test cases for Docker SD

One test case each for OAuth, BasicAuth and Authorization fields. Also includes other fields like TLSConfig, hostnetworkinghost etc.

Format code
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

I'll let @slashpai handle the merge since she is coordinating the 0.73 release. I'm not sure if there's extra coordination going on between other PRs that touch service discoveries

@slashpai
Copy link
Contributor

slashpai commented Apr 2, 2024

Yes this one should be good to go was next on queue for SDs

@slashpai
Copy link
Contributor

slashpai commented Apr 2, 2024

@mviswanathsai I remember seeing a test with Docker filter when reviewing earlier not sure if that was removed. I think a test for that would be good to see if it is configured correctly

@slashpai slashpai merged commit 9715735 into prometheus-operator:main Apr 2, 2024
17 checks passed
@slashpai
Copy link
Contributor

slashpai commented Apr 2, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants