-
Notifications
You must be signed in to change notification settings - Fork 246
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
RFE: Patch Installer Manifests #2497
Open
2uasimojo
wants to merge
1
commit into
openshift:master
Choose a base branch
from
2uasimojo:HIVE-1793/rfe-manifest-patch
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,203 @@ | ||
# Patching Installer Manifests | ||
|
||
- [Overview](#overview) | ||
- [Groundwork](#groundwork) | ||
- [Existing Manifest Patching](#existing-manifest-patching) | ||
- [`ClusterDeploymentCustomization`](#clusterdeploymentcustomization) | ||
- [API](#api) | ||
- [`ClusterDeploymentCustomization.Spec.InstallerManifestPatches`](#clusterdeploymentcustomizationspecinstallermanifestpatches) | ||
- [`ClusterDeployment.Spec.Provisioning.CustomizationRef`](#clusterdeploymentspecprovisioningcustomizationref) | ||
- [`ClusterDeployment.Spec.ClusterPoolRef.CustomizationRef`](#clusterdeploymentspecclusterpoolrefcustomizationref) | ||
- [`ClusterPool.Spec.CustomizationRef`](#clusterpoolspeccustomizationref) | ||
- [`ClusterDeployment.Status`](#clusterdeploymentstatus) | ||
- [`ClusterPool.Status`](#clusterpoolstatus) | ||
- [What About Ignition Configs?](#what-about-ignition-configs) | ||
- [Failure Modes](#failure-modes) | ||
|
||
## Overview | ||
Installer accepts spoke configuration via the | ||
[install-config.yaml](https://github.com/openshift/hive/blob/c392ca38fb489267cd7bfbdb3c5a76cc36163686/vendor/github.com/openshift/installer/pkg/types/installconfig.go#L93) | ||
file. | ||
However, some options are only available by editing the OpenShift object manifests generated by | ||
`openshift-install create manifests` prior to feeding them to `openshift-install create cluster`. | ||
Moving forward, installer frequently prefers new features to be enabled via this latter mechanism. | ||
|
||
Up to this point, hive has incorporated some piecemeal patching of these manifests at the behest of | ||
specific values passed through via hive APIs. | ||
Example: Extra worker security groups for AWS. | ||
However, this model is awkward, brittle, and not extensible. | ||
What is needed is a mechanism for the hive *user* to specify arbitrary modifications to OpenShift | ||
manifests generated by the installer. | ||
|
||
## Groundwork | ||
|
||
### Existing Manifest Patching | ||
The code that invokes `openshift-install` lives in installmanager.go. | ||
[This block](https://github.com/openshift/hive/blob/9615c77cd786dd82079a9dacd60b64c882f56327/pkg/installmanager/installmanager.go#L882-L921) | ||
contains logic to: | ||
- Walk the `openshift/` directory (one of two created by `openshift-install create manifests`, the | ||
other being `manifests/`). | ||
- Load yaml files. | ||
- Parse the files as JSON. | ||
- Patch the JSON. | ||
- Convert the JSON back to YAML. | ||
- Write the files back to storage. | ||
|
||
### `ClusterDeploymentCustomization` | ||
An earlier feature, [ClusterPool Inventory](clusterpool-inventory.md), invented the concept of | ||
[`ClusterDeploymentCustomization`](https://github.com/openshift/hive/blob/8c79fedae7011e434e8a7ff0fef40994ef92deb2/vendor/github.com/openshift/hive/apis/hive/v1/clusterdeploymentcustomization_types.go), | ||
a hive API used by ClusterPools when specific discrete values (such as reserved IP addresses) must | ||
be injected into the install-config.yaml for each cluster in the pool. | ||
As such, ClusterDeploymentCustomization originally contained only one member, | ||
[`InstallConfigPatches`](https://github.com/openshift/hive/blob/4eaf5fd7858e8def3b2ba3229801090610246243/vendor/github.com/openshift/hive/apis/hive/v1/clusterdeploymentcustomization_types.go#L45), | ||
a list of patches to apply to the install-config.yaml generated by the ClusterPool controller based | ||
on the user-provided template. | ||
Each entry in `InstallConfigPatches` is a | ||
[`PatchEntity`](https://github.com/openshift/hive/blob/4eaf5fd7858e8def3b2ba3229801090610246243/vendor/github.com/openshift/hive/apis/hive/v1/clusterdeploymentcustomization_types.go#L49), | ||
a hive-owned CRD corresponding to an | ||
[RFC 6902 JSON patch](https://datatracker.ietf.org/doc/html/rfc6902). | ||
|
||
## API | ||
|
||
### `ClusterDeploymentCustomization.Spec.InstallerManifestPatches` | ||
|
||
**Example:** | ||
```yaml | ||
apiVersion: v1 | ||
kind: ClusterDeploymentCustomization | ||
metadata: | ||
name: foo-cluster-deployment-customization | ||
namespace: my-project | ||
spec: | ||
installerManifestPatches: | ||
# Add custom labels to all master machine manifests | ||
- manifestSelector: | ||
glob: openshift/99_openshift-cluster-api_master-machines-*.yaml | ||
patches: | ||
- op: add | ||
path: metadata/labels/a-custom-label | ||
value: foo | ||
- op: add | ||
path: metadata/labels/b-custom-label | ||
value: bar | ||
- ... | ||
... | ||
``` | ||
|
||
- Build on the existing [groundwork](#groundwork), extending `ClusterDeploymentCustomization` to | ||
expose a new field, `InstallerManifestPatches`. | ||
- Because installer generates multiple manifests, and RFC 6902 syntax can only reference a single | ||
document at a time, we can't directly use `PatchEntity`; we must use an intervening type to | ||
identify which file(s) a patch is to be applied to. | ||
We'll call this `InstallerManifestPatch`. | ||
- Each `InstallerManifestPatch`: | ||
- Identifies one or more files via subfield `ManifestSelector`. | ||
- Lists `PatchEntity`s to apply to files thus identified. | ||
This is the same `PatchEntity` used for `InstallConfigPatches`. | ||
- `ManifestSelector` supports one subfield, `Glob`, which accepts a path matching string as | ||
supported by golang's [filepath.Glob](https://pkg.go.dev/path/filepath#Glob). | ||
- We will execute the glob relative to the working directory in which manifests were generated. | ||
- For security, we will attempt to detect and raise an error if any paths attempt to point outside of the working directory. | ||
- `ManifestSelector` is extensible. | ||
E.g. in the future we may wish to match manifests based on the GVK of the object therein. | ||
|
||
### `ClusterDeployment.Spec.Provisioning.CustomizationRef` | ||
|
||
**Example:** | ||
```yaml | ||
apiVersion: v1 | ||
kind: ClusterDeployment | ||
metadata: | ||
name: foo-cluster-deployment | ||
namespace: my-project | ||
spec: | ||
provisioning: | ||
installConfigSecretRef: ic-secret | ||
customizationRef: | ||
name: foo-cluster-deployment-customization | ||
... | ||
... | ||
``` | ||
|
||
- Extend `ClusterDeployment`, adding a `CustomizationRef` field at the same level as | ||
`InstallConfigSecretRef`, i.e. under `Spec.Provisioning`. | ||
- `CustomizationRef` is a `LocalObjectReference` to the name of a `ClusterDeploymentCustomization` | ||
in the same namespace as the `ClusterDeployment`. | ||
|
||
### `ClusterDeployment.Spec.ClusterPoolRef.CustomizationRef` | ||
|
||
This field already existed. | ||
It was used by the ClusterPool Inventory feature to help track which CDC from the pool's inventory | ||
was assigned to this CD. | ||
|
||
**We will add support for inventory CDCs to contain `InstallerManifestPatch`.** | ||
|
||
That is, in addition to supporting per-pool-CD patching of the install-config, we will now also | ||
support per-pool-CD patching of generated manifests. | ||
|
||
**NOTE:** This support will require copying the referenced CDC into the pool CD's (generated) | ||
namespace so it can be applied by the provisioner pod. | ||
(This was not previously necessary, as the install-config patches were used to customize the | ||
install-config Secret in memory before it was created in the target namespace.) | ||
|
||
### `ClusterPool.Spec.CustomizationRef` | ||
|
||
**Example:** | ||
```yaml | ||
apiVersion: v1 | ||
kind: ClusterPool | ||
metadata: | ||
name: foo-cluster-pool | ||
namespace: my-project | ||
spec: | ||
customizationRef: | ||
name: foo-cluster-deployment-customization | ||
... | ||
... | ||
``` | ||
|
||
- Extend `ClusterPool`, adding a `CustomizationRef` field at the top `Spec` level. | ||
- `CustomizationRef` is a `LocalObjectReference` to the name of a `ClusterDeploymentCustomization` | ||
in the same namespace as the `ClusterPool`. | ||
- In contrast to the inventory CDC, this can (should) be used when the same manifest patches are to | ||
be applied to *all* CDs in the pool. | ||
- This can be used with or without Inventory. | ||
- As with `ClusterDeployment.Spec.ClusterPoolRef.CustomizationRef`, the CDC referenced by this field will be copied into | ||
the pool CD's namespace so it can be applied by the provisioner. | ||
- If both references exist, we will apply `Spec.CustomizationRef` first so that the CD-specific | ||
patches in `ClusterDeployment.Spec.ClusterPoolRef.CustomizationRef` "win" in case of conflicts. | ||
- Note that, unlike inventory CDCs, these pool-wide CDCs will not be reserved/assigned to a single | ||
CD, or even a single ClusterPool (though they will be limited to ClusterPools in the same | ||
namespace). | ||
|
||
### `ClusterDeployment.Status` | ||
|
||
When a referenced CDC does not exist, we will update the `RequirementsMet` condition accordingly. | ||
|
||
For eventual consistency, if the CDC is subsequently created, we will "immediately" clear the | ||
condition (i.e. we need a `Watch()` on CDC that enqueues any ClusterDeployments that reference it). | ||
|
||
### `ClusterPool.Status` | ||
|
||
When the CDC referenced by `ClusterPool.Spec.CustomizationRef` does not exist, we'll set the | ||
`MissingDependencies` condition. | ||
|
||
For eventual consistency, if the CDC is subsequently created, we will "immediately" clear the | ||
condition (i.e. we need a `Watch()` on CDC that enqueues any ClusterPools that reference it). | ||
|
||
## What About Ignition Configs? | ||
Between `create manifests` and `create cluster` we `create ignition-configs`. | ||
- This feature will *not* support patching ignition configs. | ||
If we decide to add that support at some point in the future, it will be via a new CDC subfield, e.g. `IgnitionConfigPatches`. | ||
- Because `create ignition-configs` consumes and deletes generated manifests, we must apply manifest patches _between_ these two phases. | ||
|
||
## Failure Modes | ||
Other than the status conditions mentioned above, all new failure modes will occur within the | ||
provision pod, causing that pod to log an error message and fail. | ||
|
||
Error conditions: | ||
- A `ManifestSelector` matches zero manifests. | ||
- A `ManifestSelector.Glob` resolves to a path outside of the installer's working directory. | ||
- A patch has invalid syntax. | ||
- Applying a patch fails. | ||
- The usual things like files unexpectedly not being readable/writable/parseable (not expected). |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Self-
We should fix up the clusterpool controller to automatically add this ref from the inventory to the CD. That way the clusterpool user could do inventory things in either/both ways (install-config and/or manifest patching). Note: requires it not to be a failure if a CDC reffed here contains no manifest patches? Or maybe we can be strict for non-pool clusters? Is it a problem if a non-pool CD uses installConfigPatches??
-Self