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.

Sign up for GitHub

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

Fix api service reg #1337

Merged
merged 17 commits into from
Nov 15, 2023
Merged

Fix api service reg #1337

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
895e546
fix: allow custom k3s token
FabianKramm Oct 26, 2023
729b8b9
aux metrics proxy svc
ishankhare07 Sep 29, 2023
acf2680
sync endpoints and detect single binary distros
ishankhare07 Oct 31, 2023
b26d41c
fix values test
ishankhare07 Oct 31, 2023
c416093
fix lint errors
ishankhare07 Oct 31, 2023
6202d06
only add SANs when not in single binary distro
ishankhare07 Oct 31, 2023
a823b69
remove unwanted configmap
ishankhare07 Oct 31, 2023
a26f223
modify eks chart for api registration fix
ishankhare07 Oct 31, 2023
0b9273d
move service creation from chart to code
ishankhare07 Nov 2, 2023
3fac312
add deletion of aux services in case of toggle
ishankhare07 Nov 2, 2023
7b39ff4
add e2e for metrics server api registration check
ishankhare07 Nov 3, 2023
c98aba1
fix metrics server helm chart install
ishankhare07 Nov 3, 2023
eecb935
modify special service default setup and remove env var
ishankhare07 Nov 9, 2023
05b917e
add tests for metrics proxy - node and pod metrics
ishankhare07 Nov 9, 2023
c6d0d85
fix metrics server chart installation
ishankhare07 Nov 9, 2023
438d8ac
remove single bin flag, utilize existing distro detection mechanism
ishankhare07 Nov 14, 2023
f742c4e
refactor: use cache correctly
FabianKramm Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions charts/eks/templates/metrics-proxy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
ishankhare07 marked this conversation as resolved.
Show resolved Hide resolved
apiVersion: v1
kind: Service
metadata:
name: {{ .Release.Name }}-metrics-proxy
namespace: {{ .Release.Namespace }}
labels:
app: vcluster-metrics-proxy
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: "{{ .Release.Name }}"
heritage: "{{ .Release.Service }}"
{{- $annotations := merge .Values.globalAnnotations .Values.syncer.serviceAnnotations }}
{{- if $annotations }}
annotations:
{{ toYaml $annotations | indent 4 }}
{{- end }}
spec:
ports:
- name: https
port: 443
targetPort: 8443
protocol: TCP
- name: kubelet
port: 10250
targetPort: 8443
protocol: TCP
selector:
app: vcluster
release: {{ .Release.Name }}
---
apiVersion: v1
ishankhare07 marked this conversation as resolved.
Show resolved Hide resolved
kind: ConfigMap
metadata:
name: {{ .Release.Name }}-metrics-proxy
namespace: {{ .Release.Namespace }}
{{- if .Values.globalAnnotations }}
annotations:
{{ toYaml .Values.globalAnnotations | indent 4 }}
{{- end }}
data:
service.yaml: |-
apiVersion: v1
kind: Service
metadata:
labels:
k8s-app: metrics-server
name: metrics-server
namespace: kube-system
spec:
ports:
- name: https
port: 443
protocol: TCP
targetPort: 8443
selector:
k8s-app: metrics-server
sessionAffinity: None
type: ClusterIP
{{- end }}
16 changes: 15 additions & 1 deletion charts/eks/templates/syncer-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ spec:
configMap:
name: coredns-custom
optional: true
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- name: metrics-proxy-svc
configMap:
name: {{ .Release.Name }}-metrics-proxy
{{- end }}
{{- if .Values.syncer.priorityClassName }}
priorityClassName: {{ .Values.syncer.priorityClassName }}
{{- end }}
Expand All @@ -118,7 +123,7 @@ spec:
{{- if not .Values.syncer.noArgs }}
args:
- --name={{ .Release.Name }}
- --request-header-ca-cert=/pki/ca.crt
- --request-header-ca-cert=/pki/front-proxy-ca.crt
- --client-ca-cert=/pki/ca.crt
- --server-ca-cert=/pki/ca.crt
- --server-ca-key=/pki/ca.key
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

