Refactor and konnectivity #25

mendrugory · 2022-05-26T13:41:22Z

Refactoring to include custom group of resources
Konnectivity addon

prometherion

Missing Konnectivity code, please, add it.

config/crd/bases/kamaji.clastix.io_tenantcontrolplanes.yaml

config/install.yaml

internal/resources/k8s_deployment_resource.go

deploy/kind/kind-kamaji.yaml

internal/resources/k8s_deployment_resource.go

internal/resources/konnectivity/agent.go

api/v1alpha1/tenantcontrolplane_types.go

internal/resources/konnectivity/certificate_resource.go

internal/resources/konnectivity/cluster_role_binding_resource.go

internal/resources/konnectivity/certificate_resource.go

internal/utilities/utilities.go

prometherion · 2022-06-16T16:49:01Z

There's something broken with these changes since the creation of TCP stuck.

1.6553980106634037e+09  INFO    controller.tenantcontrolplane   kubeadmconfig has been configured       {"reconciler group": "kamaji.clastix.io", "reconciler kind": "TenantControlPlane", "name": "tenant-test", "namespace": "kamaji-system"}
1.6553980106703823e+09  INFO    controller.tenantcontrolplane   kubeadmconfig has been configured       {"reconciler group": "kamaji.clastix.io", "reconciler kind": "TenantControlPlane", "name": "tenant-test", "namespace": "kamaji-system"}
[certs] Generating "ca" certificate and key
1.6553980115440805e+09  INFO    controller.tenantcontrolplane   ca has been configured  {"reconciler group": "kamaji.clastix.io", "reconciler kind": "TenantControlPlane", "name": "tenant-test", "namespace": "kamaji-system"}
1.6553980115518756e+09  INFO    controller.tenantcontrolplane   ca has been configured  {"reconciler group": "kamaji.clastix.io", "reconciler kind": "TenantControlPlane", "name": "tenant-test", "namespace": "kamaji-system"}
1.655398011558895e+09   INFO    controller.tenantcontrolplane   ca has been configured  {"reconciler group": "kamaji.clastix.io", "reconciler kind": "TenantControlPlane", "name": "tenant-test", "namespace": "kamaji-system"}

edit

Reason is that e2e is testing the Helm installation, the new CRD definition hasn't been updated in that folder too.
@mendrugory please, add new definitions there too.

prometherion · 2022-06-17T12:45:15Z

api/v1alpha1/tenantcontrolplane_funcs.go

