From a0e1b3d6f87b1569ecf81e3901d6dcee652172f3 Mon Sep 17 00:00:00 2001 From: Feroze Mohideen Date: Fri, 15 Sep 2023 14:23:48 -0400 Subject: [PATCH] Fix bug where custom helm values for services were being wiped when deployed from the frontend (#3579) --- api/server/handlers/porter_app/create.go | 36 ++++++++--------- api/server/handlers/porter_app/parse.go | 49 ++++++++++++++---------- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/api/server/handlers/porter_app/create.go b/api/server/handlers/porter_app/create.go index 1e6932dbff..923fdf10bb 100644 --- a/api/server/handlers/porter_app/create.go +++ b/api/server/handlers/porter_app/create.go @@ -106,30 +106,29 @@ func (c *CreatePorterAppHandler) ServeHTTP(w http.ResponseWriter, r *http.Reques var releaseValues map[string]interface{} var releaseDependencies []*chart.Dependency - if shouldCreate || request.OverrideRelease { - releaseValues = nil - releaseDependencies = nil - - // this is required because when the front-end sends an update request with overrideRelease=true, it is unable to - // get the image info from the release. unless it is explicitly provided in the request, we avoid overwriting it - // by attempting to get the image info from the release or the provided helm values - if helmRelease != nil && (imageInfo.Repository == "" || imageInfo.Tag == "") { - if request.FullHelmValues != "" { - imageInfo, err = attemptToGetImageInfoFromFullHelmValues(request.FullHelmValues) - if err != nil { - err = telemetry.Error(ctx, span, err, "error getting image info from full helm values") - telemetry.WithAttributes(span, telemetry.AttributeKV{Key: "porter-yaml-base64", Value: porterYamlBase64}) - c.HandleAPIError(w, r, apierrors.NewErrPassThroughToClient(err, http.StatusBadRequest)) - return - } - } else { - imageInfo = attemptToGetImageInfoFromRelease(helmRelease.Config) + // unless it is explicitly provided in the request, we avoid overwriting the image info + // by attempting to get it from the release or the provided helm values + if helmRelease != nil && (imageInfo.Repository == "" || imageInfo.Tag == "") { + if request.FullHelmValues != "" { + imageInfo, err = attemptToGetImageInfoFromFullHelmValues(request.FullHelmValues) + if err != nil { + err = telemetry.Error(ctx, span, err, "error getting image info from full helm values") + telemetry.WithAttributes(span, telemetry.AttributeKV{Key: "porter-yaml-base64", Value: porterYamlBase64}) + c.HandleAPIError(w, r, apierrors.NewErrPassThroughToClient(err, http.StatusBadRequest)) + return } + } else { + imageInfo = attemptToGetImageInfoFromRelease(helmRelease.Config) } + } + if shouldCreate { + releaseValues = nil + releaseDependencies = nil } else { releaseValues = helmRelease.Config releaseDependencies = helmRelease.Chart.Metadata.Dependencies } + telemetry.WithAttributes(span, telemetry.AttributeKV{Key: "image-repo", Value: imageInfo.Repository}, telemetry.AttributeKV{Key: "image-tag", Value: imageInfo.Tag}) if request.Builder == "" { @@ -192,6 +191,7 @@ func (c *CreatePorterAppHandler) ServeHTTP(w http.ResponseWriter, r *http.Reques ShouldValidateHelmValues: shouldCreate, FullHelmValues: request.FullHelmValues, AddCustomNodeSelector: addCustomNodeSelector, + RemoveDeletedServices: request.OverrideRelease, }, ) if err != nil { diff --git a/api/server/handlers/porter_app/parse.go b/api/server/handlers/porter_app/parse.go index 41907dfaa3..01b77eddd4 100644 --- a/api/server/handlers/porter_app/parse.go +++ b/api/server/handlers/porter_app/parse.go @@ -111,6 +111,8 @@ type ParseConf struct { FullHelmValues string // AddCustomNodeSelector is a flag to determine whether to add porter.run/workload-kind: application to the nodeselector attribute of the helm values AddCustomNodeSelector bool + // RemoveDeletedServices is a flag to determine whether to remove values and dependencies for services that are not defined in the porter.yaml + RemoveDeletedServices bool } func parse(ctx context.Context, conf ParseConf) (*chart.Chart, map[string]interface{}, map[string]interface{}, error) { @@ -199,7 +201,7 @@ func parse(ctx context.Context, conf ParseConf) (*chart.Chart, map[string]interf Release: parsed.Release, } - values, err := buildUmbrellaChartValues(ctx, application, synced_env, conf.ImageInfo, conf.ExistingHelmValues, conf.SubdomainCreateOpts, conf.InjectLauncherToStartCommand, conf.ShouldValidateHelmValues, conf.UserUpdate, conf.Namespace, conf.AddCustomNodeSelector) + values, err := buildUmbrellaChartValues(ctx, application, synced_env, conf.ImageInfo, conf.ExistingHelmValues, conf.SubdomainCreateOpts, conf.InjectLauncherToStartCommand, conf.ShouldValidateHelmValues, conf.UserUpdate, conf.Namespace, conf.AddCustomNodeSelector, conf.RemoveDeletedServices) if err != nil { err = telemetry.Error(ctx, span, err, "error building values") return nil, nil, nil, err @@ -210,7 +212,7 @@ func parse(ctx context.Context, conf ParseConf) (*chart.Chart, map[string]interf return nil, nil, nil, err } - umbrellaChart, err := buildUmbrellaChart(application, conf.ServerConfig, conf.ProjectID, conf.ExistingChartDependencies) + umbrellaChart, err := buildUmbrellaChart(application, conf.ServerConfig, conf.ProjectID, conf.ExistingChartDependencies, conf.RemoveDeletedServices) if err != nil { err = telemetry.Error(ctx, span, err, "error building umbrella chart") return nil, nil, nil, err @@ -238,6 +240,7 @@ func buildUmbrellaChartValues( userUpdate bool, namespace string, addCustomNodeSelector bool, + removeDeletedValues bool, ) (map[string]interface{}, error) { values := make(map[string]interface{}) @@ -290,10 +293,12 @@ func buildUmbrellaChartValues( values[helmName] = helm_values } - // add back in the existing services that were not overwritten - for k, v := range existingValues { - if values[k] == nil { - values[k] = v + if !removeDeletedValues { + // add back in the existing services that were not overwritten + for k, v := range existingValues { + if values[k] == nil { + values[k] = v + } } } @@ -505,7 +510,7 @@ func deconstructSyncedEnvs(synced_env []*SyncedEnvSection, env map[string]string return synced } -func buildUmbrellaChart(application *Application, config *config.Config, projectID uint, existingDependencies []*chart.Dependency) (*chart.Chart, error) { +func buildUmbrellaChart(application *Application, config *config.Config, projectID uint, existingDependencies []*chart.Dependency, removeDeletedDependencies bool) (*chart.Chart, error) { deps := make([]*chart.Dependency, 0) for alias, service := range application.Services { var serviceType string @@ -540,22 +545,24 @@ func buildUmbrellaChart(application *Application, config *config.Config, project }) } - // add in the existing dependencies that were not overwritten - for _, dep := range existingDependencies { - if !dependencyExists(deps, dep) { - // have to repair the dependency name because of https://github.com/helm/helm/issues/9214 - if strings.HasSuffix(dep.Name, "-web") || strings.HasSuffix(dep.Name, "-wkr") || strings.HasSuffix(dep.Name, "-job") { - dep.Name = getChartTypeFromHelmName(dep.Name) - if dep.Name == "" { - return nil, fmt.Errorf("unable to determine type of existing dependency") - } - version, err := getLatestTemplateVersion(dep.Name, config, projectID) - if err != nil { - return nil, err + if !removeDeletedDependencies { + // add in the existing dependencies that were not overwritten + for _, dep := range existingDependencies { + if !dependencyExists(deps, dep) { + // have to repair the dependency name because of https://github.com/helm/helm/issues/9214 + if strings.HasSuffix(dep.Name, "-web") || strings.HasSuffix(dep.Name, "-wkr") || strings.HasSuffix(dep.Name, "-job") { + dep.Name = getChartTypeFromHelmName(dep.Name) + if dep.Name == "" { + return nil, fmt.Errorf("unable to determine type of existing dependency") + } + version, err := getLatestTemplateVersion(dep.Name, config, projectID) + if err != nil { + return nil, err + } + dep.Version = version } - dep.Version = version + deps = append(deps, dep) } - deps = append(deps, dep) } }