Add ability for custom DNSConfig and DNSPolicy by stafot · Pull Request #3899 · prometheus-operator/prometheus-operator

stafot · 2021-03-05T15:03:56Z

Closes #3836.

Adds ability to customize DNS configuration on `Alertmanager`, `Prometheus` and `Thanos` pods.
Adds also ability for custom DNS policy.

pkg/apis/monitoring/v1/thanos_types.go

pkg/apis/monitoring/v1/types.go

simonpasquier

do we want to expose dnsPolicy too? cc @prometheus-operator/prometheus-operator-reviewers

pkg/apis/monitoring/v1/types.go

stafot · 2021-03-08T13:09:27Z

do we want to expose dnsPolicy too? cc @prometheus-operator/prometheus-operator-reviewers

If yes I could add this too, @simonpasquier.

simonpasquier · 2021-03-08T14:55:42Z

I would assume folks would want dnsPolicy eventually but I'm not an expert here :)

simonpasquier

Thanks! Only a few nits :)

pkg/apis/monitoring/v1/types.go

pkg/apis/monitoring/v1/thanos_types.go

coderanger · 2021-03-28T21:24:26Z

Isn't this completely equivalent to the existing deep merge option? This doesn't seem common enough to need dedicated fields.

simonpasquier · 2021-03-31T13:58:56Z

@coderanger DNS config applies at the pod level, not container level so deep-merge can't work IIUC.

nitinpatil1992 · 2021-05-07T17:01:45Z

Any plan for merging this?

stafot · 2021-05-08T09:34:08Z

@nitinpatil1992 Is there a formal process to ask for more reviews to proceed?

simonpasquier · 2021-05-21T10:30:43Z

We're having discussions about adding deep-merge capabilities for the pod spec which would allow to cover any customization (see #4023). It might be more efficient and flexible in the long run than slowly extending the Prometheus CRD every time a new use case comes around.

simonpasquier · 2022-05-11T12:38:31Z

Hey! Sorry for the very late feedback but we've discussed #4691 at the last contributors office hours and we agreed that adding support for dnsConfig and dnsPolicy makes sense. Would you be able to rebase your PR?

stafot · 2022-05-11T14:01:02Z

I am unable to make generate at the moment due to golang/go#51706. I will retry asap.
cc @simonpasquier

simonpasquier

Nice!

pkg/apis/monitoring/v1/prometheus_types.go

pkg/apis/monitoring/v1/dns_types.go

simonpasquier · 2024-09-27T12:58:00Z

pkg/apis/monitoring/v1/dns_types_test.go

I don't think that we need unit tests at this level. On the other side it would be good to check that the API validation works as expected in the e2e tests (example:

prometheus-operator/test/e2e/prometheus_test.go

Line 4376 in 7405049

func testPrometheusCRDValidation(t *testing.T) {

).

@simonpasquier Followed the pattern, I saw with service_monitor_types. Thus I added test for dns_types.

Thanks for the review but I believe needs more work on my side to be reviewable as some tests failing.

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

Review suggestions Co-authored-by: Simon Pasquier <spasquie@redhat.com>

More review suggestions Co-authored-by: Simon Pasquier <spasquie@redhat.com>

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

stafot · 2024-10-05T18:36:25Z

@paulfantom and @simonpasquier I believe now this PR is ready for review.
Changes based on previous comments done, all unit test and lint failures handled.

stafot · 2024-10-06T17:18:26Z

@simonpasquier I am working on adding these cases on e2e too, as per your comment. Let me know though if it is a must have or nice to have for this PR.

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

pkg/apis/monitoring/v1/alertmanager_types.go

pkg/apis/monitoring/v1/thanos_types.go

simonpasquier · 2024-10-07T14:36:55Z

pkg/prometheus/agent/statefulset.go

 				TopologySpreadConstraints:     prompkg.MakeK8sTopologySpreadConstraint(finalSelectorLabels, cpf.TopologySpreadConstraints),
 				HostAliases:                   operator.MakeHostAliases(cpf.HostAliases),
 				HostNetwork:                   cpf.HostNetwork,
+				DNSPolicy:                     k8sutil.ConvertDNSPolicy(cpf.DNSPolicy),


We should rather only set the fields when they're not nil to avoid undesired side-effects (the same for all controllers).

if cpf.DNSPolicy != nil { spec.DNSPolicy = appsv1.DNSPolicy(*cpf.DNSPolicy) } if cpf.DNSConfig != nil { spec.DNSConfig = k8sutil.ConvertToK8sDNSConfig(*cpf.DNSConfig) }

@simonpasquier I don't fully follow where I need to change this. Could you elaborate please?

pkg/apis/monitoring/v1/dns_types_test.go

Co-authored-by: Simon Pasquier <spasquie@redhat.com>

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

stafot · 2024-10-08T12:49:04Z

@simonpasquier I think last refactor takes into consideration all your comments

simonpasquier

thanks!

paulfantom reviewed Mar 5, 2021

View reviewed changes