-func (in *TenantControlPlane) GetAddress(ctx context.Context, client client.Client) (string, error) {
+func (in *TenantControlPlane) GetAddress(ctx context.Context, client client.Client, namespacedName k8stypes.NamespacedName, specAddress string) (string, error) {


This does not entirely convince me: we got a pointer receiver with in *TenantControlPlane, which means we can access the struct data, however, we're taking as argument specAddress string used in this way.

kamaji/internal/resources/k8s_deployment_resource.go

Line 115 in 7205534

address, err := tenantControlPlane.GetAddress(ctx, r.Client, namespacedName, tenantControlPlane.Spec.NetworkProfile.Address)

Same here

kamaji/internal/resources/konnectivity/service_resource.go

Line 109 in 7205534

address, _ := tenantControlPlane.GetAddress(ctx, r.Client, namespacedName, tenantControlPlane.Spec.Addons.Konnectivity.ProxyHost)

Honestly, I'd go for different functions in order to decrease the possibility of typo in passing the wrong value, something as GetControlPlaneAddress and GetKonnectivityServerAddress.

That's the way of reusing the same logic without different implementations inside the funcion (one for the api server service and another for the konnectivity service)
From my opinion, we will increase the risk of bugs if we have several functions with the same logic (I have a TODO comment to decrease that approach in other parts of the project)

If we want to go for delegating the caller to specify the right address, I'd say we should use a function that is not bounded to the TenantControlPlane. Eventually, also an enum could be used.

I'd go for two functions here since the domain is slightly different (CP vs Konnectivity) and eventually we can replicate the same underlying logic by abstracting it.

We just need to avoid passing receiver struct members as parameters.

prometherion · 2022-06-17T12:49:09Z

api/v1alpha1/tenantcontrolplane_types.go

+	// +kubebuilder:default=8132
+	ProxyPort int32 `json:"proxyPort,omitempty"`


Port 8132 is not recommended here since we're deploying as NodePort type: unless the TCP is running with the host network (and it's not), Kubernetes is going to complain.

We could use a higher value in the range of allowed NodePorts (generally speaking, 30132), or avoid a default value since otherwise every different TCP could have collision.

IIRC, by not specifying a port, a new one should be assigned automatically: can't we go in that way? It means we would accept a *int32.

I would try to keep the default value (known one) in order to facilitate the use of kamaji to new comers. But going to port "auto-assignation" could increase a lot the complexity and I do not think that it deserves the effort because that port should be exposed, therefore, it will depend on the infrastructure where kamaji is deployed

The issue of a default value, since using a NodePort, would end up having multiple Tenant Control Plane w/ Konnectivity enabled, in which just the first one would work and be able to get a Service due to a collision.

I see two options, here:

nullable port, in order to get the allocated one by the Admin cluster

marking the field as required if the feature is enabled, with no default values

With the first one, we have to decide how the admin can retrieve that port: it could be stored in the KonnectivityStatus, as we're doing with the Kubeconfig.

With the latter one, unfortunately, we would need a webhook, and we don't have yet the support for it, and that would require an error handling in the controller.

if the solution is not clear, let's open a new improvement task

We have to address it right now since it's a new feature and we can shape the feature as we wish.

According to OAPIv3, there's no way on setting up a condition.

A possible workaround could be the following: allowing an empty object.

... addons: konnectivity: host: "fqdn.io" port: 30123 coreDNS: {} kubeProxy: {}

It means that all of the addons are activated since there are options.

Instead:

... addons: konnectivity: host: "fqdn.io" port: 30123 coreDNS: {}

in this case kubeProxy is disabled because we don't have any option declared, it's nil.

The benefit of this approach is that we can make the port required only when the object is declared, and not on the condition of enabled=true.

This is a refactoring, that must be addressed before the konnectivity feature in a different commit of this PR.

prometherion · 2022-06-20T10:56:34Z

Makefile

@@ -97,6 +97,7 @@ kustomize: ## Download kustomize locally if necessary.

 manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
 	$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
+	cp config/crd/bases/kamaji.clastix.io_tenantcontrolplanes.yaml helm/kamaji/crds/tenantcontrolplane.yaml


That's great, well done!

The lack of this kind of automation was provoking the e2e failure

prometherion · 2022-06-20T11:00:18Z

internal/utilities/utilities.go

+func IsValidIPV4(ip string) bool {
+	pattern := "^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$"
+
+	return validateRegex(pattern, ip)
+}


Why not use net.ParseIP here?

no idea about that function. I will change

prometherion · 2022-06-20T12:47:40Z

internal/resources/konnectivity/utils.go

+// TODO: refactor and merge with /internal/resources/kubeadm_utils.go
+// Logic is pretty close


Please, could you open an issue for this @mendrugory?

prometherion · 2022-06-20T12:49:40Z

api/v1alpha1/tenantcontrolplane_types.go

-	KubeProxy AddonSpec `json:"kubeProxy,omitempty"`
+	CoreDNS      *AddonSpec        `json:"coreDNS,omitempty"`
+	Konnectivity *KonnectivitySpec `json:"konnectivity,omitempty"`
+	KubeProxy    *AddonSpec        `json:"kubeProxy,omitempty"`


Since the changes, I'm having trouble understanding if we're checking the pointers here.
Did you enforce the check of the pointers? I'd like to avoid issues with nil pointer dereference.

Logic about kubeadm addons was changed into internal/resources/kubeadm_addons.go

let me know if you see anything weird

prometherion · 2022-06-20T13:20:24Z

helm/kamaji/README.md

+| addons.coreDNS | boolean |  | Enabling CoreDNS installation. |
+| addons.kubeProxy | boolean |  | Enabling KubeProxy installation |


Wrong type here, it shouldn't be boolean, but rather, object.

* cleaning code * group of resources and code improvements * addons * manifest for helm

mendrugory force-pushed the gonzalo/issues/10 branch from f6d0826 to 4007d99 Compare May 26, 2022 13:55

prometherion requested changes May 26, 2022

View reviewed changes

config/crd/bases/kamaji.clastix.io_tenantcontrolplanes.yaml Outdated Show resolved Hide resolved

config/install.yaml Outdated Show resolved Hide resolved

mendrugory force-pushed the gonzalo/issues/10 branch 7 times, most recently from c296737 to 90bffda Compare June 3, 2022 14:30

mendrugory force-pushed the gonzalo/issues/10 branch from 90bffda to 7e98d31 Compare June 7, 2022 15:23

bsctl reviewed Jun 7, 2022

View reviewed changes

internal/resources/k8s_deployment_resource.go Outdated Show resolved Hide resolved

mendrugory force-pushed the gonzalo/issues/10 branch 6 times, most recently from fc18228 to 306590d Compare June 10, 2022 14:10

prometherion requested changes Jun 13, 2022

View reviewed changes

mendrugory force-pushed the gonzalo/issues/10 branch 10 times, most recently from 6bc47ce to 2240c18 Compare June 16, 2022 14:13

mendrugory force-pushed the gonzalo/issues/10 branch from 2240c18 to 317aaf3 Compare June 17, 2022 09:58

mendrugory force-pushed the gonzalo/issues/10 branch 5 times, most recently from e313547 to 7205534 Compare June 17, 2022 11:03

mendrugory changed the title ~~WIP: Refactor and konnectivity~~ Refactor and konnectivity Jun 17, 2022

mendrugory requested a review from prometherion June 17, 2022 11:14

prometherion requested changes Jun 17, 2022

View reviewed changes

mendrugory force-pushed the gonzalo/issues/10 branch 4 times, most recently from d13eede to c9bf06e Compare June 20, 2022 10:36

prometherion reviewed Jun 20, 2022

View reviewed changes

prometherion requested changes Jun 20, 2022

View reviewed changes

mendrugory force-pushed the gonzalo/issues/10 branch 2 times, most recently from 4fa234f to 35ae2e6 Compare June 20, 2022 11:14

prometherion reviewed Jun 20, 2022

View reviewed changes

prometherion approved these changes Jun 20, 2022

View reviewed changes

prometherion requested changes Jun 20, 2022

View reviewed changes

mendrugory added 2 commits June 20, 2022 15:29

refactor:

fc5eb3a

* cleaning code * group of resources and code improvements * addons * manifest for helm

feat: konnectivity

31e7edc

mendrugory force-pushed the gonzalo/issues/10 branch from 35ae2e6 to 31e7edc Compare June 20, 2022 13:30

mendrugory merged commit 3be6cf1 into clastix:master Jun 20, 2022

mendrugory deleted the gonzalo/issues/10 branch June 20, 2022 13:53

bsctl mentioned this pull request Jun 22, 2022

Provide Konnectivity integration as an add-on #10

Closed

prometherion mentioned this pull request Jul 8, 2022

Tenant Control Plane is not fully reconciled #73

Closed

prometherion mentioned this pull request Sep 19, 2022

Release v0.1.0 #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and konnectivity #25

Refactor and konnectivity #25

mendrugory commented May 26, 2022 •

edited

prometherion left a comment

prometherion commented Jun 16, 2022 •

edited

prometherion Jun 17, 2022

mendrugory Jun 17, 2022

prometherion Jun 17, 2022

prometherion Jun 17, 2022

mendrugory Jun 17, 2022

prometherion Jun 17, 2022

mendrugory Jun 17, 2022

prometherion Jun 17, 2022

prometherion Jun 20, 2022

mendrugory Jun 20, 2022

prometherion Jun 20, 2022

mendrugory Jun 20, 2022

prometherion Jun 20, 2022

prometherion Jun 20, 2022

mendrugory Jun 20, 2022

prometherion Jun 20, 2022

		func (in *TenantControlPlane) GetAddress(ctx context.Context, client client.Client) (string, error) {
		func (in *TenantControlPlane) GetAddress(ctx context.Context, client client.Client, namespacedName k8stypes.NamespacedName, specAddress string) (string, error) {

		// +kubebuilder:default=8132
		ProxyPort int32 `json:"proxyPort,omitempty"`

		// TODO: refactor and merge with /internal/resources/kubeadm_utils.go
		// Logic is pretty close

		\| addons.coreDNS \| boolean \| \| Enabling CoreDNS installation. \|
		\| addons.kubeProxy \| boolean \| \| Enabling KubeProxy installation \|

Refactor and konnectivity #25

Refactor and konnectivity #25

Conversation

mendrugory commented May 26, 2022 • edited

prometherion left a comment

Choose a reason for hiding this comment

prometherion commented Jun 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mendrugory commented May 26, 2022 •

edited

prometherion commented Jun 16, 2022 •

edited