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

support optional env which dictates status check url format #146

Closed
wants to merge 1 commit into from

Conversation

peter-mcconnell
Copy link

@peter-mcconnell peter-mcconnell commented Aug 18, 2022

What does this PR do?

Allows the user to (optionally) specify a custom cluster domain for their kubernetes cluster.

why?

Upon deploying this to one of my clusters I noticed the operator was complaining about not being able to get a status from the service:

err

On inspection I realised the cluster domain in my kubernetes cluster and the one hardcoded here https://github.com/grafana/k6-operator/blob/main/controllers/k6_start.go#L20 were different.

other items of note

I've updated the yaml manifest and Makefile to support passing this variable through during make deploy. The approach I took here was to use envsubstr - this is a new dependency for this repository but at a glance it appears to be in use on other projects in this org so I hope that's ok. source: https://github.com/search?q=org%3Agrafana+envsubst&type=code

testing
I have tested this with and without K6_CLUSTER_DOMAIN env set and confirmed:

  • with no K6_CLUSTER_DOMAIN value set the default ...svc.cluster.local address is used
  • with K6_CLUSTER_DOMAIN value set the url ...svc$K6_CLUSTER_DOMAIN address is used. Further - I've tested that this patch results in a successful deployment on our clusters.

@CLAassistant
Copy link

CLAassistant commented Aug 18, 2022

CLA assistant check
All committers have signed the CLA.

@yorugac
Copy link
Collaborator

yorugac commented Dec 14, 2022

Hi @peter-mcconnell, I'm terribly sorry for missing your PR! It totally slipped through my notifications 😞

Your use case is 100% valid and it'd be good to have a fix for it; but I wonder if it could be resolved in some other way? First that comes to mind is to use IP instead of DNS: I'd say I prefer DNS for reasons described in #89 and there's also a case of IPv6 in #171. OTOH, there was a bug with using DNS in #132, fixed in #140.

It'd be good to make a full review of the points mentioned in these issues and come up with some unified approach that will cover all cases, including the one raised in this PR.
@peter-mcconnell, if you'd like to grok this despite the delays, please feel free to 🙂 If not, that's fine, I'll review and work on this in the next year.

@ghost ghost mentioned this pull request Feb 1, 2023
@mld
Copy link

mld commented May 11, 2023

We just started checking out k6-operator and got stuck on this issue too (custom cluster domain).
We forked and went with just removing the .cluster.local part in the check for now, but if one likes FQDN (which might be argued is a good thing), this feels like the way to handle it.

@illrill
Copy link
Contributor

illrill commented Nov 26, 2023

I think this was solved by #288, right?

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

Successfully merging this pull request may close these issues.

None yet

5 participants