CNTRLPLANE-2777: feat(resource builder): allow to inject tls configuration into annotated config maps #1322
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,249 @@ | ||
| package resourcebuilder | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "slices" | ||
| "sort" | ||
|
|
||
| "sigs.k8s.io/kustomize/kyaml/yaml" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" | ||
| configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" | ||
| "github.com/openshift/library-go/pkg/operator/configobserver/apiserver" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| "github.com/openshift/library-go/pkg/operator/resourcesynccontroller" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| "k8s.io/client-go/tools/cache" | ||
| "k8s.io/klog/v2" | ||
| "k8s.io/utils/clock" | ||
| ) | ||
|
|
||
| const ( | ||
| // ConfigMapInjectTLSAnnotation is the annotation key that triggers TLS injection into ConfigMaps | ||
| ConfigMapInjectTLSAnnotation = "config.openshift.io/inject-tls" | ||
| ) | ||
|
|
||
| func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) error { | ||
| // Check for TLS injection annotation | ||
| if value, ok := cm.Annotations[ConfigMapInjectTLSAnnotation]; !ok || value != "true" { | ||
| return nil | ||
| } | ||
|
|
||
| klog.V(2).Infof("ConfigMap %s/%s has %s annotation set to true", cm.Namespace, cm.Name, ConfigMapInjectTLSAnnotation) | ||
|
|
||
| // Empty data, nothing to inject into | ||
| if cm.Data == nil { | ||
| klog.V(2).Infof("ConfigMap %s/%s has empty data, skipping", cm.Namespace, cm.Name) | ||
| return nil | ||
| } | ||
|
|
||
| // Observe TLS configuration from APIServer | ||
| minTLSVersion, minTLSFound, cipherSuites, ciphersFound, err := b.observeTLSConfiguration(ctx, cm) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to observe TLS configuration: %v", err) | ||
| } | ||
| if !minTLSFound && !ciphersFound { | ||
| klog.V(2).Infof("ConfigMap %s/%s: no TLS configuration found, skipping", cm.Namespace, cm.Name) | ||
| return nil | ||
| } | ||
|
|
||
| klog.V(4).Infof("Observing minTLSVersion=%v, cipherSuites=%v", minTLSVersion, cipherSuites) | ||
|
|
||
| // Process each data entry that contains GenericOperatorConfig | ||
| for key, value := range cm.Data { | ||
|
Contributor
There was a problem hiding this comment. This was supposed to be a comment about the need to document this functionality in the enhancement repository. However, you already thought of this on Slack, awesome! I will have a look for a right page. I am leaving this comment for me to remember this.
Contributor
There was a problem hiding this comment. I think that we can create a new document in https://github.com/openshift/enhancements/tree/master/dev-guide/cluster-version-operator/dev, as this will not be an enhancement per se, but rather a dev-guide documentation explaining the usage and any constraints. |
||
| klog.V(4).Infof("Processing %q key", key) | ||
| // Parse YAML into RNode to preserve formatting and field order | ||
| rnode, err := yaml.Parse(value) | ||
| if err != nil { | ||
| klog.V(4).Infof("ConfigMap's %q entry parsing failed: %v", key, err) | ||
| // Not valid YAML, skip this entry | ||
ingvagabund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| continue | ||
| } | ||
|
|
||
| // Check if this is a GenericOperatorConfig by checking the kind field | ||
| kind := rnode.GetKind() | ||
| if kind != "GenericOperatorConfig" { | ||
| klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skipping this entry", key) | ||
| continue | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| klog.V(2).Infof("ConfigMap %s/%s processing GenericOperatorConfig in key %s", cm.Namespace, cm.Name, key) | ||
|
|
||
| // Inject TLS settings into the GenericOperatorConfig while preserving structure | ||
| if err := updateRNodeWithTLSSettings(rnode, minTLSVersion, minTLSFound, cipherSuites, ciphersFound); err != nil { | ||
| klog.V(4).Infof("Error injecting the TLS configuration: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| // Marshal the modified RNode back to YAML | ||
| modifiedYAML, err := rnode.String() | ||
| if err != nil { | ||
| klog.V(4).Infof("Error marshalling the modified ConfigMap back to YAML: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| // Update the ConfigMap data entry with the modified YAML | ||
| cm.Data[key] = modifiedYAML | ||
| klog.V(2).Infof("ConfigMap %s/%s updated GenericOperatorConfig in key %s with %d ciphers and minTLSVersion=%s", | ||
| cm.Namespace, cm.Name, key, len(cipherSuites), minTLSVersion) | ||
| } | ||
|
|
||
| klog.V(2).Infof("APIServer config available for ConfigMap %s/%s TLS injection", cm.Namespace, cm.Name) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // observeTLSConfiguration retrieves TLS configuration from the APIServer cluster CR | ||
| // using ObserveTLSSecurityProfile and extracts minTLSVersion and cipherSuites. | ||
| // minTLSVersion string, minTLSFound bool, cipherSuites []string, ciphersFound bool, err error | ||
| func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.ConfigMap) (string, bool, []string, bool, error) { | ||
ingvagabund marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. This is something that was not apparent to me initially. One note on error handling. The For example, the This will result in a new different TLS configuration, rather than the existing one.
Contributor
There was a problem hiding this comment. I do not see a precedent in the
Member
Author
There was a problem hiding this comment. That's true. Nice observation. Even though this is a corner case it can still happen. So what you are saying is to get the actual CM when To analyze the execution path:
So @DavidHurta please double check with me in case I omitted something.
Member
Author
There was a problem hiding this comment. I am building a new unit test for the following valid APIServer CR: spec:
tlsSecurityProfile:
type: Custom
custom:
tlsProfileSpec:
ciphers: []
minTLSVersion: ""
Member
Author
There was a problem hiding this comment. I added more unit tests for cover the case where
Contributor
There was a problem hiding this comment. Apologies, I will respond to your comment tomorrow, it's getting late. However, I appreciate you taking the time to analyze the execution path. My goal is also for us to refine our approach to error handling in the proposed feature and have a clear intention. For example, a networking error happens when fetching the For example, looking at the ConfigObserver implementation, which calls
Member
Author
There was a problem hiding this comment.
Depending on whether the error is transient or not getting the current CM from the apiserver might fail as well.
Nice find. This makes it even more free/important in what the defaults should be. Even when the merging of two observedConfigs fails the execution continues. Leaving up to each operator to bear the consequences of making the "right" decision.
Member
Author
There was a problem hiding this comment.
The last valid configuration might not be always available. Also, temporarily setting defaults until the transient situation is resolved might cause blips. Not every client can accept the defaults. E.g. if the default minTLSVersion happens to be v1.3 but clients still need v1.2 (for whatever reason) setting these defaults might cause disruptions. |
||
| // Create a lister adapter for ObserveTLSSecurityProfile | ||
| lister := &apiServerListerAdapter{ | ||
| client: b.configClientv1.APIServers(), | ||
| ctx: ctx, | ||
| } | ||
| listers := &configObserverListers{ | ||
| apiServerLister: lister, | ||
| } | ||
|
|
||
| // Create an in-memory event recorder that doesn't send events to the API server | ||
| recorder := events.NewInMemoryRecorder("configmap-tls-injection", clock.RealClock{}) | ||
|
|
||
| // Call ObserveTLSSecurityProfile to get TLS configuration | ||
| observedConfig, errs := apiserver.ObserveTLSSecurityProfile(listers, recorder, map[string]any{}) | ||
| if len(errs) > 0 { | ||
| // Log errors but continue - ObserveTLSSecurityProfile is tolerant of missing config | ||
| for _, err := range errs { | ||
| klog.Errorf("ConfigMap %s/%s: error observing TLS profile: %v", cm.Namespace, cm.Name, err) | ||
| } | ||
| } | ||
|
|
||
| // Extract the TLS settings from the observed config | ||
| minTLSVersion, minTLSFound, err := unstructured.NestedString(observedConfig, "servingInfo", "minTLSVersion") | ||
| if err != nil { | ||
| // This error is unlikely to happen unless unstructured.NestedString is buggy. | ||
| // From unstructured.NestedString's description: | ||
| // "Returns false if value is not found and an error if not a string." | ||
| // The observedConfig's servingInfo.minTLSVersion is of a string type | ||
| return "", false, nil, false, err | ||
| } | ||
| cipherSuites, ciphersFound, _ := unstructured.NestedStringSlice(observedConfig, "servingInfo", "cipherSuites") | ||
| if err != nil { | ||
| // This error is unlikely to happen unless unstructured.NestedStringSlice is buggy | ||
| // From unstructured.NestedString's description: | ||
| // "Returns false if value is not found and an error if not a []interface{} or contains non-string items in the slice." | ||
| // The observedConfig's servingInfo.minTLSVersion is of a string type | ||
| return "", false, nil, false, err | ||
| } | ||
|
|
||
| // Sort cipher suites for consistent comparison | ||
| if ciphersFound && len(cipherSuites) > 0 { | ||
| sort.Strings(cipherSuites) | ||
| } | ||
|
|
||
| return minTLSVersion, minTLSFound, cipherSuites, ciphersFound, nil | ||
| } | ||
|
|
||
| // updateRNodeWithTLSSettings injects TLS settings into a GenericOperatorConfig RNode while preserving structure | ||
| // cipherSuites is expected to be sorted | ||
| func updateRNodeWithTLSSettings(rnode *yaml.RNode, minTLSVersion string, minTLSFound bool, cipherSuites []string, ciphersFound bool) error { | ||
|
Contributor
There was a problem hiding this comment. One thing we'll need to ensure is to be explicit about the state of the desired ConfigMap object. That is, if ciphers are not found, or the minimal TLS version is not found, we'll need to explicitly state the ConfigMap contents. We work with the contents of a manifest file in
Member
Author
There was a problem hiding this comment. I have not been able to find any documentation saying what's the expected outcome when only a partial custom TLS configuration is provided. The implementation itself in the library-go's innerTLSSecurityProfileObservations allows the following: Empty custom tls profile configuration: config: &configv1.TLSSecurityProfile{
Type: configv1.TLSProfileCustomType,
Custom: &configv1.CustomTLSProfile{},
},
Only ciphers are set: config: &configv1.TLSSecurityProfile{
Type: configv1.TLSProfileCustomType,
Custom: &configv1.CustomTLSProfile{
TLSProfileSpec: configv1.TLSProfileSpec{
Ciphers: []string{
"TLS_AES_128_GCM_SHA256",
},
},
},
},
When invoked through Yet, each operator can decide differently. I.e. to preserve or to clear. E.g. checking KSO implementation (other operators adopted the same pattern by copy-pasting) the rendered
Member
Author
There was a problem hiding this comment. Just recently merged https://github.com/openshift/enhancements/blob/master/enhancements/security/centralized-tls-config.md proposal introduces a new |
||
| servingInfo, err := rnode.Pipe(yaml.LookupCreate(yaml.MappingNode, "servingInfo")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if ciphersFound { | ||
| currentCiphers, err := getSortedCipherSuites(servingInfo) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !slices.Equal(currentCiphers, cipherSuites) { | ||
| // Create a sequence node with the cipher suites | ||
| seqNode := yaml.NewListRNode(cipherSuites...) | ||
| if err := servingInfo.PipeE(yaml.SetField("cipherSuites", seqNode)); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Update minTLSVersion if found | ||
| if minTLSFound { | ||
| if err := servingInfo.PipeE(yaml.SetField("minTLSVersion", yaml.NewStringRNode(minTLSVersion))); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // getSortedCipherSuites extracts and sorts the cipherSuites string slice from a servingInfo RNode | ||
| func getSortedCipherSuites(servingInfo *yaml.RNode) ([]string, error) { | ||
| ciphersNode, err := servingInfo.Pipe(yaml.Lookup("cipherSuites")) | ||
| if err != nil || ciphersNode == nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| elements, err := ciphersNode.Elements() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var ciphers []string | ||
| for _, elem := range elements { | ||
| // For scalar nodes, access the value directly without YAML serialization | ||
| // This avoids the trailing newline that String() (which uses yaml.Encode) adds | ||
| if elem.YNode().Kind == yaml.ScalarNode { | ||
| value := elem.YNode().Value | ||
| // Skip empty values | ||
| if value == "" { | ||
| continue | ||
| } | ||
| ciphers = append(ciphers, value) | ||
| } | ||
| } | ||
|
|
||
| // Sort cipher suites for consistent comparison | ||
| sort.Strings(ciphers) | ||
|
|
||
| return ciphers, nil | ||
| } | ||
|
|
||
| // apiServerListerAdapter adapts a client interface to the lister interface | ||
| type apiServerListerAdapter struct { | ||
| client configclientv1.APIServerInterface | ||
| ctx context.Context | ||
| } | ||
|
|
||
| func (a *apiServerListerAdapter) List(selector labels.Selector) ([]*configv1.APIServer, error) { | ||
| // Not implemented - ObserveTLSSecurityProfile only uses Get() | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (a *apiServerListerAdapter) Get(name string) (*configv1.APIServer, error) { | ||
| return a.client.Get(a.ctx, name, metav1.GetOptions{}) | ||
| } | ||
|
|
||
| // configObserverListers implements the configobserver.Listers interface. | ||
| // It's expected to be used solely for apiserver.ObserveTLSSecurityProfile. | ||
| type configObserverListers struct { | ||
| apiServerLister configlistersv1.APIServerLister | ||
| } | ||
|
|
||
| func (l *configObserverListers) APIServerLister() configlistersv1.APIServerLister { | ||
| return l.apiServerLister | ||
| } | ||
|
|
||
| func (l *configObserverListers) ResourceSyncer() resourcesynccontroller.ResourceSyncer { | ||
| // Not needed for TLS observation | ||
| return nil | ||
| } | ||
|
|
||
| func (l *configObserverListers) PreRunHasSynced() []cache.InformerSynced { | ||
| // Not needed for TLS observation | ||
| return nil | ||
| } | ||
|
Comment on lines
+241
to
+249
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: # First, let's understand the repo structure and find the relevant files
git ls-files | grep -E "(resourcebuilder|configobserver)" | head -20Repository: openshift/cluster-version-operator Length of output: 1188 🏁 Script executed: # Read the core.go file to see the full context
cat -n lib/resourcebuilder/core.go | sed -n '220,240p'
# Also check the interface definition
cat -n lib/resourcebuilder/interface.go | head -50
# Look at the observe_tlssecurityprofile implementation
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_tlssecurityprofile.goRepository: openshift/cluster-version-operator Length of output: 8005 🏁 Script executed: # Find the APIServerLister interface definition
rg -n "type APIServerLister" --type go
# Search for all uses of ResourceSyncer in the codebase
rg -n "ResourceSyncer" --type go
# Search for all uses of PreRunHasSynced in the codebase
rg -n "PreRunHasSynced" --type go
# Find where configObserverListers is instantiated
rg -n "configObserverListers{" --type goRepository: openshift/cluster-version-operator Length of output: 2179 🏁 Script executed: # Read the config_observer_controller.go around the lines that call PreRunHasSynced
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/config_observer_controller.go | sed -n '35,45p'
# Read the context around lines 255-257 where PreRunHasSynced is called
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/config_observer_controller.go | sed -n '250,270p'
# Look at the full Listers interface definition
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/config_observer_controller.go | sed -n '33,50p'
# Check the apiserver listers.go to understand the interface
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/listers.goRepository: openshift/cluster-version-operator Length of output: 3078 🏁 Script executed: # Find where listersToInformer is called
rg -n "listersToInformer" --type go
# Look for the Run method or startup code that uses this
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/config_observer_controller.go | sed -n '200,250p'
# Check how configObserverListers is initialized and used in lib/resourcebuilder/core.go
cat -n lib/resourcebuilder/core.go | sed -n '1,50p'
# Look for where configObserverListers is created/instantiated
rg -n "New.*configObserverListers\|&configObserverListers" --type goRepository: openshift/cluster-version-operator Length of output: 6154 🏁 Script executed: # Look at the code around line 126 in config_observer_controller.go
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/config_observer_controller.go | sed -n '100,140p'
# Search for any calls to ResourceSyncer() in the codebase
rg -n "\.ResourceSyncer\(\)" --type go -A 2
# Check if the configObserverListers is actually used with the config observer controller
grep -r "configObserverListers\|NewConfigObserverListers" lib/ --include="*.go"Repository: openshift/cluster-version-operator Length of output: 2578 🏁 Script executed: # Find where configObserverListers is used
rg -n "configObserverListers" lib/resourcebuilder/core.go -B 5 -A 10
# Check if it's passed to NewNestedConfigObserver or similar
rg -n "NewNestedConfigObserver\|NewConfigObserver" lib/ --include="*.go" -B 2 -A 2Repository: openshift/cluster-version-operator Length of output: 2026 🏁 Script executed: # Double-check the current usage is safe by seeing what ObserveTLSSecurityProfile actually does
cat -n vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_tlssecurityprofile.go | sed -n '35,80p'Repository: openshift/cluster-version-operator Length of output: 2460 Return proper implementations instead of
🤖 Prompt for AI Agents |
||
Uh oh!
There was an error while loading. Please reload this page.