Skip to content

Commit ea702a6

Browse files
committed
fix: return an error when APIServer CR is not found to avoid falling back to defaults
Falling back to the library-go's ObserveTLSSecurityProfile defaults can cause a temporary flip to different TLS configuration that does not adhere the centralized TLS configuration. To avoid any unexpected changes error right away and wait until the CR is available again. Meantime, do not update the corresponding CM.
1 parent 83faa35 commit ea702a6

3 files changed

Lines changed: 47 additions & 39 deletions

File tree

lib/resourcebuilder/core.go

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818

1919
configv1 "github.com/openshift/api/config/v1"
2020
operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1"
21-
configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
2221
configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
2322
"github.com/openshift/library-go/pkg/operator/configobserver/apiserver"
2423
"github.com/openshift/library-go/pkg/operator/events"
@@ -114,13 +113,15 @@ func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) err
114113
// observeTLSConfiguration retrieves TLS configuration from the APIServer cluster CR
115114
// using ObserveTLSSecurityProfile and extracts minTLSVersion and cipherSuites.
116115
func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.ConfigMap) (*tlsConfig, error) {
117-
// Create a lister adapter for ObserveTLSSecurityProfile
118-
lister := &apiServerListerAdapter{
119-
client: b.configClientv1.APIServers(),
120-
ctx: ctx,
116+
apiServer, err := b.configClientv1.APIServers().Get(ctx, "cluster", metav1.GetOptions{})
117+
if err != nil {
118+
return nil, fmt.Errorf("failed to retrieve APIServer CR for ConfigMap %s/%s: %w", cm.Namespace, cm.Name, err)
121119
}
120+
122121
listers := &configObserverListers{
123-
apiServerLister: lister,
122+
apiServerLister: &apiServerListerAdapter{
123+
apiServer: apiServer,
124+
},
124125
}
125126

126127
// Create an in-memory event recorder that doesn't send events to the API server
@@ -187,10 +188,9 @@ func updateRNodeWithTLSSettings(rnode *yaml.RNode, tlsConf *tlsConfig) error {
187188
return nil
188189
}
189190

190-
// apiServerListerAdapter adapts a client interface to the lister interface
191+
// apiServerListerAdapter adapts an injected APIServer to the lister interface
191192
type apiServerListerAdapter struct {
192-
client configclientv1.APIServerInterface
193-
ctx context.Context
193+
apiServer *configv1.APIServer
194194
}
195195

196196
func (a *apiServerListerAdapter) List(selector labels.Selector) ([]*configv1.APIServer, error) {
@@ -199,7 +199,10 @@ func (a *apiServerListerAdapter) List(selector labels.Selector) ([]*configv1.API
199199
}
200200

201201
func (a *apiServerListerAdapter) Get(name string) (*configv1.APIServer, error) {
202-
return a.client.Get(a.ctx, name, metav1.GetOptions{})
202+
if name != a.apiServer.Name {
203+
return nil, fmt.Errorf("APIServer %q not found", name)
204+
}
205+
return a.apiServer, nil
203206
}
204207

205208
// configObserverListers implements the configobserver.Listers interface.

lib/resourcebuilder/core_test.go

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
configv1 "github.com/openshift/api/config/v1"
2323
operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1"
2424
fakeconfigclientv1 "github.com/openshift/client-go/config/clientset/versioned/fake"
25-
"github.com/openshift/library-go/pkg/crypto"
2625
)
2726

2827
// makeGenericConfigYAML generates a generic config YAML string with the specified servingInfo fields.
@@ -77,12 +76,6 @@ func makeGenericControllerConfigYAML(cipherSuites []string, minTLSVersion string
7776
return makeGenericConfigYAML(configv1.GroupVersion.String(), "GenericControllerConfig", cipherSuites, minTLSVersion)
7877
}
7978

80-
func getDefaultCipherSuitesSorted() []string {
81-
cipherSuites := crypto.CipherSuitesToNamesOrDie(crypto.DefaultCiphers())
82-
sort.Strings(cipherSuites)
83-
return cipherSuites
84-
}
85-
8679
const (
8780
// TLS version constants used in test configurations
8881
tlsVersion12 = "VersionTLS12"
@@ -126,8 +119,6 @@ var (
126119
"AES256-GCM-SHA384",
127120
"AES128-SHA256",
128121
}
129-
130-
defaultCipherSuites = getDefaultCipherSuitesSorted()
131122
)
132123

133124
// makeConfigMap is a helper to create a ConfigMap with optional annotation and data
@@ -374,19 +365,14 @@ func TestModifyConfigMap(t *testing.T) {
374365
},
375366
},
376367
{
377-
name: "ConfigMap with annotation and Data - APIServer not found - default TLS configuration expected",
368+
name: "ConfigMap with annotation and Data - APIServer not found - error expected",
378369
configMap: makeConfigMap(true, map[string]string{
379370
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion12),
380371
}),
381372
apiServer: nil,
382-
expectError: false,
373+
expectError: true,
383374
validateConfigMap: func(t *testing.T, original, modified *corev1.ConfigMap) {
384-
defaultTLSConfigMap := makeConfigMap(true, map[string]string{
385-
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(defaultCipherSuites, tlsVersion12),
386-
})
387-
if err := validateConfigMapsEqual(defaultTLSConfigMap, modified); err != nil {
388-
t.Fatalf("validation failed: %v", err)
389-
}
375+
// No validation needed when error is expected
390376
},
391377
},
392378
{

lib/resourcebuilder/resourcebuilder_test.go

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,35 +29,44 @@ metadata:
2929
func TestModifyConfigMapWithBuilder(t *testing.T) {
3030

3131
tests := []struct {
32-
name string
33-
configMap *corev1.ConfigMap
34-
apiServer *configv1.APIServer
35-
expectYAML string // Expected YAML content after modification
32+
name string
33+
configMap *corev1.ConfigMap
34+
apiServer *configv1.APIServer
35+
expectYAML string // Expected YAML content after modification
36+
expectError bool // Whether an error is expected from Do()
3637
}{
3738
{
3839
name: "ConfigMap with GenericOperatorConfig - TLS injection",
3940
configMap: makeConfigMap(true, map[string]string{
40-
"config.yaml": makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion12),
41+
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion12),
4142
}),
4243
apiServer: makeAPIServerConfig(withCustomTLSProfile(testOpenSSLCipherSuites2, configv1.VersionTLS13)),
4344
expectYAML: makeGenericOperatorConfigYAML(testCipherSuites2, tlsVersion13),
4445
},
4546
{
4647
name: "ConfigMap without annotation - no modification",
4748
configMap: makeConfigMap(false, map[string]string{
48-
"config.yaml": makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion12),
49+
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion12),
4950
}),
5051
apiServer: makeAPIServerConfig(withCustomTLSProfile(testOpenSSLCipherSuites2, configv1.VersionTLS13)),
5152
expectYAML: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion12),
5253
},
5354
{
5455
name: "ConfigMap with non-GenericOperatorConfig - no modification",
5556
configMap: makeConfigMap(true, map[string]string{
56-
"config.yaml": noGenericOperatorConfigYAML,
57+
genericOperatorConfigCMKey: noGenericOperatorConfigYAML,
5758
}),
5859
apiServer: makeAPIServerConfig(withCustomTLSProfile(testOpenSSLCipherSuites, configv1.VersionTLS13)),
5960
expectYAML: noGenericOperatorConfigYAML,
6061
},
62+
{
63+
name: "ConfigMap with annotation - APIServer not found - error expected",
64+
configMap: makeConfigMap(true, map[string]string{
65+
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion13),
66+
}),
67+
expectYAML: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion13),
68+
expectError: true,
69+
},
6170
}
6271