{{- end }}
{{- if .Values.coredns.integrated }}
- --integrated-coredns=true
Expand Down Expand Up @@ -228,13 +234,21 @@ spec:
{{- end }}
- name: VCLUSTER_TELEMETRY_CONFIG
value: {{ .Values.telemetry | toJson | quote }}
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- name: SINGLE_BINARY_DISTRO
value: "false"
{{- end }}
volumeMounts:
- name: helm-cache
mountPath: /.cache/helm
- name: tmp
mountPath: /tmp
- mountPath: /pki
name: certs
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- name: metrics-proxy-svc
mountPath: /manifests/metrics-server
{{- end }}
{{- if .Values.syncer.volumeMounts }}
{{ toYaml .Values.syncer.volumeMounts | indent 10 }}
{{- end }}
Expand Down
59 changes: 59 additions & 0 deletions charts/k8s/templates/metrics-proxy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
apiVersion: v1
kind: Service
metadata:
name: {{ .Release.Name }}-metrics-proxy
namespace: {{ .Release.Namespace }}
labels:
app: vcluster-metrics-proxy
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: "{{ .Release.Name }}"
heritage: "{{ .Release.Service }}"
{{- $annotations := merge .Values.globalAnnotations .Values.syncer.serviceAnnotations }}
{{- if $annotations }}
annotations:
{{ toYaml $annotations | indent 4 }}
{{- end }}
spec:
ports:
- name: https
port: 443
targetPort: 8443
protocol: TCP
- name: kubelet
port: 10250
targetPort: 8443
protocol: TCP
selector:
app: vcluster
release: {{ .Release.Name }}
---
apiVersion: v1
kind: ConfigMap
metadata:
name: {{ .Release.Name }}-metrics-proxy
namespace: {{ .Release.Namespace }}
{{- if .Values.globalAnnotations }}
annotations:
{{ toYaml .Values.globalAnnotations | indent 4 }}
{{- end }}
data:
service.yaml: |-
apiVersion: v1
kind: Service
metadata:
labels:
k8s-app: metrics-server
name: metrics-server
namespace: kube-system
spec:
ports:
- name: https
port: 443
protocol: TCP
targetPort: 8443
selector:
k8s-app: metrics-server
sessionAffinity: None
type: ClusterIP
{{- end }}
16 changes: 15 additions & 1 deletion charts/k8s/templates/syncer-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ spec:
configMap:
name: coredns-custom
optional: true
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- name: metrics-proxy-svc
configMap:
name: {{ .Release.Name }}-metrics-proxy
{{- end }}
{{- if .Values.syncer.priorityClassName }}
priorityClassName: {{ .Values.syncer.priorityClassName }}
{{- end }}
Expand All @@ -148,7 +153,7 @@ spec:
{{- if not .Values.syncer.noArgs }}
args:
- --name={{ .Release.Name }}
- --request-header-ca-cert=/pki/ca.crt
- --request-header-ca-cert=/pki/front-proxy-ca.crt
- --client-ca-cert=/pki/ca.crt
- --server-ca-cert=/pki/ca.crt
- --server-ca-key=/pki/ca.key
Expand Down Expand Up @@ -202,6 +207,7 @@ spec:
{{- end }}
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- --proxy-metrics-server=true
- --single-binary-distro=false
{{- end }}
{{- if .Values.coredns.integrated }}
- --integrated-coredns=true
Expand Down Expand Up @@ -261,6 +267,10 @@ spec:
{{- end }}
- name: VCLUSTER_TELEMETRY_CONFIG
value: {{ .Values.telemetry | toJson | quote }}
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- name: SINGLE_BINARY_DISTRO
value: "false"
{{- end }}
volumeMounts:
- name: helm-cache
mountPath: /.cache/helm
Expand All @@ -273,6 +283,10 @@ spec:
mountPath: /manifests/coredns
readOnly: true
{{- end }}
{{- if or .Values.proxy.metricsServer.nodes.enabled .Values.proxy.metricsServer.pods.enabled }}
- name: metrics-proxy-svc
mountPath: /manifests/metrics-server
{{- end }}
{{- if .Values.syncer.volumeMounts }}
{{ toYaml .Values.syncer.volumeMounts | indent 10 }}
{{- end }}
Expand Down
6 changes: 6 additions & 0 deletions devspace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ deployments:
disabled: "false"
instanceCreatorType: "devspace"
instanceCreatorUID: "devspace"
proxy:
metricsServer:
nodes:
enabled: true
pods:
enabled: true
sync:
generic:
clusterRole:
Expand Down
94 changes: 93 additions & 1 deletion pkg/controllers/k8sdefaultendpoint/k8sdefaultendpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/loft-sh/vcluster/pkg/setup/options"

"github.com/loft-sh/vcluster/pkg/specialservices"
"github.com/loft-sh/vcluster/pkg/util/loghelper"
corev1 "k8s.io/api/core/v1"
discoveryv1 "k8s.io/api/discovery/v1"
Expand Down Expand Up @@ -40,6 +41,8 @@ type EndpointController struct {
Log loghelper.Logger

provider provider

singleBinaryDistro bool
}

func NewEndpointController(ctx *options.ControllerContext, provider provider) *EndpointController {
Expand All @@ -51,6 +54,7 @@ func NewEndpointController(ctx *options.ControllerContext, provider provider) *E
VirtualManagerCache: ctx.VirtualManager.GetCache(),
Log: loghelper.New("kubernetes-default-endpoint-controller"),
provider: provider,
singleBinaryDistro: ctx.Options.SingleBinaryDistro,
}
}

