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: Enable Gardener ingress annotations for dns (#250) #251

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

JensAc
Copy link
Contributor

@JensAc JensAc commented Aug 10, 2023

This PR enables the use of Gardener ingress for automatic creation of Dnsentries, when the operator is run on a Gardener-based Kubernetes cluster.

Copy link
Member

@pmig pmig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution :-)

We are happy to support gardener.

Please make sure to not break clusters without this specific config map. A quick test would be to delete the glasskube-settings and shoot-info config map and restart the operator.

As an additional tip you can forward the MinIO service via:

kubectl port-forward -n glasskube-system svc/glasskube-minio 9000

and configure it in your local IDE with an additional env var:

MINIO_HOST_NAME:localhost

So you can connect your operator to a remote cluster.

Comment on lines 61 to 62
kubernetesClient.configMaps().inNamespace("kube-system").withName("shoot-info").get().data["extensions"]!!.contains("shoot-dns-service") ->
CloudProvider.gardener
Copy link
Member

@pmig pmig Aug 10, 2023

Choose a reason for hiding this comment

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

The use of !! is generally not recommended it hides NullPointerExceptions. Especially in this case where an Exception is thrown if no config map with the name shoot-info can be found.

2023-08-10T17:32:09.837+02:00 [ReconcilerExecutor-configgenerator-182] ERROR i.j.o.p.e.ReconciliationDispatcher - Error during event processing ExecutionScope{ resource id: ResourceID{name='glasskube-settings', namespace='glasskube-system'}, version: 168764586} failed. 
java.lang.NullPointerException: Cannot invoke "io.fabric8.kubernetes.api.model.ConfigMap.getData()" because the return value of "io.fabric8.kubernetes.client.dsl.Resource.get()" is null
	at eu.glasskube.operator.config.ConfigService.getDynamicCloudProvider(ConfigService.kt:61)

This will break config map creation for non gardener managed clusters. I recommend a solution similar to .. ?.let { it.contains(...) }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I see. I misunderstood the behavior of .!!. So what about?

kubernetesClient.configMaps().inNamespace("kube-system")?.withName("shoot-info")?.get()?.data?.get("extensions")?.contains("shoot-dns-service") == true ->
                CloudProvider.gardener

I tried this at least, and it seams to be working fine, even the safe call chain does not look very pretty.
Sorry, I am new to Kotlin and tried to search the web for the reason why you've suggested a solution based on let but did not find any appropriate explanation. Maybe, you could point me somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Alright, I see. I misunderstood the behavior of .!!. So what about?

kubernetesClient.configMaps().inNamespace("kube-system")?.withName("shoot-info")?.get()?.data?.get("extensions")?.contains("shoot-dns-service") == true ->
                CloudProvider.gardener

I tried this at least, and it seams to be working fine, even the safe call chain does not look very pretty. Sorry, I am new to Kotlin and tried to search the web for the reason why you've suggested a solution based on let but did not find any appropriate explanation. Maybe, you could point me somewhere?

Yes that should work fine, although I think the first result that can actually be null is that of get(), so you can omit the first two safe calls. Using let (or another scope function) is not necessary here.

(Also, small correction: The !! operator does not hide NullPointerExceptions, it raises one if the value is null.)

@pmig pmig linked an issue Aug 10, 2023 that may be closed by this pull request
1 task
@pmig pmig self-requested a review August 29, 2023 07:40
Copy link
Member

@pmig pmig left a comment

Choose a reason for hiding this comment

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

We are happy to merge your contribution, with a small improvement - as kosmoz mentioned.

One last thing: Please squash your commits to a single commit following the conventional commit syntax.

@@ -58,12 +58,14 @@ class ConfigService(

private val dynamicCloudProvider
get() = when {
kubernetesClient.configMaps().inNamespace("kube-system")?.withName("shoot-info")?.get()?.data?.get("extensions")?.contains("shoot-dns-service") == true ->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
kubernetesClient.configMaps().inNamespace("kube-system")?.withName("shoot-info")?.get()?.data?.get("extensions")?.contains("shoot-dns-service") == true ->
kubernetesClient.configMaps().inNamespace("kube-system").withName("shoot-info").get()?.data?.get("extensions")?.contains("shoot-dns-service") == true ->

as @kosmoz mentioned in his comment:

I think the first result that can actually be null is that of get(), so you can omit the first two safe calls

@JensAc JensAc force-pushed the feat/gardener-ingress-annotations branch from b945b84 to 586413f Compare August 29, 2023 08:14
@JensAc
Copy link
Contributor Author

JensAc commented Aug 29, 2023

We are happy to merge your contribution, with a small improvement - as kosmoz mentioned.

One last thing: Please squash your commits to a single commit following the conventional commit syntax.

I just did that. I am not entirely sure whether this is a good workflow, as I needed a force push in order to rewrite my history to a single commit. Wouldn't it make more sense that you simply "squash and merge"? Anyways, I hope I met your requirements now.

@pmig pmig requested review from kosmoz and pmig August 29, 2023 08:46
@kosmoz kosmoz merged commit 868853f into glasskube:main Aug 29, 2023
2 checks passed
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.

[Feature]: Use Gardener ingress annotations for dns
3 participants