Fix api service reg #1337

ishankhare07 · 2023-10-31T11:50:02Z

What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
/kind enhancement

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
resolves #1163 ENG-1901

Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster when deployed with k8s/eks distro and metrics proxy enabled could not resolve apiservice properly because syncer and apiserver being in separate pods

What else do we need to know?

Signed-off-by: Ishan Khare <[email protected]>

pkg/specialservices/proxy_service_syncer.go

charts/eks/templates/metrics-proxy.yaml

FabianKramm · 2023-10-31T14:32:15Z

charts/eks/templates/syncer-deployment.yaml

@@ -169,6 +174,7 @@ spec:
          {{- end }}
          {{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled}}
          - --proxy-metrics-server=true
+          - --single-binary-distro=false


Why do we have an env var and flag for this? Also why is this necessary?

this info is needed at 2 places basically, one is at places like endpoints syncer, apiregistration etc.
and the other is in specialservices

Since we don't have a way to inject controller context here, I thought to inject the env var

How about removing the global Default variable in the resolver (https://github.com/ishankhare07/vcluster/blob/c98aba12ee29bc661a75af190cabca0b8bd5d213/pkg/specialservices/resolver.go#L12) and adding a boolean to the function definition of DefaultNameserverFinder.

As the specialservices.Default variable is used in two places, why not store the variable from the controller context in those two syncers and then pass it down to the DefaultNameserverFinder function?

Also, would it make more sense to pass down the name of the distro as an arg instead of this flag and then decide within the code if it's a single binary distro?

charts/eks/templates/metrics-proxy.yaml

FabianKramm · 2023-10-31T19:52:09Z

@ishankhare07 also would be good if we add some sort of test here to avoid breaking this in future

address other minor review comments Signed-off-by: Ishan Khare <[email protected]>

Signed-off-by: Ishan Khare <[email protected]>

ThomasK33 · 2023-11-07T10:58:36Z

charts/eks/templates/syncer-deployment.yaml

@@ -169,6 +174,7 @@ spec:
          {{- end }}
          {{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled}}
          - --proxy-metrics-server=true
+          - --single-binary-distro=false


How about removing the global Default variable in the resolver (https://github.com/ishankhare07/vcluster/blob/c98aba12ee29bc661a75af190cabca0b8bd5d213/pkg/specialservices/resolver.go#L12) and adding a boolean to the function definition of DefaultNameserverFinder.

As the specialservices.Default variable is used in two places, why not store the variable from the controller context in those two syncers and then pass it down to the DefaultNameserverFinder function?

pkg/controllers/k8sdefaultendpoint/k8sdefaultendpoint.go

test/e2e_metrics_proxy/metricsProxy.go

ishankhare07 · 2023-11-08T10:22:07Z

#1337 (comment)

The DefaultNameserverFinder function is not called explicitly anywhere and is implicitly called on package import.
Also the .pro code depends on it and is supposed to override it in case integrated coredns is used over there.

We are still storing the boolean in the controllerctx and using it where possible. But if we want to pass it to DefaultNameserverFinder we need to be explicitly calling it in OSS somewhere and also when calling it in Pro, based on the flag we have to call it as default or reset it in case

Edit: I think I misunderstood, I got what you're suggesting and it makes sense - will push an update soon

Signed-off-by: Ishan Khare <[email protected]>

enable hostNetwork for metrics server Signed-off-by: Ishan Khare <[email protected]>

Signed-off-by: Ishan Khare <[email protected]>

pkg/controllers/resources/endpoints/syncer.go

Signed-off-by: Ishan Khare <[email protected]>

FabianKramm · 2023-11-15T09:31:54Z

pkg/specialservices/resolver.go


 const (
 	DefaultKubeDNSServiceName      = "kube-dns"
 	DefaultKubeDNSServiceNamespace = "kube-system"
+
+	DistroEnvKey = "SINGLE_BINARY_DISTRO"


Can we remove this?

FabianKramm · 2023-11-15T12:13:37Z

pkg/controllers/k8sdefaultendpoint/k8sdefaultendpoint.go

+		if vEndpoints.Labels == nil {
+			vEndpoints.Labels = map[string]string{}
+		}
+		// vEndpoints.Labels[discoveryv1.LabelSkipMirror] = "true"


why is this commented out?

netlify · 2023-11-15T12:14:46Z

✅ Deploy Preview for vcluster-docs canceled.

Name	Link
🔨 Latest commit	`f742c4e`
🔍 Latest deploy log	https://app.netlify.com/sites/vcluster-docs/deploys/6554b680eb9899000845f053

FabianKramm and others added 8 commits October 31, 2023 17:14

fix: allow custom k3s token

895e546

aux metrics proxy svc

729b8b9

Signed-off-by: Ishan Khare <[email protected]>

sync endpoints and detect single binary distros

acf2680

Signed-off-by: Ishan Khare <[email protected]>

fix values test

b26d41c

Signed-off-by: Ishan Khare <[email protected]>

fix lint errors

c416093

Signed-off-by: Ishan Khare <[email protected]>

only add SANs when not in single binary distro

6202d06

Signed-off-by: Ishan Khare <[email protected]>

remove unwanted configmap

a823b69

Signed-off-by: Ishan Khare <[email protected]>

modify eks chart for api registration fix

a26f223

Signed-off-by: Ishan Khare <[email protected]>

ishankhare07 marked this pull request as ready for review October 31, 2023 12:45

ishankhare07 requested a review from FabianKramm October 31, 2023 12:47

FabianKramm reviewed Oct 31, 2023

View reviewed changes

pkg/specialservices/proxy_service_syncer.go Outdated Show resolved Hide resolved

FabianKramm reviewed Oct 31, 2023

View reviewed changes

pkg/specialservices/proxy_service_syncer.go Outdated Show resolved Hide resolved

FabianKramm requested changes Oct 31, 2023

View reviewed changes

FabianKramm reviewed Oct 31, 2023

View reviewed changes

charts/eks/templates/metrics-proxy.yaml Outdated Show resolved Hide resolved

ishankhare07 added 3 commits November 2, 2023 20:17

move service creation from chart to code

0b9273d

address other minor review comments Signed-off-by: Ishan Khare <[email protected]>

add deletion of aux services in case of toggle

3fac312

Signed-off-by: Ishan Khare <[email protected]>

add e2e for metrics server api registration check

7b39ff4

Signed-off-by: Ishan Khare <[email protected]>

ishankhare07 force-pushed the fix-api-service-reg branch from e2a8de8 to 7b39ff4 Compare November 3, 2023 16:22

fix metrics server helm chart install

c98aba1

Signed-off-by: Ishan Khare <[email protected]>

ishankhare07 requested review from FabianKramm and ThomasK33 November 3, 2023 18:15

ThomasK33 reviewed Nov 7, 2023

View reviewed changes

modify special service default setup and remove env var

eecb935

Signed-off-by: Ishan Khare <[email protected]>

ishankhare07 force-pushed the fix-api-service-reg branch 2 times, most recently from 441f68f to 73719a4 Compare November 9, 2023 10:13

ishankhare07 added 2 commits November 9, 2023 17:52

add tests for metrics proxy - node and pod metrics

05b917e

enable hostNetwork for metrics server Signed-off-by: Ishan Khare <[email protected]>

fix metrics server chart installation

c6d0d85

Signed-off-by: Ishan Khare <[email protected]>

ishankhare07 force-pushed the fix-api-service-reg branch from 13b7442 to c6d0d85 Compare November 9, 2023 12:22

ishankhare07 requested a review from ThomasK33 November 9, 2023 13:30

ThomasK33 approved these changes Nov 14, 2023

View reviewed changes

FabianKramm reviewed Nov 14, 2023

View reviewed changes

pkg/controllers/resources/endpoints/syncer.go Show resolved Hide resolved

remove single bin flag, utilize existing distro detection mechanism

438d8ac

Signed-off-by: Ishan Khare <[email protected]>

FabianKramm reviewed Nov 15, 2023

View reviewed changes

refactor: use cache correctly

f742c4e

FabianKramm force-pushed the fix-api-service-reg branch from 720cbd5 to f742c4e Compare November 15, 2023 12:15

FabianKramm approved these changes Nov 15, 2023

View reviewed changes

FabianKramm merged commit e61c61b into loft-sh:main Nov 15, 2023
56 checks passed

joaocc mentioned this pull request Nov 28, 2023

vcluster v0.17.x system:serviceaccount:myvclusterns:vc-vcluster cannot list resource "secrets" in API group "" at the cluster scope #1384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix api service reg #1337

Fix api service reg #1337

ishankhare07 commented Oct 31, 2023

FabianKramm Oct 31, 2023

ishankhare07 Nov 2, 2023

ThomasK33 Nov 7, 2023

ThomasK33 Nov 7, 2023

FabianKramm commented Oct 31, 2023

ThomasK33 Nov 7, 2023

ishankhare07 commented Nov 8, 2023 •

edited

Loading

FabianKramm Nov 15, 2023

FabianKramm Nov 15, 2023

netlify bot commented Nov 15, 2023 •

edited

Loading

Fix api service reg #1337

Fix api service reg #1337

Conversation

ishankhare07 commented Oct 31, 2023

FabianKramm Oct 31, 2023

Choose a reason for hiding this comment

ishankhare07 Nov 2, 2023

Choose a reason for hiding this comment

ThomasK33 Nov 7, 2023

Choose a reason for hiding this comment

ThomasK33 Nov 7, 2023

Choose a reason for hiding this comment

FabianKramm commented Oct 31, 2023

ThomasK33 Nov 7, 2023

Choose a reason for hiding this comment

ishankhare07 commented Nov 8, 2023 • edited Loading

FabianKramm Nov 15, 2023

Choose a reason for hiding this comment

FabianKramm Nov 15, 2023

Choose a reason for hiding this comment

netlify bot commented Nov 15, 2023 • edited Loading

✅ Deploy Preview for vcluster-docs canceled.

ishankhare07 commented Nov 8, 2023 •

edited

Loading

netlify bot commented Nov 15, 2023 •

edited

Loading