pkg/apis/monitoring/v1/thanos_types.go Outdated Show resolved Hide resolved

pkg/apis/monitoring/v1/types.go Outdated Show resolved Hide resolved

pkg/apis/monitoring/v1/types.go Outdated Show resolved Hide resolved

stafot force-pushed the add_dns_capabilities_to_pods branch 5 times, most recently from b380215 to b18a8e1 Compare March 6, 2021 16:55

stafot marked this pull request as ready for review March 6, 2021 16:55

stafot requested a review from a team as a code owner March 6, 2021 16:55

simonpasquier reviewed Mar 8, 2021

View reviewed changes

pkg/apis/monitoring/v1/types.go Outdated Show resolved Hide resolved

stafot force-pushed the add_dns_capabilities_to_pods branch 2 times, most recently from d09b9d7 to b8f0288 Compare March 12, 2021 17:23

simonpasquier reviewed Mar 15, 2021

View reviewed changes

stafot force-pushed the add_dns_capabilities_to_pods branch 3 times, most recently from f62a5aa to 197dc30 Compare March 28, 2021 15:06

stafot requested a review from paulfantom March 28, 2021 17:15

stafot requested a review from simonpasquier March 31, 2021 14:09

simonpasquier mentioned this pull request May 21, 2021

Allow to define custom scheduler in the pod definition #4023

Open

simonpasquier mentioned this pull request Mar 28, 2022

How can the CRDs offer more flexibility/control for the pod template spec? #4691

Open

stafot force-pushed the add_dns_capabilities_to_pods branch from 197dc30 to 35f5e52 Compare May 11, 2022 13:59

stafot force-pushed the add_dns_capabilities_to_pods branch from 08039b6 to b30e606 Compare May 12, 2022 20:39

stafot force-pushed the add_dns_capabilities_to_pods branch from 673603a to 3bf1aed Compare September 27, 2024 12:33

pull-request-size bot added the size/XL label Sep 27, 2024

simonpasquier reviewed Sep 27, 2024

View reviewed changes

stafot marked this pull request as draft September 27, 2024 14:19

github-actions bot removed the stale label Sep 28, 2024

stafot and others added 4 commits October 5, 2024 18:45

Sync with latest code. Take into account review comments

4f97e77

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

Apply suggestions from code review

d056fab

Review suggestions Co-authored-by: Simon Pasquier <spasquie@redhat.com>

Apply suggestions from code review

4425abc

More review suggestions Co-authored-by: Simon Pasquier <spasquie@redhat.com>

Fix DNS policy conversion

195f540

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

stafot force-pushed the add_dns_capabilities_to_pods branch from dc9fe8f to 34b6591 Compare October 5, 2024 15:46

stafot marked this pull request as ready for review October 5, 2024 18:32

Format, lint and unit test fixes

1c03c32

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

stafot force-pushed the add_dns_capabilities_to_pods branch from 0b8eef0 to 1c03c32 Compare October 5, 2024 18:34

stafot requested a review from simonpasquier October 5, 2024 18:36

WIP: Add e2e tests for DNSPolicy and DNSConfig

558977b

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

stafot force-pushed the add_dns_capabilities_to_pods branch from 8a563da to 558977b Compare October 7, 2024 08:12

simonpasquier reviewed Oct 7, 2024

View reviewed changes

Apply suggestions from code review

ce11c35

Co-authored-by: Simon Pasquier <spasquie@redhat.com>

stafot marked this pull request as draft October 7, 2024 18:55

stafot added 2 commits October 8, 2024 12:18

Review changes

ca7787a

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

Make tidy

80a0414

Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>

stafot marked this pull request as ready for review October 8, 2024 12:48

stafot requested a review from simonpasquier October 8, 2024 12:48

simonpasquier approved these changes Oct 8, 2024

View reviewed changes

simonpasquier merged commit d723855 into prometheus-operator:main Oct 8, 2024

simonpasquier mentioned this pull request Oct 8, 2024

chore: implement DNS options for DaemonSet #6998

Merged

5 tasks

stafot deleted the add_dns_capabilities_to_pods branch October 12, 2024 16:45

Conversation

stafot commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stafot commented Mar 8, 2021

Uh oh!

simonpasquier commented Mar 8, 2021

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderanger commented Mar 28, 2021

Uh oh!

simonpasquier commented Mar 31, 2021

Uh oh!

nitinpatil1992 commented May 7, 2021

Uh oh!

stafot commented May 8, 2021

Uh oh!

simonpasquier commented May 21, 2021

Uh oh!

simonpasquier commented May 11, 2022

Uh oh!

stafot commented May 11, 2022

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonpasquier Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

stafot Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

stafot commented Oct 6, 2024

Uh oh!

Uh oh!

Uh oh!

simonpasquier Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

stafot Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stafot commented Oct 8, 2024

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

stafot commented Mar 5, 2021 •

edited

Loading

stafot Sep 27, 2024 •

edited

Loading

stafot commented Oct 5, 2024 •

edited

Loading