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

merge vCluster charts & new values.yaml #1583

Merged
merged 6 commits into from
Mar 15, 2024
Merged

Conversation

FabianKramm
Copy link
Member

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

Copy link

netlify bot commented Mar 5, 2024

Deploy Preview for vcluster-docs canceled.

Name Link
🔨 Latest commit f1e599e
🔍 Latest deploy log https://app.netlify.com/sites/vcluster-docs/deploys/65f434f00cda7c0008984849

@FabianKramm FabianKramm changed the title DRAFT: add new merged vcluster chart add new merged vcluster chart Mar 14, 2024
Copy link
Contributor

@facchettos facchettos left a comment

Choose a reason for hiding this comment

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

I left a few comments mainly on the config part, will continue tomorrow first thing

.golangci.yml Show resolved Hide resolved
Comment on lines +45 to +69
case config.K3SDistro:
distroConfig = config.VirtualClusterKubeConfig{
KubeConfig: "/data/server/cred/admin.kubeconfig",
ServerCAKey: "/data/server/tls/server-ca.key",
ServerCACert: "/data/server/tls/server-ca.crt",
ClientCACert: "/data/server/tls/client-ca.crt",
RequestHeaderCACert: "/data/server/tls/request-header-ca.crt",
}
case config.K0SDistro:
distroConfig = config.VirtualClusterKubeConfig{
KubeConfig: "/data/k0s/pki/admin.conf",
ServerCAKey: "/data/k0s/pki/ca.key",
ServerCACert: "/data/k0s/pki/ca.crt",
ClientCACert: "/data/k0s/pki/ca.crt",
RequestHeaderCACert: "/data/k0s/pki/front-proxy-ca.crt",
}
case config.EKSDistro, config.K8SDistro:
distroConfig = config.VirtualClusterKubeConfig{
KubeConfig: "/pki/admin.conf",
ServerCAKey: "/pki/ca.key",
ServerCACert: "/pki/ca.crt",
ClientCACert: "/pki/ca.crt",
RequestHeaderCACert: "/pki/front-proxy-ca.crt",
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this be a good occasion to put the certs in the same directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

problem is that the certs are generated not always by us and are called different even if in the same directory. Its a good idea, but I would separate this change from this PR

Comment on lines +71 to +86
retConfig := v.Config.Experimental.VirtualClusterKubeConfig
if retConfig.KubeConfig == "" {
retConfig.KubeConfig = distroConfig.KubeConfig
}
if retConfig.ClientCACert == "" {
retConfig.ClientCACert = distroConfig.ClientCACert
}
if retConfig.ServerCAKey == "" {
retConfig.ServerCAKey = distroConfig.ServerCAKey
}
if retConfig.ServerCACert == "" {
retConfig.ServerCACert = distroConfig.ServerCACert
}
if retConfig.RequestHeaderCACert == "" {
retConfig.RequestHeaderCACert = distroConfig.RequestHeaderCACert
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use something like mergo to set those ? That would simplify the process

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess its not too bad to have this explicit for now since its just 5 fields and people don't know how mergo operates and we rely on another dependency for 10 lines of code

pkg/config/validation.go Outdated Show resolved Hide resolved
Comment on lines +29 to +31
if v.Config.ControlPlane.Distro.K3S.Enabled {
return config.K3SDistro
} else if v.Config.ControlPlane.Distro.K0S.Enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little redundant no? without that if the function is the same

Copy link
Member Author

@FabianKramm FabianKramm Mar 15, 2024

Choose a reason for hiding this comment

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

Thing is more that we want to have k3s if all are disabled, so we need that function

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah but it's not the point, if you lose

	if v.Config.ControlPlane.Distro.K3S.Enabled {
		return config.K3SDistro
	} else

the end result is the same still. because the default will be to return k3s

Comment on lines +22 to +24
var (
verbs = []string{"get", "list", "create", "update", "patch", "watch", "delete", "deletecollection"}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add enums with jsonschema using tags so that will even work within the helm chart

Copy link
Member Author

Choose a reason for hiding this comment

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

We need this within golang code as well, e.g. if people don't use our helm chart

Copy link
Contributor

Choose a reason for hiding this comment

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

but now if they do only use our helm charts they don't have the validation
We can run jsonschema validator within go and get the errors, that way validation would be consistent across the board

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/santhosh-tekuri/jsonschema/tree/v5.3.1 for example would allow us to validate the schema for everything that we can, and then rely on go code to only validate things that aren't possible with the schema
That also would allow us to be able to validate for more than 1 version when we introduce breaking changes

pkg/config/validation.go Outdated Show resolved Hide resolved
pkg/config/validation.go Show resolved Hide resolved
pkg/config/validation.go Show resolved Hide resolved
pkg/config/validation.go Outdated Show resolved Hide resolved
cmd/vcluster/cmd/start.go Show resolved Hide resolved
cmd/vclusterctl/cmd/create.go Outdated Show resolved Hide resolved
@FabianKramm FabianKramm changed the title add new merged vcluster chart merge vCluster charts & new values.yaml Mar 15, 2024
@FabianKramm FabianKramm merged commit 35794ea into loft-sh:main Mar 15, 2024
70 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.

3 participants