6372
for _, tt := range tests {
@@ -73,6 +82,13 @@ func TestModifyConfigMapWithBuilder(t *testing.T) {
7382
// Create fake kubernetes client
7483
fakeKubeClient := fakekubernetes.NewClientset()
7584

85+
// Create the ConfigMap in the fake client before running the builder
86+
ctx := context.Background()
87+
_, err := fakeKubeClient.CoreV1().ConfigMaps(tt.configMap.Namespace).Create(ctx, tt.configMap, metav1.CreateOptions{})
88+
if err != nil {
89+
t.Fatalf("Failed to create ConfigMap: %v", err)
90+
}
91+
7692
// Marshal ConfigMap to YAML bytes
7793
configMapYAML, err := yaml.Marshal(tt.configMap)
7894
if err != nil {
@@ -87,20 +103,23 @@ func TestModifyConfigMapWithBuilder(t *testing.T) {
87103
}
88104

89105
// Call Do()
90-
ctx := context.Background()
91106
err = b.Do(ctx)
92-
if err != nil {
93-
t.Fatalf("Do() unexpected error: %v", err)
107+
108+
// Check error expectation
109+
if (err != nil) != tt.expectError {
110+
t.Fatalf("Do() error = %v, expectError %v", err, tt.expectError)
94111
}
95112

96-
// Retrieve the applied ConfigMap from the fake client
113+
// ConfigMap should always be retrievable, regardless of error
97114
appliedConfigMap, err := fakeKubeClient.CoreV1().ConfigMaps(tt.configMap.Namespace).Get(ctx, tt.configMap.Name, metav1.GetOptions{})
98115
if err != nil {
99116
t.Fatalf("Failed to get applied ConfigMap: %v", err)
100117
}
101118

102119
// Verify the result matches expected YAML
103-
modifiedYAML := appliedConfigMap.Data["config.yaml"]
120+
// When Do() errors, the ConfigMap should remain unchanged (expectYAML has the original values)
121+
// When Do() succeeds, the ConfigMap should have modified values (expectYAML has the modified values)
122+
modifiedYAML := appliedConfigMap.Data[genericOperatorConfigCMKey]
104123
if diff := cmp.Diff(tt.expectYAML, modifiedYAML); diff != "" {
105124
t.Errorf("ConfigMap YAML mismatch (-want +got):\n%s", diff)
106125
}

0 commit comments

Comments
 (0)