CNTRLPLANE-2777: wip: feat(resource builder): allow to inject tls configuration into annotated config maps#1322
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ingvagabund The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
lib/resourcebuilder/core.go
Outdated
| } | ||
|
|
||
| // Process each data entry that contains GenericOperatorConfig | ||
| for key, value := range cm.Data { |
There was a problem hiding this comment.
have you considered going for a single config.yaml entry ?
There was a problem hiding this comment.
Some operators use a different name for the field. E.g. https://github.com/openshift/service-ca-operator/blob/main/manifests/03_cm.yaml#L12 and https://github.com/openshift/service-serving-cert-signer/blob/master/manifests/0000_09_service-serving-cert-signer_03_cm.yaml#L7 set operator-config.yaml entry.
ingvagabund
force-pushed
the
configmap-tls-injection
branch
4 times, most recently
from
9eac62c to
938cef8
Compare
ingvagabund
force-pushed
the
configmap-tls-injection
branch
from
938cef8 to
bea8cc6
Compare
|
@ingvagabund: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // Parse YAML into RNode to preserve formatting and field order | ||
| rnode, err := yaml.Parse(value) | ||
| if err != nil { | ||
| // Not valid YAML, skip this entry |
There was a problem hiding this comment.
should we log these kind of scenarios ? maybe with a higher verbosity level ?
| // Check if this is a GenericOperatorConfig by checking the kind field | ||
| kind, err := rnode.GetString("kind") | ||
| if err != nil || kind != "GenericOperatorConfig" { | ||
| // Not a GenericOperatorConfig, skip this entry |
There was a problem hiding this comment.
ditto (in case of err != nil).
| recorder := events.NewInMemoryRecorder("configmap-tls-injection", clock.RealClock{}) | ||
|
|
||
| // Call ObserveTLSSecurityProfile to get TLS configuration | ||
| observedConfig, errs := apiserver.ObserveTLSSecurityProfile(listers, recorder, map[string]interface{}{}) |
|
/cc |
ingvagabund
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@ingvagabund: This pull request references CNTRLPLANE-2777 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
4 participants
Design notes:
NewControllerCommandConfiggets--configflag where operatorv1alpha1.GenericOperatorConfig manifest is expected..servingInfo.minTLSVersionand.servingInfo.cipherSuiteswhere CVO can inject the centralized TLS configuration.lib/resourcebuilder.buildercan be extended to inject the tls configuration the same way as proxy injection is.config.openshift.io/inject-tlscan be injected with the TLS configuration.