[cloud_hotfix_releases] MGMT-12435: Add a way to apply patches to core manifests (#4636)

* MGMT-12435: Add a way to apply patches to core manifests

Starting in 4.12 it's not possible to completely overwrite existing
manifests, the way AI was doing it. Core manifests that need to be
modified should be patched instead.

This PR adds a logic to upload patches to the manifests api and then
apply them.

Note that the InfrastructureCR patching was not changed to us this new
flow as this PR focuses only in adding this possibility and changing the
SchedulableMasters flow. Future PRs will align the rest of the codebase.

Signed-off-by: Flavio Percoco <[email protected]>

* Defer delete of manifests dir in case of error

Signed-off-by: Flavio Percoco <[email protected]>

* Address review comments

Signed-off-by: Flavio Percoco <[email protected]>

* Revert "OCPBUGS-1482: Don't override schedulableMasters unnecessarily (#4414)"

This reverts commit 5661d9c.

Let's go back to applying the schedulable masters patch when either
SchedulableMastersForce or SchedulableMasters are set.

Signed-off-by: Flavio Percoco <[email protected]>
Co-authored-by: Flavio Percoco <[email protected]>

Author: openshift-cherrypick-robot

internal/common/common.go

@@ -120,14 +120,7 @@ func IsImportedCluster(cluster *Cluster) bool {
 }
 
 func AreMastersSchedulable(cluster *Cluster) bool {
-	// If the topology forces schedulable masters (i.e. there are no workers),
-	// SchedulableMastersForcedTrue will be true, but also it will be enabled
-	// by default in the installer. Since overriding it currently causes a
-	// failure due to a conflict starting with 4.12, we prefer to avoid it. We
-	// only need to override the installer when the user has set
-	// SchedulableMasters explicitly and that is not already the default in the
-	// installer.
-	return !swag.BoolValue(cluster.SchedulableMastersForcedTrue) && swag.BoolValue(cluster.SchedulableMasters)
+	return swag.BoolValue(cluster.SchedulableMastersForcedTrue) || swag.BoolValue(cluster.SchedulableMasters)
 }
 
 func GetEffectiveRole(host *models.Host) models.HostRole {

internal/common/common_test.go

@@ -253,8 +253,8 @@ var _ = Describe("Test AreMastersSchedulable", func() {
 		}{
 			{schedulableMastersForcedTrue: false, schedulableMasters: false, expectedSchedulableMasters: false},
 			{schedulableMastersForcedTrue: false, schedulableMasters: true, expectedSchedulableMasters: true},
-			{schedulableMastersForcedTrue: true, schedulableMasters: false, expectedSchedulableMasters: false}, // Handled by installer
-			{schedulableMastersForcedTrue: true, schedulableMasters: true, expectedSchedulableMasters: false},  // Handled by installer
+			{schedulableMastersForcedTrue: true, schedulableMasters: false, expectedSchedulableMasters: true},
+			{schedulableMastersForcedTrue: true, schedulableMasters: true, expectedSchedulableMasters: true},
 		} {
 			test := test
 			It(fmt.Sprintf("schedulableMastersForcedTrue=%v schedulableMasters=%v AreMastersSchedulable? %v", test.schedulableMastersForcedTrue, test.schedulableMasters, test.expectedSchedulableMasters), func() {

internal/ignition/ignition.go

@@ -298,15 +298,23 @@ func (g *installerGenerator) UploadToS3(ctx context.Context) error {
 // Generate generates ignition files and applies modifications.
 func (g *installerGenerator) Generate(ctx context.Context, installConfig []byte, platformType models.PlatformType) error {
 	var icspFile string
+	var err error
 	log := logutil.FromContext(ctx, g.log)
 
+	defer func() {
+		if err != nil {
+			os.Remove(filepath.Join(g.workDir, "manifests"))
+			os.Remove(filepath.Join(g.workDir, "openshift"))
+		}
+	}()
+
 	// In case we don't want to override image for extracting installer use release one
 	if g.installerReleaseImageOverride == "" {
 		g.installerReleaseImageOverride = g.releaseImage
 	}
 
 	// If ImageContentSources are defined, store in a file for the 'oc' command
-	icspFile, err := getIcspFileFromInstallConfig(installConfig, log)
+	icspFile, err = getIcspFileFromInstallConfig(installConfig, log)
 	if err != nil {
 		return errors.Wrap(err, "failed to create file with ImageContentSources")
 	}
@@ -384,13 +392,17 @@ func (g *installerGenerator) Generate(ctx context.Context, installConfig []byte,
 		log.Infof("adding manifest %s to working dir for cluster %s", manifest, g.cluster.ID)
 		err = g.downloadManifest(ctx, manifest)
 		if err != nil {
-			_ = os.Remove(filepath.Join(g.workDir, "manifests"))
-			_ = os.Remove(filepath.Join(g.workDir, "openshift"))
 			log.WithError(err).Errorf("Failed to download manifest %s to working dir for cluster %s", manifest, g.cluster.ID)
 			return err
 		}
 	}
 
+	err = g.applyManifestPatches(ctx)
+	if err != nil {
+		log.WithError(err).Errorf("failed to apply manifests' patches for cluster '%s'", g.cluster.ID)
+		return err
+	}
+
 	err = g.applyInfrastructureCRPatch(ctx)
 	if err != nil {
 		log.WithError(err).Errorf("failed to patch the infrastructure CR manifest '%s'", common.PlatformTypeValue(g.cluster.Platform.Type))
@@ -468,6 +480,77 @@ func (g *installerGenerator) addBootstrapKubeletIpIfRequired(log logrus.FieldLog
 	return envVars, nil
 }
 
+func (g *installerGenerator) applyManifestPatches(ctx context.Context) error {
+	log := logutil.FromContext(ctx, g.log)
+	manifestsOpenShiftDir := filepath.Join(g.workDir, "openshift")
+
+	// File path walks the directory in lexical order, which means it's possible to have some control on
+	// how files are being walked through by using a numeric prefix for the patch extension. For example:
+	// - cluster-scheduler-02-config.yml.patch_01_set_schedulable_masters
+	// - cluster-scheduler-02-config.yml.patch_02_something_else
+	err := filepath.Walk(manifestsOpenShiftDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+
+		// We allow files that have the following extension .y(a)ml.patch(_something).
+		// This allows to pushing multuple patches for the same Manifest.
+		extension := filepath.Ext(info.Name())
+		if !strings.HasPrefix(extension, ".patch") {
+			return nil
+		}
+
+		// This is the path to the patch
+		manifestPatchPath := filepath.Join(manifestsOpenShiftDir, info.Name())
+		log.Debugf("Applying the following patch: %s", manifestPatchPath)
+		manifestPatch, err := os.ReadFile(manifestPatchPath)
+		if err != nil {
+			return errors.Wrapf(err, "failed to read manifest patch %s", manifestPatchPath)
+		}
+		log.Debugf("read the manifest at %s", manifestPatchPath)
+
+		// Let's look for the actual manifest. Code first looks in the `openshift` directory and
+		// fallsback to the `manifests` directory if no patch was found in the former.
+		manifestPath := filepath.Join(manifestsOpenShiftDir, strings.TrimSuffix(info.Name(), extension))
+		if _, err = os.Stat(manifestPath); errors.Is(err, os.ErrNotExist) {
+			log.Debugf("Manifest %s does not exist. Trying with the openshift dir next")
+			manifestPath = filepath.Join(g.workDir, "manifests", strings.TrimSuffix(info.Name(), extension))
+		}
+
+		data, err := os.ReadFile(manifestPath)
+		if err != nil {
+			return errors.Wrapf(err, "failed to read manifest %s", manifestPath)
+		}
+		log.Debugf("read the manifest at %s", manifestPath)
+
+		// Let's apply the patch now since both files have been read
+		data, err = common.ApplyYamlPatch(data, manifestPatch)
+		if err != nil {
+			return errors.Wrapf(err, "failed to patch manifest \"%s\"", manifestPath)
+		}
+		log.Debugf("applied the yaml patch to the manifest at %s: \n %s", manifestPath, string(data[:]))
+
+		err = os.WriteFile(manifestPath, data, 0600)
+		if err != nil {
+			return errors.Wrapf(err, "failed to write manifest \"%s\"", manifestPath)
+		}
+
+		log.Debugf("wrote the resulting manifest at %s", manifestPath)
+
+		err = os.Remove(manifestPatchPath)
+		if err != nil {
+			return errors.Wrapf(err, "failed to remove patch %s", manifestPatchPath)
+		}
+		return nil
+	})
+
+	if err != nil {
+		return errors.Wrapf(err, "failed to apply patches")
+	}
+
+	return nil
+}
+
 func (g *installerGenerator) applyInfrastructureCRPatch(ctx context.Context) error {
 	log := logutil.FromContext(ctx, g.log)
 

internal/ignition/ignition_test.go

@@ -1698,7 +1698,7 @@ status:
 		// the infra name should not have changed
 		Expect(status["infrastructureName"].(string)).To(Equal("test-cluster-s9qbn"))
 	})
-	It("patches the manifest correctly", func() {
+	It("patches the infrastructure manifest correctly", func() {
 		base := `---
 apiVersion: config.openshift.io/v1
 kind: Infrastructure
@@ -1746,6 +1746,71 @@ status:
 		// the infra name should not have changed
 		Expect(status["infrastructureName"].(string)).To(Equal("test-cluster-s9qbn"))
 	})
+
+	It("patches core manifests correctly", func() {
+		base := `---
+apiVersion: config.openshift.io/v1
+kind: Scheduler
+metadata:
+  creationTimestamp: "2022-11-03T06:20:17Z"
+  generation: 1
+  name: cluster
+  resourceVersion: "620"
+  uid: b74da926-8664-41a2-8fbf-4217156a63c6
+spec:
+  mastersSchedulable: false
+  policy:
+    name: ""`
+
+		schedulableMastersManifestPatch := `---
+- op: replace
+  path: /spec/mastersSchedulable
+  value: true
+`
+		schedulerPatchCustomTest := `---
+- op: add
+  path: /spec/customTest
+  value: true
+`
+
+		manifestsOpenshiftDir := filepath.Join(workDir, "/openshift")
+		Expect(os.Mkdir(manifestsOpenshiftDir, 0755)).To(Succeed())
+
+		manifestPatchPath := filepath.Join(manifestsOpenshiftDir, "cluster-scheduler-02-config.yml.patch")
+		err := os.WriteFile(manifestPatchPath, []byte(schedulableMastersManifestPatch), 0600)
+		Expect(err).NotTo(HaveOccurred())
+
+		manifestPatchCustomTestPath := filepath.Join(manifestsOpenshiftDir, "cluster-scheduler-02-config.yml.patch_custom_test")
+		err = os.WriteFile(manifestPatchCustomTestPath, []byte(schedulerPatchCustomTest), 0600)
+		Expect(err).NotTo(HaveOccurred())
+
+		manifestsDir := filepath.Join(workDir, "/manifests")
+		Expect(os.Mkdir(manifestsDir, 0755)).To(Succeed())
+
+		err = os.WriteFile(filepath.Join(manifestsDir, "cluster-scheduler-02-config.yml"), []byte(base), 0600)
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(generator.applyManifestPatches(ctx)).To(Succeed())
+
+		content, err := os.ReadFile(filepath.Join(manifestsDir, "cluster-scheduler-02-config.yml"))
+		Expect(err).NotTo(HaveOccurred())
+
+		merged := map[string]interface{}{}
+		err = yaml.Unmarshal(content, &merged)
+		Expect(err).NotTo(HaveOccurred())
+
+		status := merged["spec"].(map[interface{}]interface{})
+
+		// Master should now be schedulable
+		Expect(status["mastersSchedulable"].(bool)).To(Equal(true))
+		Expect(status["customTest"].(bool)).To(Equal(true))
+
+		_, err = os.Stat(manifestPatchPath)
+		Expect(errors.Is(err, os.ErrNotExist)).To(Equal(true))
+
+		_, err = os.Stat(manifestPatchCustomTestPath)
+		Expect(errors.Is(err, os.ErrNotExist)).To(Equal(true))
+	})
 })
 
 var _ = Describe("Set kubelet node ip", func() {

internal/manifests/manifests.go

@@ -91,6 +91,11 @@ func (m *Manifests) CreateClusterManifestInternal(ctx context.Context, params op
 		if !json.Valid(manifestContent) {
 			return nil, common.NewApiError(http.StatusBadRequest, errors.New("Manifest content has an illegal JSON format"))
 		}
+	} else if strings.HasPrefix(extension, ".patch") && (strings.Contains(fileName, ".yaml.patch") || strings.Contains(fileName, ".yml.patch")) {
+		var s []map[interface{}]interface{}
+		if yaml.Unmarshal(manifestContent, &s) != nil {
+			return nil, common.NewApiError(http.StatusBadRequest, errors.New("Patch content has an invalid YAML format"))
+		}
 	} else {
 		return nil, common.NewApiError(http.StatusBadRequest, errors.New("Unsupported manifest extension. Only json, yaml and yml extensions are supported"))
 	}

internal/network/manifests_generator.go

@@ -147,16 +147,10 @@ spec:
             WantedBy=multi-user.target
 `
 
-const schedulableMastersManifest = `
-apiVersion: config.openshift.io/v1
-kind: Scheduler
-metadata:
-  name: cluster
-spec:
-  mastersSchedulable: true
-  policy:
-    name: ""
-status: {}
+const schedulableMastersManifestPatch = `---
+- op: replace
+  path: /spec/mastersSchedulable
+  value: true
 `
 
 func createChronyManifestContent(c *common.Cluster, role models.HostRole, log logrus.FieldLogger) ([]byte, error) {
@@ -213,8 +207,8 @@ func (m *ManifestsGenerator) AddChronyManifest(ctx context.Context, log logrus.F
 }
 
 func (m *ManifestsGenerator) AddSchedulableMastersManifest(ctx context.Context, log logrus.FieldLogger, cluster *common.Cluster) error {
-	content := []byte(schedulableMastersManifest)
-	schedulableMastersManifestFile := "50-schedulable_masters.yaml"
+	content := []byte(schedulableMastersManifestPatch)
+	schedulableMastersManifestFile := "cluster-scheduler-02-config.yml.patch_ai_set_masters_schedulable"
 	err := m.createManifests(ctx, cluster, schedulableMastersManifestFile, content)
 	if err != nil {
 		return err

internal/network/manifests_generator_test.go

@@ -474,7 +474,7 @@ var _ = Describe("schedulable masters manifest", func() {
 	})
 
 	Context("CreateClusterManifest success", func() {
-		fileName := "50-schedulable_masters.yaml"
+		fileName := "cluster-scheduler-02-config.yml.patch_ai_set_masters_schedulable"
 		It("CreateClusterManifest success", func() {
 			manifestsApi.EXPECT().CreateClusterManifestInternal(gomock.Any(), gomock.Any()).Return(&models.Manifest{
 				FileName: fileName,