Expand All @@ -67,6 +71,14 @@ func (e *EndpointController) Reconcile(ctx context.Context, _ ctrl.Request) (ctr
if err != nil {
return ctrl.Result{RequeueAfter: time.Second}, err
}

// TODO: Change this to only be set for k8s/eks distros and when using metrics proxy
ThomasK33 marked this conversation as resolved.
Show resolved Hide resolved
if !e.singleBinaryDistro {
err = e.syncMetricsServerEndpoints(ctx, e.VirtualClient, e.LocalClient, e.ServiceName, e.ServiceNamespace)
if err != nil {
return ctrl.Result{RequeueAfter: time.Second}, err
}
}
return ctrl.Result{}, nil
}

Expand All @@ -79,7 +91,11 @@ func (e *EndpointController) SetupWithManager(mgr ctrl.Manager) error {
pfuncs := predicate.NewPredicateFuncs(pp)

vp := func(object client.Object) bool {
return object.GetNamespace() == "default" && object.GetName() == "kubernetes"
if object.GetNamespace() == specialservices.DefaultKubernetesSvcKey.Namespace && object.GetName() == specialservices.DefaultKubernetesSvcKey.Name {
return true
}

return false
}
vfuncs := predicate.NewPredicateFuncs(vp)

Expand All @@ -94,6 +110,82 @@ func (e *EndpointController) SetupWithManager(mgr ctrl.Manager) error {
Complete(e)
}

func (e *EndpointController) syncMetricsServerEndpoints(ctx context.Context, virtualClient, localClient client.Client, serviceName, serviceNamespace string) error {
// get physical service endpoints
pEndpoints := &corev1.Endpoints{}
err := localClient.Get(ctx, types.NamespacedName{
Namespace: serviceNamespace,
Name: serviceName,
}, pEndpoints)
if err != nil {
if kerrors.IsNotFound(err) {
return nil
}

return err
}

vEndpoints := &corev1.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: specialservices.VclusterProxyMetricsSvcKey.Namespace,
Name: specialservices.VclusterProxyMetricsSvcKey.Name,
},
}

result, err := controllerutil.CreateOrPatch(ctx, virtualClient, vEndpoints, func() error {
if vEndpoints.Labels == nil {
vEndpoints.Labels = map[string]string{}
}
// vEndpoints.Labels[discoveryv1.LabelSkipMirror] = "true"
Copy link
Member

Choose a reason for hiding this comment

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

why is this commented out?


// build new subsets
newSubsets := []corev1.EndpointSubset{}
for _, subset := range pEndpoints.Subsets {
newPorts := []corev1.EndpointPort{}
for _, p := range subset.Ports {
if p.Name != "https" {
continue
}

newPorts = append(newPorts, p)
}

newAddresses := []corev1.EndpointAddress{}
for _, address := range subset.Addresses {
address.Hostname = ""
address.NodeName = nil
address.TargetRef = nil
newAddresses = append(newAddresses, address)
}
newNotReadyAddresses := []corev1.EndpointAddress{}
for _, address := range subset.NotReadyAddresses {
address.Hostname = ""
address.NodeName = nil
address.TargetRef = nil
newNotReadyAddresses = append(newNotReadyAddresses, address)
}

newSubsets = append(newSubsets, corev1.EndpointSubset{
Addresses: newAddresses,
NotReadyAddresses: newNotReadyAddresses,
Ports: newPorts,
})
}

vEndpoints.Subsets = newSubsets
return nil
})
if err != nil {
return nil
}

if result == controllerutil.OperationResultCreated || result == controllerutil.OperationResultUpdated {
return e.provider.createOrPatch(ctx, virtualClient, vEndpoints)
}

return err
}

func (e *EndpointController) syncKubernetesServiceEndpoints(ctx context.Context, virtualClient client.Client, localClient client.Client, serviceName, serviceNamespace string) error {
// get physical service endpoints
pEndpoints := &corev1.Endpoints{}
Expand Down
4 changes: 1 addition & 3 deletions pkg/controllers/resources/endpoints/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ func (s *endpointsSyncer) Sync(ctx *synccontext.SyncContext, pObj client.Object,
var _ syncer.Starter = &endpointsSyncer{}

func (s *endpointsSyncer) ReconcileStart(ctx *synccontext.SyncContext, req ctrl.Request) (bool, error) {
if req.Namespace == "default" && req.Name == "kubernetes" {
return true, nil
} else if _, ok := specialservices.Default.SpecialServicesToSync()[req.NamespacedName]; ok {
ishankhare07 marked this conversation as resolved.
Show resolved Hide resolved
if req.NamespacedName == specialservices.DefaultKubernetesSvcKey {
return true, nil
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/controllers/resources/services/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ var _ syncertypes.Starter = &serviceSyncer{}
func (s *serviceSyncer) ReconcileStart(ctx *synccontext.SyncContext, req ctrl.Request) (bool, error) {
// don't do anything for the kubernetes service
specialServices := specialservices.Default.SpecialServicesToSync()
if svc, ok := specialServices[req.NamespacedName]; ok {

svc, ok := specialServices[req.NamespacedName]
if ok {
return true, svc(ctx, ctx.CurrentNamespace, s.serviceName, req.NamespacedName, TranslateServicePorts)
}

Expand Down
Loading
Loading