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,250 @@ | ||
| package resourcebuilder | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "slices" | ||
| "sort" | ||
|
|
||
| "sigs.k8s.io/kustomize/kyaml/yaml" | ||
|
|
||
| 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" | ||
|
|
||
| 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" | ||
| ) | ||
|
|
||
| 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 { | ||
| 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" { | ||
|
There was a problem hiding this comment. It will be necessary to also support cc @DavidHurta |
||
| 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. I am thinking about the possible options and implementations. I am not opposed to the defaulting approach on errors, given we explicitly document this behaviour, and the current components, for which the feature is being implemented, are okay with this. However, it would result in the mentioned issues. One approach could also be to firmly error out on any issue, but it has the potential to break things. The logic is part of the CVO trying to apply a payload. Thus, if logic were brittle or prone to errors, this could hinder the CVO's ability to reconcile existing payload or to upgrade the cluster. Thus, there is a need for the logic to be somewhat resilient. Although quick transient errors are somewhat acceptable, IMO. I really do like the proposed approach. That is, to de facto make sure that the last valid TLS configuration is applied. No blips. No hard CVO failures. Although a good point regarding the metrics and alerting. I assume these issues should be transient, but who certainly knows, and this is an important security aspect. If I understand correctly, the CVO’s goal would be to ensure that it leaves the existing TLS configuration when it cannot correctly assess the desired TLS configuration. Alas, keep the existing configuration on errors. As to the proposed mechanism, I believe such usage of the annotation is to some degree outside the existing processing pipeline, and it also has some downsides. For example, a ConfigMap may contain critical data that needs to be applied to the cluster. However, I am pondering about the option and its similar alternatives. I am trying to implement the thing as well, but it might take me a day. I suppose one of the big upsides of the current CVO applying pipeline is its simplicity, which is handy for critical mechanisms. Thus, the introduction of mechanisms able to error out on a number of things needs to be crafted cautiously, IMO.
Contributor
There was a problem hiding this comment. I am working on and testing some potential changes for this PR in #1350. However, the CI is busted for me to verify the logs (looks unrelated, but requires further investigation). I am leaning towards the The approach should be one of the most straightforward, simplest, ensuring only a valid applied TLS configuration, no blips, and a clear signal to the cluster administrator in case something goes wrong approaches.
Member
Author
There was a problem hiding this comment. Thank you for sharing the code improvements. When it comes to the error handling the only difference is a replacement of a warning with an error when
Contributor
There was a problem hiding this comment. I believe so, yes. And a bunch of smaller changes around other areas. I have quickly left a few comments on the PR, highlighting them. Although I have glanced at metrics from the CI job of the PR, and I'll need to crosscheck it with more CI jobs.
Contributor
There was a problem hiding this comment. We could lock in the expected behavior of The error out approach requires additional investigation to verify its correctness,but it looks promising, IMO. |
||
| // 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
Contributor
There was a problem hiding this comment. Yes, the field has now landed in the openshift/api repository. I do not know how or whether the |
||
| 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
+242
to
+250
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 |
||
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.
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.
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.
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.
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.
fyi @vincentdephily