Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Fix DNS lookup failure error for host URLs #449

Closed
wants to merge 6 commits into from

Conversation

FedeBev
Copy link

@FedeBev FedeBev commented Jan 17, 2020

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml (Seems to be not required for current tests)

This is a fix PR for #394 bug, feel free to suggest any other solution/idea.

Thanks

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
34e98ee

Please, read and sign the above mentioned agreement if you want to contribute to this project

@FedeBev
Copy link
Author

FedeBev commented Jan 17, 2020

CLA has now been signed.
Sorry, what I have to do now?

@pbecotte
Copy link
Contributor

I wound up fixing this like this-

#439 (comment)

@FedeBev
Copy link
Author

FedeBev commented Jan 17, 2020

I wound up fixing this like this-

#439 (comment)

That could be a great solution!
Both solutions are valid and feasible in my opinion, maybe I cane make this options configurable through values file, what do you think?

@pbecotte
Copy link
Contributor

I had an earlier PR that set this variable, you could resurrect it and trim it down to just this. Eventually I came to the conclusion that host networking was the easiest solution, but almost never the right one. That's not up to me though ;)

@jmlrt jmlrt added the enhancement New feature or request label Jan 20, 2020
@jmlrt
Copy link
Member

jmlrt commented Jan 20, 2020

Hi @FedeBev,
This was already discussed a lot, host networking can be a great addition to this chart as long as it is optional and disabled by default.
You can also find an example in @pbecotte's Filebeat PR (this was disabled by default in 538af31).

@FedeBev
Copy link
Author

FedeBev commented Feb 17, 2020

Hi @jmlrt ,
hostNetwork and dnsPolicy options are now configurable through values.

In my opinion also @pbecotte solution is fine, but I can't see the new options in 7.6 release and in my opinion is needful.

Please, could you let me know the state on that?

Thanks

@jmlrt jmlrt self-requested a review February 18, 2020 06:27
Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Hi @FedeBev, many thanks for this PR.
Sorry for the long delay for reviewing this PR.

Filebeat PR for hostNetwork (#321) use only hostNetworking value to manage both hostNetwork and dnsPolicy.

From what I see in Pod’s DNS Policy doc, the only relevant dnsPolicy values are ClusterFirst (default value) when hostNetwork is disabled and ClusterFirstWithHostNet when hostNetwork is enabled.

Do you have some use case where you would need a different policy?

@jmlrt
Copy link
Member

jmlrt commented Apr 17, 2020

@FedeBev I created #585 to add host networking to Metricbeat while staying consistent with Filebeat.
This will be merged today or in a couple of days so I'll close your PR.

I'm sorry for making you wait so long then replacing your work due to bad timing.
I hope it won't demotivate you for contributing to this repos in the future.

@jmlrt jmlrt closed this Apr 17, 2020
@FedeBev
Copy link
Author

FedeBev commented Apr 17, 2020

Hi @jmlrt, thank's for you work.

I kept the options separate to leave maximum configuration flexibility. At the moment I can't image a situation where this is really required, so it's perfectly fine for me to keep that configuration in one option and #585 is perfect for my use case.

Don't worry for the timing, luckily the workaround was very easy 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants