Skip to content

Commit ec4b294

Browse files
saschagrunertsaschagrunert
committed
Add feature gating and enhanced tests for additional storage configuration
This commit addresses CodeRabbit PR review feedback by: 1. Feature Gate Definition (AdditionalStorageConfig): - Created proper feature gate "AdditionalStorageConfig" in features/features.go - Gate enabled in DevPreviewNoUpgrade and TechPreviewNoUpgrade feature sets - Linked to enhancement PR openshift/enhancements#1934 - Contact: saschagrunert, Component: MachineConfigOperator 2. Feature Gating Applied: - Added +openshift:enable:FeatureGate=AdditionalStorageConfig to all three storage field declarations (AdditionalLayerStores, AdditionalImageStores, AdditionalArtifactStores) - Fields now only appear in CRD schema when AdditionalStorageConfig feature gate is enabled (DevPreview/TechPreview) - Generated feature-gated CRD manifest: AdditionalStorageConfig.yaml 3. Enhanced Test Coverage: - Added path validation tests for additionalImageStores (non-absolute path, empty struct/MinProperties) - Added path validation tests for additionalArtifactStores (non-absolute path, empty struct/MinProperties) - Ensures all three store types have consistent validation coverage - Total test count: 15 tests 4. Path Validation Enhancement: - Updated regex to allow dots in paths: ^/[a-zA-Z0-9/._-]+$ - Documentation updated to reflect dot support - Test paths include dots (e.g., /opt/layer_store-v1.0) Verification: - Fields present in AdditionalStorageConfig.yaml (feature-gated) ✓ - Fields NOT present in AAA_ungated.yaml ✓ - Linter passes with 0 issues ✓ Addresses: #2681 (review) Addresses: #2681 (comment) Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
1 parent a5273ad commit ec4b294

25 files changed

Lines changed: 1979 additions & 127 deletions

File tree

‎features.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
| AWSDedicatedHosts| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
2525
| AWSDualStackInstall| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
2626
| AWSServiceLBNetworkSecurityGroup| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
27+
| AdditionalStorageConfig| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
2728
| AutomatedEtcdBackup| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
2829
| AzureClusterHostedDNSInstall| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
2930
| AzureDedicatedHosts| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |

‎features/features.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,14 @@ var (
401401
enableIn(configv1.Default, configv1.OKD, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
402402
mustRegister()
403403

404+
FeatureGateAdditionalStorageConfig = newFeatureGate("AdditionalStorageConfig").
405+
reportProblemsToJiraComponent("MachineConfigOperator").
406+
contactPerson("saschagrunert").
407+
productScope(ocpSpecific).
408+
enhancementPR("https://github.com/openshift/enhancements/pull/1934").
409+
enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
410+
mustRegister()
411+
404412
FeatureGateUpgradeStatus = newFeatureGate("UpgradeStatus").
405413
reportProblemsToJiraComponent("Cluster Version Operator").
406414
contactPerson("pmuller").

‎machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,26 @@ tests:
120120
- path: /var/lib/store11
121121
expectedError: "additionalImageStores in body should have at most 10 items"
122122

123+
- name: Should fail if additionalImageStores path is not absolute
124+
initial: |
125+
apiVersion: machineconfiguration.openshift.io/v1
126+
kind: ContainerRuntimeConfig
127+
spec:
128+
containerRuntimeConfig:
129+
additionalImageStores:
130+
- path: var/lib/images
131+
expectedError: "path must be absolute and contain only alphanumeric characters, '/', '.', '_', and '-'"
132+
133+
- name: Should fail if additionalImageStores item has no path field
134+
initial: |
135+
apiVersion: machineconfiguration.openshift.io/v1
136+
kind: ContainerRuntimeConfig
137+
spec:
138+
containerRuntimeConfig:
139+
additionalImageStores:
140+
- {}
141+
expectedError: "spec.containerRuntimeConfig.additionalImageStores[0] in body should have at least 1 properties"
142+
123143
# AdditionalArtifactStores - test max items validation (different from layer stores)
124144
- name: Should fail if additionalArtifactStores exceeds maximum of 10 items
125145
initial: |
@@ -141,6 +161,26 @@ tests:
141161
- path: /var/lib/store11
142162
expectedError: "additionalArtifactStores in body should have at most 10 items"
143163

164+
- name: Should fail if additionalArtifactStores path is not absolute
165+
initial: |
166+
apiVersion: machineconfiguration.openshift.io/v1
167+
kind: ContainerRuntimeConfig
168+
spec:
169+
containerRuntimeConfig:
170+
additionalArtifactStores:
171+
- path: var/lib/artifacts
172+
expectedError: "path must be absolute and contain only alphanumeric characters, '/', '.', '_', and '-'"
173+
174+
- name: Should fail if additionalArtifactStores item has no path field
175+
initial: |
176+
apiVersion: machineconfiguration.openshift.io/v1
177+
kind: ContainerRuntimeConfig
178+
spec:
179+
containerRuntimeConfig:
180+
additionalArtifactStores:
181+
- {}
182+
expectedError: "spec.containerRuntimeConfig.additionalArtifactStores[0] in body should have at least 1 properties"
183+
144184
# Combined test - all storage types together with other fields
145185
- name: Should be able to create ContainerRuntimeConfig with all storage types and existing fields
146186
initial: |

‎machineconfiguration/v1/types.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,6 +900,7 @@ type ContainerRuntimeConfiguration struct {
900900
//
901901
// When omitted, no additional layer stores are configured.
902902
//
903+
// +openshift:enable:FeatureGate=AdditionalStorageConfig
903904
// +optional
904905
// +listType=atomic
905906
// +kubebuilder:validation:MinItems=1
@@ -916,6 +917,7 @@ type ContainerRuntimeConfiguration struct {
916917
//
917918
// When omitted, only the default image location is used.
918919
//
920+
// +openshift:enable:FeatureGate=AdditionalStorageConfig
919921
// +optional
920922
// +listType=atomic
921923
// +kubebuilder:validation:MinItems=1
@@ -932,6 +934,7 @@ type ContainerRuntimeConfiguration struct {
932934
//
933935
// When omitted, only the default artifact location (/var/lib/containers/storage/artifacts/) is used.
934936
//
937+
// +openshift:enable:FeatureGate=AdditionalStorageConfig
935938
// +optional
936939
// +listType=atomic
937940
// +kubebuilder:validation:MinItems=1

machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs.crd.yaml renamed to machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ metadata:
66
api.openshift.io/merged-by-featuregates: "true"
77
include.release.openshift.io/ibm-cloud-managed: "true"
88
include.release.openshift.io/self-managed-high-availability: "true"
9+
release.openshift.io/feature-set: CustomNoUpgrade
910
labels:
1011
openshift.io/operator-managed: ""
1112
name: containerruntimeconfigs.machineconfiguration.openshift.io

‎machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-Default.crd.yaml‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
apiVersion: apiextensions.k8s.io/v1
2+
kind: CustomResourceDefinition
3+
metadata:
4+
annotations:
5+
api-approved.openshift.io: https://github.com/openshift/api/pull/1453
6+
api.openshift.io/merged-by-featuregates: "true"
7+
include.release.openshift.io/ibm-cloud-managed: "true"
8+
include.release.openshift.io/self-managed-high-availability: "true"
9+
release.openshift.io/feature-set: Default
10+
labels:
11+
openshift.io/operator-managed: ""
12+
name: containerruntimeconfigs.machineconfiguration.openshift.io
13+
spec:
14+
group: machineconfiguration.openshift.io
15+
names:
16+
kind: ContainerRuntimeConfig
17+
listKind: ContainerRuntimeConfigList
18+
plural: containerruntimeconfigs
19+
shortNames:
20+
- ctrcfg
21+
singular: containerruntimeconfig
22+
scope: Cluster
23+
versions:
24+
- name: v1
25+
schema:
26+
openAPIV3Schema:
27+
description: |-
28+
ContainerRuntimeConfig describes a customized Container Runtime configuration.
29+
30+
Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
31+
properties:
32+
apiVersion:
33+
description: |-
34+
APIVersion defines the versioned schema of this representation of an object.
35+
Servers should convert recognized schemas to the latest internal value, and
36+
may reject unrecognized values.
37+
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
38+
type: string
39+
kind:
40+
description: |-
41+
Kind is a string value representing the REST resource this object represents.
42+
Servers may infer this from the endpoint the client submits requests to.
43+
Cannot be updated.
44+
In CamelCase.
45+
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
46+
type: string
47+
metadata:
48+
type: object
49+
spec:
50+
description: spec contains the desired container runtime configuration.
51+
properties:
52+
containerRuntimeConfig:
53+
description: containerRuntimeConfig defines the tuneables of the container
54+
runtime.
55+
properties:
56+
defaultRuntime:
57+
description: |-
58+
defaultRuntime is the name of the OCI runtime to be used as the default for containers.
59+
Allowed values are `runc` and `crun`.
60+
When set to `runc`, OpenShift will use runc to execute the container
61+
When set to `crun`, OpenShift will use crun to execute the container
62+
When omitted, this means no opinion and the platform is left to choose a reasonable default,
63+
which is subject to change over time. Currently, the default is `crun`.
64+
enum:
65+
- crun
66+
- runc
67+
type: string
68+
logLevel:
69+
description: |-
70+
logLevel specifies the verbosity of the logs based on the level it is set to.
71+
Options are fatal, panic, error, warn, info, and debug.
72+
type: string
73+
logSizeMax:
74+
anyOf:
75+
- type: integer
76+
- type: string
77+
description: |-
78+
logSizeMax specifies the Maximum size allowed for the container log file.
79+
Negative numbers indicate that no size limit is imposed.
80+
If it is positive, it must be >= 8192 to match/exceed conmon's read buffer.
81+
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
82+
x-kubernetes-int-or-string: true
83+
overlaySize:
84+
anyOf:
85+
- type: integer
86+
- type: string
87+
description: |-
88+
overlaySize specifies the maximum size of a container image.
89+
This flag can be used to set quota on the size of container images. (default: 10GB)
90+
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
91+
x-kubernetes-int-or-string: true
92+
pidsLimit:
93+
description: pidsLimit specifies the maximum number of processes
94+
allowed in a container
95+
format: int64
96+
type: integer
97+
type: object
98+
machineConfigPoolSelector:
99+
description: |-
100+
machineConfigPoolSelector selects which pools the ContainerRuntimeConfig shoud apply to.
101+
A nil selector will result in no pools being selected.
102+
properties:
103+
matchExpressions:
104+
description: matchExpressions is a list of label selector requirements.
105+
The requirements are ANDed.
106+
items:
107+
description: |-
108+
A label selector requirement is a selector that contains values, a key, and an operator that
109+
relates the key and values.
110+
properties:
111+
key:
112+
description: key is the label key that the selector applies
113+
to.
114+
type: string
115+
operator:
116+
description: |-
117+
operator represents a key's relationship to a set of values.
118+
Valid operators are In, NotIn, Exists and DoesNotExist.
119+
type: string
120+
values:
121+
description: |-
122+
values is an array of string values. If the operator is In or NotIn,
123+
the values array must be non-empty. If the operator is Exists or DoesNotExist,
124+
the values array must be empty. This array is replaced during a strategic
125+
merge patch.
126+
items:
127+
type: string
128+
type: array
129+
x-kubernetes-list-type: atomic
130+
required:
131+
- key
132+
- operator
133+
type: object
134+
type: array
135+
x-kubernetes-list-type: atomic
136+
matchLabels:
137+
additionalProperties:
138+
type: string
139+
description: |-
140+
matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels
141+
map is equivalent to an element of matchExpressions, whose key field is "key", the
142+
operator is "In", and the values array contains only "value". The requirements are ANDed.
143+
type: object
144+
type: object
145+
x-kubernetes-map-type: atomic
146+
required:
147+
- containerRuntimeConfig
148+
type: object
149+
status:
150+
description: status contains observed information about the container
151+
runtime configuration.
152+
properties:
153+
conditions:
154+
description: conditions represents the latest available observations
155+
of current state.
156+
items:
157+
description: ContainerRuntimeConfigCondition defines the state of
158+
the ContainerRuntimeConfig
159+
properties:
160+
lastTransitionTime:
161+
description: lastTransitionTime is the time of the last update
162+
to the current status object.
163+
format: date-time
164+
nullable: true
165+
type: string
166+
message:
167+
description: |-
168+
message provides additional information about the current condition.
169+
This is only to be consumed by humans.
170+
type: string
171+
reason:
172+
description: reason is the reason for the condition's last transition. Reasons
173+
are PascalCase
174+
type: string
175+
status:
176+
description: status of the condition, one of True, False, Unknown.
177+
type: string
178+
type:
179+
description: type specifies the state of the operator's reconciliation
180+
functionality.
181+
type: string
182+
type: object
183+
type: array
184+
x-kubernetes-list-type: atomic
185+
observedGeneration:
186+
description: observedGeneration represents the generation observed
187+
by the controller.
188+
format: int64
189+
type: integer
190+
type: object
191+
required:
192+
- spec
193+
type: object
194+
served: true
195+
storage: true
196+
subresources:
197+
status: {}

0 commit comments

Comments
 (0)