-
Notifications
You must be signed in to change notification settings - Fork 173
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: moving cli config to --set for nodeport and storageclass #2083
Changes from all commits
6a17d66
af54703
b79bae9
74570e0
3c0e9d5
3005fb6
d67f9e8
697554f
4fbadc0
f49e4ad
6576d6b
d5f7d35
0a7817f
4cd06a1
cf0d90a
83175e1
7c2fde7
e355116
ae7eff1
c1ed163
b12026d
8042767
5874c1c
8240867
8954308
431a292
5c32128
4fe956c
7a043a6
a5d9cf2
6698814
20f0821
878d29d
294ffbe
f36a71d
4e977f1
5d21c11
43acd57
db54848
15eb1a8
5e7e182
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,16 @@ variables: | |
default: "" | ||
autoIndent: true | ||
|
||
- name: REGISTRY_NODEPORT | ||
description: NodePort for the registry | ||
default: "31999" | ||
autoIndent: true | ||
|
||
- name: REGISTRY_STORAGE_CLASS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a corresponding variable for the GIT_SERVER |
||
description: StorageClass for the registry PVC | ||
default: "" | ||
autoIndent: true | ||
|
||
constants: | ||
- name: REGISTRY_IMAGE | ||
value: "###ZARF_PKG_TMPL_REGISTRY_IMAGE###" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be more consistent with other deprecated fields/flags in the codebase, it would probably be good to rename the deprecated fields to |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import ( | |
"os" | ||
"path" | ||
"path/filepath" | ||
"strconv" | ||
"strings" | ||
|
||
"github.com/AlecAivazis/survey/v2" | ||
|
@@ -26,6 +27,9 @@ import ( | |
"github.com/spf13/cobra" | ||
) | ||
|
||
var nodePortArg = 0 | ||
var storageClassArg = "" | ||
|
||
// initCmd represents the init command. | ||
var initCmd = &cobra.Command{ | ||
Use: "init", | ||
|
@@ -61,6 +65,10 @@ var initCmd = &cobra.Command{ | |
pkgConfig.PkgOpts.SetVariables = helpers.TransformAndMergeMap( | ||
v.GetStringMapString(common.VPkgDeploySet), pkgConfig.PkgOpts.SetVariables, strings.ToUpper) | ||
|
||
// DEPRECATED_V1.0.0: these functions will need cleanup | ||
setRegistryStorageClass() | ||
setRegistryNodePort() | ||
|
||
// Configure the packager | ||
pkgClient := packager.NewOrDie(&pkgConfig, packager.WithSource(src)) | ||
defer pkgClient.ClearTempPaths() | ||
|
@@ -73,6 +81,62 @@ var initCmd = &cobra.Command{ | |
}, | ||
} | ||
|
||
// DEPRECATED_V1.0.0: --nodeport should be removed from the cli in v1.0.0 | ||
func setRegistryNodePort() { | ||
configVar := "REGISTRY_NODEPORT" | ||
|
||
internalRegistry := pkgConfig.InitOpts.RegistryInfo.Address == "" | ||
|
||
// warn for deprecation | ||
if nodePortArg != 0 { | ||
message.Warn(lang.WarnNodePortDeprecated) | ||
} | ||
|
||
// check the --set REGISTRY_NODEPORT first | ||
configuredNodePort, err := strconv.Atoi(pkgConfig.PkgOpts.SetVariables[configVar]) | ||
if err != nil { | ||
configuredNodePort = 0 | ||
} | ||
|
||
// check the old --nodeport flag second | ||
if configuredNodePort == 0 { | ||
configuredNodePort = nodePortArg | ||
} | ||
|
||
// the user can't set both that this is an external registry and the nodeport stuff. | ||
if !internalRegistry && configuredNodePort != 0 { | ||
message.Fatal(nil, "both --registry-url and --nodeport are set, please only use one") | ||
} | ||
|
||
if internalRegistry { | ||
if configuredNodePort > 32767 || configuredNodePort < 30000 { | ||
configuredNodePort = config.ZarfInClusterContainerRegistryNodePort | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would probably be good to notify the user that they provided an invalid nodeport number and that we're going to use the default Zarf nodeport of |
||
} | ||
pkgConfig.PkgOpts.SetVariables[configVar] = strconv.Itoa(configuredNodePort) | ||
pkgConfig.InitOpts.RegistryInfo.Address = fmt.Sprintf("%s:%d", helpers.IPV4Localhost, configuredNodePort) | ||
pkgConfig.InitOpts.RegistryInfo.NodePort = configuredNodePort | ||
} else { | ||
// do not set the nodeport if this is an external registry | ||
pkgConfig.PkgOpts.SetVariables[configVar] = "" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be able to remove this else block. If the user specifies an external registry, we shouldn't have to set the |
||
} | ||
|
||
pkgConfig.InitOpts.RegistryInfo.InternalRegistry = internalRegistry | ||
|
||
// TODO: test external registry and CLI. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo |
||
} | ||
|
||
// DEPRECATED_V1.0.0: --storage-class should be removed from the CLI in v1.0.0 | ||
func setRegistryStorageClass() { | ||
configVar := "REGISTRY_STORAGE_CLASS" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would set both the registry and git server vars |
||
// there is no validation if this storage class is valid | ||
if storageClassArg != "" { | ||
message.Warn(lang.WarnStorageClassDeprecated) | ||
} | ||
if pkgConfig.PkgOpts.SetVariables[configVar] == "" { | ||
pkgConfig.PkgOpts.SetVariables[configVar] = storageClassArg | ||
} | ||
} | ||
|
||
func findInitPackage(initPackageName string) (string, error) { | ||
// First, look for the init package in the current working directory | ||
if !utils.InvalidPath(initPackageName) { | ||
|
@@ -188,7 +252,10 @@ func init() { | |
// Continue to require --confirm flag for init command to avoid accidental deployments | ||
initCmd.Flags().BoolVar(&config.CommonOptions.Confirm, "confirm", false, lang.CmdInitFlagConfirm) | ||
initCmd.Flags().StringVar(&pkgConfig.PkgOpts.OptionalComponents, "components", v.GetString(common.VInitComponents), lang.CmdInitFlagComponents) | ||
initCmd.Flags().StringVar(&pkgConfig.InitOpts.StorageClass, "storage-class", v.GetString(common.VInitStorageClass), lang.CmdInitFlagStorageClass) | ||
|
||
// [Deprecated] --storage-class is deprecated in favor of --set REGISTRY_STORAGE_CLASS (to be removed in v1.0.0) | ||
initCmd.Flags().StringVar(&storageClassArg, "storage-class", v.GetString(common.VInitStorageClass), lang.CmdInitFlagStorageClass) | ||
initCmd.Flags().MarkHidden("storage-class") | ||
|
||
// Flags for using an external Git server | ||
initCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.Address, "git-url", v.GetString(common.VInitGitURL), lang.CmdInitFlagGitURL) | ||
|
@@ -199,7 +266,11 @@ func init() { | |
|
||
// Flags for using an external registry | ||
initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.Address, "registry-url", v.GetString(common.VInitRegistryURL), lang.CmdInitFlagRegURL) | ||
initCmd.Flags().IntVar(&pkgConfig.InitOpts.RegistryInfo.NodePort, "nodeport", v.GetInt(common.VInitRegistryNodeport), lang.CmdInitFlagRegNodePort) | ||
|
||
// [Deprecated] --nodeport is deprecated in favor of --set REGISTRY_NODEPORT | ||
initCmd.Flags().IntVar(&nodePortArg, "nodeport", v.GetInt(common.VInitRegistryNodeport), lang.CmdInitFlagRegNodePort) | ||
initCmd.Flags().MarkHidden("nodeport") | ||
|
||
initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PushUsername, "registry-push-username", v.GetString(common.VInitRegistryPushUser), lang.CmdInitFlagRegPushUser) | ||
initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PushPassword, "registry-push-password", v.GetString(common.VInitRegistryPushPass), lang.CmdInitFlagRegPushPass) | ||
initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PullUsername, "registry-pull-username", v.GetString(common.VInitRegistryPullUser), lang.CmdInitFlagRegPullUser) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
// DEPRECATED_V1.0.0: do not check pkgConfig.InitOpts.RegistryInfo.NodePort, always overwrite it. | ||
package cmd | ||
|
||
import ( | ||
"testing" | ||
) | ||
|
||
func TestSetRegistryStorageClass(t *testing.T) { | ||
// Test case 1: storageClass is set | ||
pkgConfig.PkgOpts.SetVariables = map[string]string{"REGISTRY_STORAGE_CLASS": "test-storage-class"} | ||
storageClassArg = "" | ||
setRegistryStorageClass() | ||
if storageClassArg != "test-storage-class" { | ||
t.Errorf("Expected storage class to be set to 'test-storage-class', but got '%s'", storageClassArg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use |
||
} | ||
|
||
// Test case 2: storageClass is not set, use old way | ||
pkgConfig.PkgOpts.SetVariables = map[string]string{} | ||
storageClassArg = "old-storage-class" | ||
setRegistryStorageClass() | ||
if pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"] != "old-storage-class" { | ||
t.Errorf("Expected storage class to be set to old way value, but got '%s'", pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use |
||
} | ||
|
||
// Test case 3: neither is set, should be empty | ||
pkgConfig.PkgOpts.SetVariables = map[string]string{} | ||
storageClassArg = "" | ||
setRegistryStorageClass() | ||
if pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"] != "" { | ||
t.Errorf("Expected storage class to be set to empty string, but got '%s'", pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use |
||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -31,6 +31,13 @@ const ( | |||||
ErrFileNameExtract = "failed to extract filename from URL %s: %s" | ||||||
) | ||||||
|
||||||
// Zarf CLI deprecation warnings | ||||||
const ( | ||||||
// remove deprecation in v1.0.0 | ||||||
WarnStorageClassDeprecated = "--storage-class <value> is deprecated, please use --set REGISTRY_STORAGE_CLASS=<value>" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
WarnNodePortDeprecated = "--nodeport <value> is deprecated, please use --set REGISTRY_NODEPORT=<value>" | ||||||
) | ||||||
|
||||||
// Zarf CLI commands. | ||||||
const ( | ||||||
// common command language | ||||||
|
@@ -128,7 +135,7 @@ $ zarf init --components=git-server | |||||
$ zarf init --components=git-server,logging | ||||||
|
||||||
# Initializing w/ an internal registry but with a different nodeport: | ||||||
$ zarf init --nodeport=30333 | ||||||
$ zarf init --set REGISTRY_NODEPORT=30333 | ||||||
|
||||||
# Initializing w/ an external registry: | ||||||
$ zarf init --registry-push-password={PASSWORD} --registry-push-username={USERNAME} --registry-url={URL} | ||||||
|
@@ -158,7 +165,7 @@ $ zarf init --artifact-push-password={PASSWORD} --artifact-push-username={USERNA | |||||
|
||||||
CmdInitFlagConfirm = "Confirms package deployment without prompting. ONLY use with packages you trust. Skips prompts to review SBOM, configure variables, select optional components and review potential breaking changes." | ||||||
CmdInitFlagComponents = "Specify which optional components to install. E.g. --components=git-server,logging" | ||||||
CmdInitFlagStorageClass = "Specify the storage class to use for the registry and git server. E.g. --storage-class=standard" | ||||||
CmdInitFlagStorageClass = "[DEPRECATED] This command has been replaced by 'zarf init --set REGISTRY_STORAGE_CLASS=<value>' and will be removed in Zarf v1.0.0" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This should match other deprecated flags (see |
||||||
|
||||||
CmdInitFlagGitURL = "External git server url to use for this Zarf cluster" | ||||||
CmdInitFlagGitPushUser = "Username to access to the git server Zarf is configured to use. User must be able to create repositories via 'git push'" | ||||||
|
@@ -167,7 +174,7 @@ $ zarf init --artifact-push-password={PASSWORD} --artifact-push-username={USERNA | |||||
CmdInitFlagGitPullPass = "Password for the pull-only user to access the git server" | ||||||
|
||||||
CmdInitFlagRegURL = "External registry url address to use for this Zarf cluster" | ||||||
CmdInitFlagRegNodePort = "Nodeport to access a registry internal to the k8s cluster. Between [30000-32767]" | ||||||
CmdInitFlagRegNodePort = "[DEPRECATED] Nodeport to access a registry internal to the k8s cluster. Between [30000-32767]" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
CmdInitFlagRegPushUser = "Username to access to the registry Zarf is configured to use" | ||||||
CmdInitFlagRegPushPass = "Password for the push-user to connect to the registry" | ||||||
CmdInitFlagRegPullUser = "Username for pull-only access to the registry" | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,21 +127,6 @@ func (c *Cluster) InitZarfState(initOptions types.ZarfInitOptions) error { | |
} | ||
} | ||
|
||
switch state.Distro { | ||
case k8s.DistroIsK3s, k8s.DistroIsK3d: | ||
state.StorageClass = "local-path" | ||
|
||
case k8s.DistroIsKind, k8s.DistroIsGKE: | ||
state.StorageClass = "standard" | ||
|
||
case k8s.DistroIsDockerDesktop: | ||
state.StorageClass = "hostpath" | ||
} | ||
|
||
if initOptions.StorageClass != "" { | ||
state.StorageClass = initOptions.StorageClass | ||
} | ||
|
||
spinner.Success() | ||
|
||
// Save the state back to K8s | ||
|
@@ -249,13 +234,6 @@ func (c *Cluster) MergeZarfState(oldState *types.ZarfState, initOptions types.Za | |
|
||
if slices.Contains(services, message.RegistryKey) { | ||
newState.RegistryInfo = helpers.MergeNonZero(newState.RegistryInfo, initOptions.RegistryInfo) | ||
// Set the state of the internal registry if it has changed | ||
if newState.RegistryInfo.Address == fmt.Sprintf("%s:%d", helpers.IPV4Localhost, newState.RegistryInfo.NodePort) { | ||
newState.RegistryInfo.InternalRegistry = true | ||
} else { | ||
newState.RegistryInfo.InternalRegistry = false | ||
} | ||
Comment on lines
-253
to
-257
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing this logic from here will break the library usages of |
||
|
||
// Set the new passwords if they should be autogenerated | ||
if newState.RegistryInfo.PushPassword == oldState.RegistryInfo.PushPassword && oldState.RegistryInfo.InternalRegistry { | ||
newState.RegistryInfo.PushPassword = utils.RandomString(config.ZarfGeneratedPasswordLen) | ||
|
@@ -310,12 +288,6 @@ func (c *Cluster) fillInEmptyContainerRegistryValues(containerRegistry types.Reg | |
containerRegistry.NodePort = config.ZarfInClusterContainerRegistryNodePort | ||
} | ||
|
||
// Set default url if an external registry was not provided | ||
if containerRegistry.Address == "" { | ||
containerRegistry.InternalRegistry = true | ||
containerRegistry.Address = fmt.Sprintf("%s:%d", helpers.IPV4Localhost, containerRegistry.NodePort) | ||
} | ||
|
||
// Generate a push-user password if not provided by init flag | ||
if containerRegistry.PushPassword == "" { | ||
containerRegistry.PushPassword = utils.RandomString(config.ZarfGeneratedPasswordLen) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,12 +85,18 @@ func (values *Values) GetVariables(component types.ZarfComponent) (templateMap m | |
regInfo := values.config.State.RegistryInfo | ||
gitInfo := values.config.State.GitServer | ||
|
||
builtinMap := map[string]string{ | ||
"STORAGE_CLASS": values.config.State.StorageClass, | ||
if values.config.SetVariableMap != nil { | ||
if regInfo.NodePort > 0 && values.config.SetVariableMap["REGISTRY_NODEPORT"] != nil { | ||
values.config.SetVariableMap["REGISTRY_NODEPORT"].Value = fmt.Sprintf("%d", regInfo.NodePort) | ||
} | ||
if values.config.State.StorageClass != "" && values.config.SetVariableMap["REGISTRY_STORAGE_CLASS"] != nil { | ||
values.config.SetVariableMap["REGISTRY_STORAGE_CLASS"].Value = values.config.State.StorageClass | ||
} | ||
} | ||
Comment on lines
+88
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need to be here - variables in a zarf.yaml will just pass through |
||
|
||
builtinMap := map[string]string{ | ||
// Registry info | ||
"REGISTRY": values.registry, | ||
"NODEPORT": fmt.Sprintf("%d", regInfo.NodePort), | ||
"REGISTRY_AUTH_PUSH": regInfo.PushPassword, | ||
"REGISTRY_AUTH_PULL": regInfo.PullPassword, | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -189,7 +189,6 @@ func (p *Packager) deployComponents() (deployedComponents []types.DeployedCompon | |
} | ||
|
||
func (p *Packager) deployInitComponent(component types.ZarfComponent) (charts []types.InstalledChart, err error) { | ||
hasExternalRegistry := p.cfg.InitOpts.RegistryInfo.Address != "" | ||
isSeedRegistry := component.Name == "zarf-seed-registry" | ||
isRegistry := component.Name == "zarf-registry" | ||
isInjector := component.Name == "zarf-injector" | ||
|
@@ -203,7 +202,7 @@ func (p *Packager) deployInitComponent(component types.ZarfComponent) (charts [] | |
} | ||
} | ||
|
||
if hasExternalRegistry && (isSeedRegistry || isInjector || isRegistry) { | ||
if !p.cfg.InitOpts.RegistryInfo.InternalRegistry && (isSeedRegistry || isInjector || isRegistry) { | ||
message.Notef("Not deploying the component (%s) since external registry information was provided during `zarf init`", component.Name) | ||
return charts, nil | ||
} | ||
|
@@ -216,6 +215,18 @@ func (p *Packager) deployInitComponent(component types.ZarfComponent) (charts [] | |
// Before deploying the seed registry, start the injector | ||
if isSeedRegistry { | ||
p.cluster.StartInjectionMadness(p.layout.Base, p.layout.Images.Base, component.Images) | ||
|
||
// Retrieve the default storage class from the cluster, it will be the default | ||
if p.cfg.PkgOpts.SetVariables == nil { | ||
p.cfg.PkgOpts.SetVariables = map[string]string{} | ||
} | ||
if c := p.cfg.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"]; c == "" { | ||
p.cfg.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"], err = p.cluster.GetDefaultStorageClass() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be needed if the storage class is empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested this with |
||
} | ||
|
||
if err != nil { | ||
message.WarnErr(err, "Unable to get the default storage class from the cluster") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have an opportunity to fail fast here. If RKE2 clusters come to mind since they don't provide a default storage class |
||
} | ||
} | ||
|
||
charts, err = p.deployComponent(component, isAgent /* skip img checksum if isAgent */, isSeedRegistry /* skip image push if isSeedRegistry */) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.