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" | ||||||||||||||
|
|
||||||||||||||
| 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" | ||||||||||||||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||||||||||||||
| 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. |
||||||||||||||
| 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, err := rnode.GetString("kind") | ||||||||||||||
|
Contributor
There was a problem hiding this comment. We may use the available the
Contributor
There was a problem hiding this comment. Can we also ensure to check the API version? The My goal is to be explicit about the API we are supporting, and to not make malformed configurations in the future in case the API changes, which is possible. If the API changes greatly, we may pivot to using a scheme-aware decoder for the YAML content and typed objects for modifications instead of fine-grained YAML processing. This was something I wanted to propose to us to use, as the YAML processing is more prone to errors. However, the order of the fields is then not preserved (need to double-check), and additional logic would need to be implemented to teach the CVO to semantically check ConfigMap contents in this scenario for equality. Given the urgency, the extensive testing, and the likelihood of the API not changing too much, I am fine with us going with this approach for the moment. The check for the API version should ensure we will not break other versions in the future. |
||||||||||||||
| if err != nil { | ||||||||||||||
| klog.V(4).Infof("ConfigMap's %q kind accessing failed: %v", key, err) | ||||||||||||||
| continue | ||||||||||||||
| } | ||||||||||||||
| if kind != "GenericOperatorConfig" { | ||||||||||||||
| klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skiping this entry", key) | ||||||||||||||
| continue | ||||||||||||||
|
Comment on lines
+76
to
+78
There was a problem hiding this comment. Typo: "skiping" → "skipping". if kind != "GenericOperatorConfig" {
- klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skiping this entry", key)
+ klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skipping this entry", key)
continue
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| 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) { | ||||||||||||||
|
Contributor
There was a problem hiding this comment. This is one of the more important comments. Currently, the proposed logic is somewhat different from what would usually be done by just the We result in finding nothing when On actual non-tolerated errors, the function will return the original configuration. It will not try to evaluate as many fields in a best-effort manner. The CVO maintainers are not experts in the area. Conceptually, I would rather have us entirely (or as much as possible) depend on the library-go logic for processing. These are rather edge-cases, but conceptually, I would rather the CVO to entirely trust the library-go for what a correct TLS configuration is supposed to look like in a given cluster.
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. |
||||||||||||||
| // First check if the APIServer cluster resource exists | ||||||||||||||
| _, err := b.configClientv1.APIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||||||||||||||
| if err != nil { | ||||||||||||||
| if apierrors.IsNotFound(err) { | ||||||||||||||
| klog.V(2).Infof("ConfigMap %s/%s: APIServer cluster resource not found, skipping TLS injection", cm.Namespace, cm.Name) | ||||||||||||||
| return "", false, nil, false, nil | ||||||||||||||
| } | ||||||||||||||
| // For transient API failures (network errors, timeouts, etc.), log at higher severity | ||||||||||||||
| klog.Warningf("ConfigMap %s/%s: failed to get APIServer cluster resource (transient failure), skipping TLS injection: %v", cm.Namespace, cm.Name, err) | ||||||||||||||
| return "", false, nil, false, err | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // 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.V(2).Infof("ConfigMap %s/%s: error observing TLS profile: %v", cm.Namespace, cm.Name, err) | ||||||||||||||
|
Contributor
There was a problem hiding this comment. If this is in fact an error logging, we may utilize the
Contributor
There was a problem hiding this comment. ... Although this could end up in continuous eternal errors due to a manifest not setting the |
||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Extract the TLS settings from the observed config | ||||||||||||||
| minTLSVersion, minTLSFound, _ := unstructured.NestedString(observedConfig, "servingInfo", "minTLSVersion") | ||||||||||||||
| cipherSuites, ciphersFound, _ := unstructured.NestedStringSlice(observedConfig, "servingInfo", "cipherSuites") | ||||||||||||||
|
|
||||||||||||||
| // 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 { | ||||||||||||||
| servingInfo, err := rnode.Pipe(yaml.LookupCreate(yaml.MappingNode, "servingInfo")) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if ciphersFound && len(cipherSuites) > 0 { | ||||||||||||||
| currentCiphers, err := getSortedCipherSuites(servingInfo) | ||||||||||||||
| if err != nil || !slices.Equal(currentCiphers, cipherSuites) { | ||||||||||||||
|
Contributor
There was a problem hiding this comment. We are silently ignoring errors here. Could we log the error in case the YAML is malformed? |
||||||||||||||
| // 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 && minTLSVersion != "" { | ||||||||||||||
| 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.
🧩 Analysis chain
🏁 Script executed:
Repository: openshift/cluster-version-operator
Length of output: 660
Simplify boolean check to be more idiomatic.
Line 127 uses explicit equality comparison
client['interface'] == True, which triggers Ruff E712. Useclient.get('interface')instead for cleaner, idiomatic Python.Proposed fix
🧰 Tools
🪛 Ruff (0.15.2)
[error] 127-127: Avoid equality comparisons to
True; useclient['interface']:for truth checksReplace with
client['interface'](E712)
🤖 Prompt for AI Agents