Do Not Merge: feat(resource builder): allow to inject tls configuration into annotated config maps: Rework #1350
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,220 @@ | ||
| package resourcebuilder | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "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" | ||
| operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1" | ||
| 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" | ||
| ) | ||
|
|
||
| type optional[T any] struct { | ||
| value T | ||
| found bool | ||
| } | ||
|
|
||
| type tlsConfig struct { | ||
| minTLSVersion optional[string] | ||
| cipherSuites optional[[]string] | ||
| } | ||
|
|
||
| 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 TLS profile injection", cm.Namespace, cm.Name) | ||
| return nil | ||
| } | ||
|
|
||
| // Observe TLS configuration from APIServer | ||
| tlsConf, err := b.observeTLSConfiguration(ctx, cm) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to observe TLS configuration: %v", err) | ||
| } | ||
|
|
||
| minTLSLog := "<not found>" | ||
| if tlsConf.minTLSVersion.found { | ||
| minTLSLog = tlsConf.minTLSVersion.value | ||
| } | ||
| cipherSuitesLog := "<not found>" | ||
| if tlsConf.cipherSuites.found { | ||
| cipherSuitesLog = fmt.Sprintf("%v", tlsConf.cipherSuites.value) | ||
| } | ||
| klog.V(4).Infof("ConfigMap %s/%s: observed minTLSVersion=%v, cipherSuites=%v", | ||
| cm.Namespace, cm.Name, minTLSLog, cipherSuitesLog) | ||
|
|
||
| // 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 | ||
| continue | ||
| } | ||
|
|
||
| // Check if this is a supported GenericOperatorConfig kind | ||
| if rnode.GetKind() != "GenericOperatorConfig" || rnode.GetApiVersion() != operatorv1alpha1.GroupVersion.String() { | ||
|
Contributor
Author
There was a problem hiding this comment. apiVersion check |
||
| klog.V(4).Infof("ConfigMap's %q entry is not a supported GenericOperatorConfig, skipping this entry", key) | ||
| continue | ||
| } | ||
|
|
||
| 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, tlsConf); err != nil { | ||
| return fmt.Errorf("failed to inject the TLS configuration: %v", err) | ||
|
Contributor
Author
There was a problem hiding this comment. bubbling up some context of errors |
||
| } | ||
|
|
||
| // Marshal the modified RNode back to YAML | ||
| modifiedYAML, err := rnode.String() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshall the modified ConfigMap back to YAML: %v", err) | ||
| } | ||
|
|
||
| // Update the ConfigMap data entry with the modified YAML | ||
| cm.Data[key] = modifiedYAML | ||
| klog.V(2).Infof("ConfigMap %s/%s updated GenericOperatorConfig with TLS profile in key %s", cm.Namespace, cm.Name, key) | ||
|
Contributor
Author
There was a problem hiding this comment. the tls configuration remains the same across all keys, no need to log it on every processed key |
||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // observeTLSConfiguration retrieves TLS configuration from the APIServer cluster CR | ||
| // using ObserveTLSSecurityProfile and extracts minTLSVersion and cipherSuites. | ||
| func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.ConfigMap) (*tlsConfig, error) { | ||
| // 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 { | ||
| return nil, fmt.Errorf("error observing TLS profile for ConfigMap %s/%s: %w", cm.Namespace, cm.Name, errors.Join(errs...)) | ||
|
Contributor
Author
There was a problem hiding this comment. hard failure; here i would like us to be defensive with testing to make sure any future bumps won't cause issues
Contributor
Author
There was a problem hiding this comment. however, we already have extensive testing, I believe |
||
| } | ||
|
|
||
| config := &tlsConfig{} | ||
|
|
||
| // Extract minTLSVersion from the observed config | ||
| if minTLSVersion, minTLSFound, err := unstructured.NestedString(observedConfig, "servingInfo", "minTLSVersion"); err != nil { | ||
| return nil, err | ||
| } else if minTLSFound { | ||
| config.minTLSVersion = optional[string]{value: minTLSVersion, found: true} | ||
| } | ||
|
|
||
| // Extract cipherSuites from the observed config | ||
| if cipherSuites, ciphersFound, err := unstructured.NestedStringSlice(observedConfig, "servingInfo", "cipherSuites"); err != nil { | ||
| return nil, err | ||
| } else if ciphersFound { | ||
| // Sort cipher suites for consistent ordering | ||
| sort.Strings(cipherSuites) | ||
| config.cipherSuites = optional[[]string]{value: cipherSuites, found: true} | ||
| } | ||
|
|
||
| return config, nil | ||
| } | ||
|
|
||
| // updateRNodeWithTLSSettings injects TLS settings into a GenericOperatorConfig RNode while preserving structure. | ||
| // If a field in tlsConf is not found, the corresponding field will be deleted from the RNode. | ||
| func updateRNodeWithTLSSettings(rnode *yaml.RNode, tlsConf *tlsConfig) error { | ||
|
Contributor
Author
There was a problem hiding this comment. New unit tests to ensure the YAML processing remains functional. |
||
| servingInfo, err := rnode.Pipe(yaml.LookupCreate(yaml.MappingNode, "servingInfo")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Handle cipherSuites field | ||
| if tlsConf.cipherSuites.found { | ||
| seqNode := yaml.NewListRNode(tlsConf.cipherSuites.value...) | ||
| if err := servingInfo.PipeE(yaml.SetField("cipherSuites", seqNode)); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| if err := servingInfo.PipeE(yaml.Clear("cipherSuites")); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Handle minTLSVersion field | ||
| if tlsConf.minTLSVersion.found { | ||
| if err := servingInfo.PipeE(yaml.SetField("minTLSVersion", yaml.NewStringRNode(tlsConf.minTLSVersion.value))); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| if err := servingInfo.PipeE(yaml.Clear("minTLSVersion")); err != nil { | ||
|
Contributor
Author
There was a problem hiding this comment. Clearing if not found to honor the observeTLSConfiguration function |
||
| return err | ||
| } | ||
| } | ||
|
|
||
| return 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 | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
It turns out that
GenericOperatorConfigis something that has been left alone inv1alpha1and has been replaced byGenericControllerConfigonconfigv1. We need to support both, specially now we want to keep things consistent with the work we are doing on hypershift.cc @ingvagabund