CNTRLPLANE-2777: feat(resource builder): allow to inject tls configuration into annotated config maps#1322
Open
ingvagabund wants to merge 2 commits intoopenshift:mainfrom
Open
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughAdds TLS-injection for ConfigMaps annotated Changes
Sequence DiagramsequenceDiagram
participant Builder as Resource Builder
participant CM as ConfigMap
participant APIServer as APIServer Lister
participant Observer as TLS Observer
participant YAML as kyaml Parser
participant RNode as RNode Modifier
Builder->>CM: Read annotations & data
CM-->>Builder: Return data & annotation
alt annotation == "true"
Builder->>APIServer: Get APIServer TLS/securityProfile
APIServer-->>Observer: Provide servingInfo / securityProfile
Observer-->>Builder: Return minTLSVersion + cipherSuites
Builder->>YAML: Parse each data entry
YAML-->>Builder: Identify GenericOperatorConfig entries
Builder->>RNode: Inject minTLSVersion & cipherSuites into nodes
RNode-->>YAML: Return modified YAML text
Builder->>CM: Update data entries and apply resource
CM-->>Builder: Persisted
else annotation missing/false
Builder-->>CM: Skip modification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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 ?
Member
Author
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
5 times, most recently
from
938cef8 to
bea8cc6
Compare
Contributor
|
/cc |
ingvagabund
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@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. |
ingvagabund
changed the title
Member
Author
|
@DavidHurta the high level functionality design is finished. Is the current implementation in the right direction that is aligned with how CVO is expected to reconcile? |
ingvagabund
force-pushed
the
configmap-tls-injection
branch
from
bea8cc6 to
4ae1b14
Compare
Member
Author
As you suggested we can either:
|
Contributor
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
lib/resourcebuilder/core_test.go (1)
64-84: Ignored error fromjson.Marshalin test helper.While marshalling a simple Pod struct should never fail, silently discarding the error at Line 82 is a test smell. Consider using
tparameter ormust-style helper.♻️ Suggested fix
Since this is a package-level helper without access to
*testing.T, a simple panic is acceptable in test code:-func makePodJSON() string { +func makePodJSON(t *testing.T) string { + t.Helper() pod := &corev1.Pod{ ... } - data, _ := json.Marshal(pod) + data, err := json.Marshal(pod) + if err != nil { + t.Fatalf("Failed to marshal Pod: %v", err) + } return string(data) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core_test.go` around lines 64 - 84, The helper makePodJSON ignores the error returned by json.Marshal; change it to fail loudly in tests by checking the error from json.Marshal(pod) and panicking (or using a must-style behavior) if err != nil so test failures are obvious; update the function makePodJSON to capture the error from json.Marshal and call panic with that error (or wrap a descriptive message) instead of discarding it.hack/generate-lib-resources.py (1)
127-127: Avoid equality comparison toTrue; use truthiness check instead.Per Ruff E712, use
client['interface']instead ofclient['interface'] == True.♻️ Suggested fix
- 'type': '{}{}.{}'.format("" if 'interface' in client and client['interface'] == True else "*", client_short_name, client['type']), + 'type': '{}{}.{}'.format("" if client.get('interface') else "*", client_short_name, client['type']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-lib-resources.py` at line 127, Replace the explicit equality check client['interface'] == True with a truthiness check so the 'type' dict entry uses if client.get('interface') or client['interface'] (truthy) instead of comparing to True; update the expression that builds the 'type' value (the dict entry using '{}{}.{}'.format(...)) to use a truthy test on client['interface'] to decide whether to include the empty string or "*" (reference the 'client' variable and the 'type' dict entry in the generator).lib/resourcebuilder/core.go (4)
98-104: Redundant API call:APIServers().Get()is called twice per ConfigMap.Line 100 fetches the APIServer resource to check existence, and then
ObserveTLSSecurityProfile(via theapiServerListerAdapter.Getat Line 212) fetches it again. This doubles the API server load for every annotated ConfigMap on each reconciliation cycle. Consider removing the pre-check and relying solely on the errors returned byObserveTLSSecurityProfile.♻️ Suggested simplification
func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.ConfigMap) (minTLSVersion string, minTLSFound bool, cipherSuites []string, ciphersFound bool) { - // First check if the APIServer cluster resource exists - _, err := b.configClientv1.APIServers().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - klog.V(2).Infof("ConfigMap %s/%s: APIServer cluster resource not found, skipping TLS injection: %v", cm.Namespace, cm.Name, err) - return "", false, nil, false - } - // Create a lister adapter for ObserveTLSSecurityProfile lister := &apiServerListerAdapter{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 98 - 104, The initial APIServer existence check in builder.observeTLSConfiguration that calls b.configClientv1.APIServers().Get should be removed to avoid duplicate API calls; instead, rely on ObserveTLSSecurityProfile (and the apiServerListerAdapter.Get it uses) to return appropriate errors if the APIServer is missing. Edit observeTLSConfiguration to delete the pre-check block (the call to b.configClientv1.APIServers().Get and its early return/log) and let the subsequent call to ObserveTLSSecurityProfile handle not-found or error cases, preserving existing logging where ObserveTLSSecurityProfile reports failures.
29-41: Empty (non-nil)Datamap silently passes through without logging.Line 38 checks
cm.Data == nilbut logs "empty data". IfDatais an empty map (map[string]string{}), it won't be logged as "empty" and the loop at Line 52 will simply not iterate. Consider also checkinglen(cm.Data) == 0for consistency with the log message.♻️ Minor fix
// Empty data, nothing to inject into - if cm.Data == nil { + if len(cm.Data) == 0 { klog.V(2).Infof("ConfigMap %s/%s has empty data, skipping", cm.Namespace, cm.Name) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 29 - 41, In modifyConfigMap on builder, the current check only tests cm.Data == nil but the log says "empty data" while an empty (non-nil) map will silently pass through; change the condition to treat both nil and zero-length maps as empty (e.g., if cm.Data == nil || len(cm.Data) == 0) and keep the klog message, or adjust the message to reflect nil vs empty; update the early-return branch so the range loop that follows (the iteration over cm.Data) isn't relied on to detect emptiness.
86-93: Log at Line 91 is misleading: it prints even when no entries were modified.The "APIServer config available for ConfigMap ... TLS injection" message is emitted unconditionally after the loop, even if no
GenericOperatorConfigentries were found or updated. Consider moving it inside the loop (where updates happen) or tracking whether any modification occurred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 86 - 93, The log "APIServer config available for ConfigMap ... TLS injection" is emitted unconditionally; update the code in the routine that iterates cm.Data and updates GenericOperatorConfig entries (the block that sets cm.Data[key] = modifiedYAML and calls klog.V(2).Infof about updated ciphers/minTLSVersion) to only emit the APIServer-config-available message when at least one entry was actually modified—either move that klog.V(2).Infof into the modification branch or introduce a boolean modified flag (e.g., modified := false; set to true when you change cm.Data[key]) and emit the message only if modified is true, referencing cm.Namespace and cm.Name as currently done.
141-166:servingInfois unconditionally created even when neither TLS field needs updating.
LookupCreate(yaml.MappingNode, "servingInfo")at Line 142 will create aservingInfomapping if it doesn't exist. If bothciphersFoundandminTLSFoundare false (shouldn't reach here given the caller check, but defensively), this would inject an emptyservingInfonode. The current caller guards against this, so this is a minor defensive-coding note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 141 - 166, The function updateRNodeWithTLSSettings unconditionally creates servingInfo via yaml.LookupCreate which can add an empty mapping when neither TLS value needs updating; change the logic to only obtain/create servingInfo when needed: check ciphersFound and minTLSFound first, and for cipherSuites call yaml.Lookup(servingInfoParent, "servingInfo") to see if it exists (and only create with yaml.SetField("servingInfo", yaml.NewMapRNode(...)) or yaml.LookupCreate when actually about to set fields), and similarly only create servingInfo when setting minTLSVersion; update the code paths in updateRNodeWithTLSSettings so servingInfo is created lazily (use yaml.Lookup instead of yaml.LookupCreate or defer yaml.LookupCreate until just before PipeE calls) while keeping the existing comparisons with getSortedCipherSuites and slices.Equal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resourcebuilder/core_test.go`:
- Around line 536-541: The table-driven test includes a shouldCallAPI boolean
that is never asserted in the test loop; either remove the unused shouldCallAPI
field from the tests slice or add an assertion inside the test loop that
inspects the fake client's recorded actions and compares whether an API call
occurred to tests[i].shouldCallAPI (e.g., check
fakeClient.Actions()/RecordedActions length or presence of a specific verb) so
the table entry is validated; update the tests slice entry points and the test
loop where the resource builder is exercised to perform this check.
- Around line 226-235: The test declares expectModified in TestModifyConfigMap
but never asserts it; update the test loop in TestModifyConfigMap to verify
expectModified for each case by comparing the original and returned/modified
*corev1.ConfigMap (or by using a boolean return if ModifyConfigMap returns one):
after calling the function under test, add an assertion that when
tc.expectModified is true the modified ConfigMap differs (use reflect.DeepEqual
or cmp.Diff) from the original, and when false they are equal; reference the
existing test case variable tc.expectModified and the test helper
validateConfigMap to keep behavior checks consistent.
In `@lib/resourcebuilder/core.go`:
- Around line 224-232: configObserverListers currently returns nil from
ResourceSyncer() and PreRunHasSynced(), violating the configobserver.Listers
contract and risking a panic in callers like listersToInformer() or
NewNestedConfigObserver(); fix by returning concrete no-op implementations: have
PreRunHasSynced() return an empty slice (e.g. return []cache.InformerSynced{}),
and implement a small noop ResourceSyncer type that satisfies
resourcesynccontroller.ResourceSyncer (methods are no-ops) and return an
instance from ResourceSyncer(); update core.go's configObserverListers methods
accordingly (referencing configObserverListers, ResourceSyncer(),
PreRunHasSynced(), ObserveTLSSecurityProfile, APIServerLister(),
listersToInformer(), NewNestedConfigObserver()) or, if you choose not to add a
noop, add a clear comment on configObserverListers' type header stating it must
never be passed to code paths that call these methods.
---
Nitpick comments:
In `@hack/generate-lib-resources.py`:
- Line 127: Replace the explicit equality check client['interface'] == True with
a truthiness check so the 'type' dict entry uses if client.get('interface') or
client['interface'] (truthy) instead of comparing to True; update the expression
that builds the 'type' value (the dict entry using '{}{}.{}'.format(...)) to use
a truthy test on client['interface'] to decide whether to include the empty
string or "*" (reference the 'client' variable and the 'type' dict entry in the
generator).
In `@lib/resourcebuilder/core_test.go`:
- Around line 64-84: The helper makePodJSON ignores the error returned by
json.Marshal; change it to fail loudly in tests by checking the error from
json.Marshal(pod) and panicking (or using a must-style behavior) if err != nil
so test failures are obvious; update the function makePodJSON to capture the
error from json.Marshal and call panic with that error (or wrap a descriptive
message) instead of discarding it.
In `@lib/resourcebuilder/core.go`:
- Around line 98-104: The initial APIServer existence check in
builder.observeTLSConfiguration that calls b.configClientv1.APIServers().Get
should be removed to avoid duplicate API calls; instead, rely on
ObserveTLSSecurityProfile (and the apiServerListerAdapter.Get it uses) to return
appropriate errors if the APIServer is missing. Edit observeTLSConfiguration to
delete the pre-check block (the call to b.configClientv1.APIServers().Get and
its early return/log) and let the subsequent call to ObserveTLSSecurityProfile
handle not-found or error cases, preserving existing logging where
ObserveTLSSecurityProfile reports failures.
- Around line 29-41: In modifyConfigMap on builder, the current check only tests
cm.Data == nil but the log says "empty data" while an empty (non-nil) map will
silently pass through; change the condition to treat both nil and zero-length
maps as empty (e.g., if cm.Data == nil || len(cm.Data) == 0) and keep the klog
message, or adjust the message to reflect nil vs empty; update the early-return
branch so the range loop that follows (the iteration over cm.Data) isn't relied
on to detect emptiness.
- Around line 86-93: The log "APIServer config available for ConfigMap ... TLS
injection" is emitted unconditionally; update the code in the routine that
iterates cm.Data and updates GenericOperatorConfig entries (the block that sets
cm.Data[key] = modifiedYAML and calls klog.V(2).Infof about updated
ciphers/minTLSVersion) to only emit the APIServer-config-available message when
at least one entry was actually modified—either move that klog.V(2).Infof into
the modification branch or introduce a boolean modified flag (e.g., modified :=
false; set to true when you change cm.Data[key]) and emit the message only if
modified is true, referencing cm.Namespace and cm.Name as currently done.
- Around line 141-166: The function updateRNodeWithTLSSettings unconditionally
creates servingInfo via yaml.LookupCreate which can add an empty mapping when
neither TLS value needs updating; change the logic to only obtain/create
servingInfo when needed: check ciphersFound and minTLSFound first, and for
cipherSuites call yaml.Lookup(servingInfoParent, "servingInfo") to see if it
exists (and only create with yaml.SetField("servingInfo", yaml.NewMapRNode(...))
or yaml.LookupCreate when actually about to set fields), and similarly only
create servingInfo when setting minTLSVersion; update the code paths in
updateRNodeWithTLSSettings so servingInfo is created lazily (use yaml.Lookup
instead of yaml.LookupCreate or defer yaml.LookupCreate until just before PipeE
calls) while keeping the existing comparisons with getSortedCipherSuites and
slices.Equal.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (294)
go.sumis excluded by!**/*.sumvendor/github.com/go-errors/errors/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/LICENSE.MITis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/error_1_13.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/error_backward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/parse_panic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-errors/errors/stackframe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/.deepsource.tomlis excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/CODE_OF_CONDUCT.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/merge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/imdario/mergo/mergo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.coderabbit.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.go-validated.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/AGENTS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/annotations/annotations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiserver/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/v1/types_apirequestcount.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiserver/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiserver/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/consts.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/deprecated_consts.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/authorization/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/authorization/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/build/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/consts.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/build/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/cloudnetwork/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/cloudnetwork/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/cloudnetwork/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_cli_download.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_external_log_links.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_link.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_notification.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_plugin.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_quick_start.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_sample.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_yaml_sample.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/types_helm_chart_repository.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/types_project_helm_chart_repository.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/helm/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/helm/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/image/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/types_imagepruner.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/imageregistry/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/imageregistry/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/kubecontrolplane/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/legacyconfig/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/legacyconfig/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/legacyconfig/v1/serialization.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/legacyconfig/v1/stringsource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/legacyconfig/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/legacyconfig/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/legacyconfig/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/common.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_alibabaprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_aws.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_nutanixprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_powervsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1alpha1/types_openstack.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_azureprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_gcpprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machinehealthcheck.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_provider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_vsphereprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/monitoring/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/monitoring/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/monitoring/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/constants.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1alpha1/types_dnsnameresolver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/networkoperator/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/types_egressrouter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/networkoperator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/networkoperator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/oauth/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/oauth/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/openshiftcontrolplane/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openshiftcontrolplane/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openshiftcontrolplane/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openshiftcontrolplane/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openshiftcontrolplane/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openshiftcontrolplane/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/openshiftcontrolplane/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/types_conditioncheck.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/osin/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/pkg/serialization/serialization.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/project/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/legacy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modhack/generate-lib-resources.pylib/resourcebuilder/core.golib/resourcebuilder/core_test.golib/resourcebuilder/resourcebuilder.golib/resourcebuilder/resourcebuilder_test.go
Comment on lines
+224
to
+232
| 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 | ||
| } |
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 nil for ResourceSyncer() and PreRunHasSynced(), or document that configObserverListers must never be passed to code paths that invoke these methods.
ObserveTLSSecurityProfile currently only calls APIServerLister() and never invokes ResourceSyncer() or PreRunHasSynced(), so the current usage is safe. However, configObserverListers implements the configobserver.Listers interface, which requires both methods. The listersToInformer() function in library-go's config_observer_controller.go (lines 255–257) iterates over the result of PreRunHasSynced() without nil-checking, which would panic if this lister is ever passed to NewNestedConfigObserver() or similar code paths. This pattern violates the interface contract and creates a landmine for future refactoring. Either return empty slices/objects instead of nil, or add a clear comment that this lister is only for TLS observation and must never be used with the config observer controller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/resourcebuilder/core.go` around lines 224 - 232, configObserverListers
currently returns nil from ResourceSyncer() and PreRunHasSynced(), violating the
configobserver.Listers contract and risking a panic in callers like
listersToInformer() or NewNestedConfigObserver(); fix by returning concrete
no-op implementations: have PreRunHasSynced() return an empty slice (e.g. return
[]cache.InformerSynced{}), and implement a small noop ResourceSyncer type that
satisfies resourcesynccontroller.ResourceSyncer (methods are no-ops) and return
an instance from ResourceSyncer(); update core.go's configObserverListers
methods accordingly (referencing configObserverListers, ResourceSyncer(),
PreRunHasSynced(), ObserveTLSSecurityProfile, APIServerLister(),
listersToInformer(), NewNestedConfigObserver()) or, if you choose not to add a
noop, add a clear comment on configObserverListers' type header stating it must
never be passed to code paths that call these methods.
Contributor
Hello, @ingvagabund. The high-level design looks good on my end. From quickly going through the code, the implementation looks good. I have reserved some time for myself to properly review the PR. Thanks for creating some tests, that's always appreciated! I see the openshift/cluster-image-registry-operator#1297 PR, which is great. We can leverage it to verify the PR before merging, if possible. |
Contributor
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/resourcebuilder/core.go (2)
163-168: Consider checking if minTLSVersion needs updating.Unlike
cipherSuites(which compares current vs. new values before updating),minTLSVersionis always set without checking the current value. While kyaml preserves YAML structure when setting the same value, adding a comparison would make the idempotency intent explicit and consistent with the cipher handling.// Update minTLSVersion if found if minTLSFound && minTLSVersion != "" { + currentMinTLS, err := servingInfo.GetString("minTLSVersion") + if err != nil || currentMinTLS != minTLSVersion { if err := servingInfo.PipeE(yaml.SetField("minTLSVersion", yaml.NewStringRNode(minTLSVersion))); err != nil { return err } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 163 - 168, The code unconditionally sets minTLSVersion when minTLSFound is true, unlike the cipherSuites logic which first compares existing vs new value; update the block around minTLSFound/minTLSVersion and servingInfo.PipeE to read the current "minTLSVersion" (via yaml.Field/Get or the same helper used for cipherSuites), compare it to minTLSVersion, and only call servingInfo.PipeE(yaml.SetField("minTLSVersion", yaml.NewStringRNode(minTLSVersion))) when they differ so the update is idempotent and consistent with cipher handling.
104-109: Minor: Redundant APIServer fetch.The APIServer is fetched twice: once explicitly at Line 105, then again inside
ObserveTLSSecurityProfilevia the lister adapter. Consider removing the explicit check sinceObserveTLSSecurityProfilealready handles missing config gracefully.func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.ConfigMap) (minTLSVersion string, minTLSFound bool, cipherSuites []string, ciphersFound bool) { - // First check if the APIServer cluster resource exists - _, err := b.configClientv1.APIServers().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - klog.V(2).Infof("ConfigMap %s/%s: APIServer cluster resource not found, skipping TLS injection: %v", cm.Namespace, cm.Name, err) - return "", false, nil, false - } - // Create a lister adapter for ObserveTLSSecurityProfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 104 - 109, Remove the redundant explicit APIServer fetch block that calls b.configClientv1.APIServers().Get and its error handling/klog/return; rely on ObserveTLSSecurityProfile (and its lister adapter) to detect a missing APIServer and handle it gracefully. Specifically, delete the lines invoking b.configClientv1.APIServers().Get(ctx, "cluster", metav1.GetOptions{}) and the subsequent if err != nil { ... return ... } block so the function flow continues to ObserveTLSSecurityProfile without an extra fetch.lib/resourcebuilder/core_test.go (1)
177-193: Minor: Variable shadowing reduces readability.The inner
foundvariable at Line 183 shadows the outerfoundfrom Line 173. While not a bug, consider renaming tocipherFoundfor clarity.- for _, expectedCipher := range expectedCiphers { - found := false - for _, cipher := range cipherSuites { - if cipher == expectedCipher { - found = true - break - } - } - if !found { + for _, expectedCipher := range expectedCiphers { + cipherFound := false + for _, cipher := range cipherSuites { + if cipher == expectedCipher { + cipherFound = true + break + } + } + if !cipherFound {Alternatively, use
slices.Contains:if !slices.Contains(cipherSuites, expectedCipher) { return fmt.Errorf("expected cipher %s not found in cipherSuites: %v", expectedCipher, cipherSuites) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core_test.go` around lines 177 - 193, The inner loop uses a variable named "found" which shadows the outer "found" (from the outer scope) and reduces readability; rename the inner boolean to "cipherFound" (or similar) in the loop that iterates over cipherSuites to avoid shadowing, or replace the whole inner loop by calling slices.Contains(cipherSuites, expectedCipher) to check presence and simplify the logic in the test that iterates over expectedCiphers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resourcebuilder/core.go`:
- Around line 70-72: The log message in the ConfigMap handling branch (where
kind != "GenericOperatorConfig") contains a typo ("skiping"); update the
klog.V(4).Infof call string in the code path that checks kind (the conditional
using variable kind) to correct "skiping" to "skipping" so the message logged by
klog.V(4).Infof accurately reads "skipping this entry".
---
Nitpick comments:
In `@lib/resourcebuilder/core_test.go`:
- Around line 177-193: The inner loop uses a variable named "found" which
shadows the outer "found" (from the outer scope) and reduces readability; rename
the inner boolean to "cipherFound" (or similar) in the loop that iterates over
cipherSuites to avoid shadowing, or replace the whole inner loop by calling
slices.Contains(cipherSuites, expectedCipher) to check presence and simplify the
logic in the test that iterates over expectedCiphers.
In `@lib/resourcebuilder/core.go`:
- Around line 163-168: The code unconditionally sets minTLSVersion when
minTLSFound is true, unlike the cipherSuites logic which first compares existing
vs new value; update the block around minTLSFound/minTLSVersion and
servingInfo.PipeE to read the current "minTLSVersion" (via yaml.Field/Get or the
same helper used for cipherSuites), compare it to minTLSVersion, and only call
servingInfo.PipeE(yaml.SetField("minTLSVersion",
yaml.NewStringRNode(minTLSVersion))) when they differ so the update is
idempotent and consistent with cipher handling.
- Around line 104-109: Remove the redundant explicit APIServer fetch block that
calls b.configClientv1.APIServers().Get and its error handling/klog/return; rely
on ObserveTLSSecurityProfile (and its lister adapter) to detect a missing
APIServer and handle it gracefully. Specifically, delete the lines invoking
b.configClientv1.APIServers().Get(ctx, "cluster", metav1.GetOptions{}) and the
subsequent if err != nil { ... return ... } block so the function flow continues
to ObserveTLSSecurityProfile without an extra fetch.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/resourcebuilder/core.golib/resourcebuilder/core_test.go
Comment on lines
+70
to
+72
| if kind != "GenericOperatorConfig" { | ||
| klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skiping this entry", key) | ||
| continue |
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if kind != "GenericOperatorConfig" { | |
| klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skiping this entry", key) | |
| continue | |
| if kind != "GenericOperatorConfig" { | |
| klog.V(4).Infof("ConfigMap's %q entry is not a GenericOperatorConfig, skipping this entry", key) | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/resourcebuilder/core.go` around lines 70 - 72, The log message in the
ConfigMap handling branch (where kind != "GenericOperatorConfig") contains a
typo ("skiping"); update the klog.V(4).Infof call string in the code path that
checks kind (the conditional using variable kind) to correct "skiping" to
"skipping" so the message logged by klog.V(4).Infof accurately reads "skipping
this entry".
Contributor
|
@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. |
Contributor
|
@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. |
Contributor
|
@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. |
Contributor
|
@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. |
Contributor
|
@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. |
Contributor
|
@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. |
Contributor
|
@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. |
Contributor
|
@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. |
ingvagabund
force-pushed
the
configmap-tls-injection
branch
from
1718c36 to
66b3ff8
Compare
Contributor
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/resourcebuilder/core.go (2)
70-72:⚠️ Potential issue | 🟡 MinorFix typo in log text (
skiping→skipping).Line 71 still has a typo in a user-visible diagnostic message.
Suggested change
- 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)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 70 - 72, The log message in the GenericOperatorConfig check contains a typo ("skiping") — update the klog.V(4).Infof call inside the block that checks kind != "GenericOperatorConfig" (the line using klog.V(4).Infof with the key variable) to use "skipping" instead of "skiping" so the user-visible diagnostic reads correctly.
230-237:⚠️ Potential issue | 🟡 MinorAvoid returning
nilfromconfigobserver.Listersno-op methods.Lines 230-237 still return
nil. This is brittle if this adapter is ever reused by code paths that iterate/use these methods without nil-guards.#!/bin/bash # Inspect interface contract and likely call paths fd 'config_observer_controller.go$' vendor | xargs -r -I{} sh -c ' echo "== {} =="; rg -n "type Listers|PreRunHasSynced|ResourceSyncer|listersToInformer" "{}"; ' rg -n -C3 'func \(l \*configObserverListers\) ResourceSyncer|func \(l \*configObserverListers\) PreRunHasSynced' lib/resourcebuilder/core.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 230 - 237, The methods currently return nil which is brittle; change ResourceSyncer() to return a no-op implementation instead of nil by adding a private noop type in this file that implements resourcesynccontroller.ResourceSyncer (e.g., noopResourceSyncer with the required methods) and return &noopResourceSyncer{} from configObserverListers.ResourceSyncer, and change PreRunHasSynced() to return an empty slice (return []cache.InformerSynced{}) instead of nil so callers can safely iterate without nil checks.
🧹 Nitpick comments (2)
lib/resourcebuilder/core.go (1)
38-45: Skip TLS observation whenConfigMap.Datais empty.Line 38 checks only
nil; an empty map still causes unnecessary APIServer observation calls each reconcile.Suggested change
- // Empty data, nothing to inject into - if cm.Data == nil { + // Empty data, nothing to inject into + if len(cm.Data) == 0 { klog.V(2).Infof("ConfigMap %s/%s has empty data, skipping", cm.Namespace, cm.Name) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 38 - 45, The current nil-check only skips when ConfigMap.Data is nil but still calls observeTLSConfiguration for empty maps; update the early return in the code that handles the ConfigMap (the if block that currently checks "if cm.Data == nil") to also return when the map is empty (e.g., check len(cm.Data) == 0) so that observeTLSConfiguration(ctx, cm) is not invoked for empty ConfigMaps; this change should be applied where cm.Data is tested before calling the method observeTLSConfiguration.hack/generate-lib-resources.py (1)
127-127: Use an idiomatic truthy check for the interface flag.Line 127 uses
== True; preferclient.get("interface")for clarity and PEP 8 compliance.Suggested change
- 'type': '{}{}.{}'.format("" if 'interface' in client and client['interface'] == True else "*", client_short_name, client['type']), + 'type': '{}{}.{}'.format("" if client.get('interface') else "*", client_short_name, client['type']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-lib-resources.py` at line 127, Replace the explicit "'interface' in client and client['interface'] == True" check in the dict construction for the 'type' key with a Pythonic truthy lookup using client.get('interface'); e.g. change the ternary to use "" if client.get('interface') else "*" so the expression becomes "'type': '{}{}.{}'.format("" if client.get('interface') else "*", client_short_name, client['type'])", referencing the same client, client_short_name, and client['type'] symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resourcebuilder/core.go`:
- Around line 105-108: The call to b.configClientv1.APIServers().Get in the
function handling TLS injection currently treats all errors as "not found" and
returns success; change it to check apierrors.IsNotFound(err) and only take the
skip path when that is true, otherwise log the error (using klog.Errorf or
similar) with context (cm.Namespace, cm.Name) and return the error so the
reconciler can retry; update the block around
b.configClientv1.APIServers().Get(...) to differentiate NotFound vs other errors
and return non-NotFound errors upward instead of returning "", false, nil,
false.
---
Duplicate comments:
In `@lib/resourcebuilder/core.go`:
- Around line 70-72: The log message in the GenericOperatorConfig check contains
a typo ("skiping") — update the klog.V(4).Infof call inside the block that
checks kind != "GenericOperatorConfig" (the line using klog.V(4).Infof with the
key variable) to use "skipping" instead of "skiping" so the user-visible
diagnostic reads correctly.
- Around line 230-237: The methods currently return nil which is brittle; change
ResourceSyncer() to return a no-op implementation instead of nil by adding a
private noop type in this file that implements
resourcesynccontroller.ResourceSyncer (e.g., noopResourceSyncer with the
required methods) and return &noopResourceSyncer{} from
configObserverListers.ResourceSyncer, and change PreRunHasSynced() to return an
empty slice (return []cache.InformerSynced{}) instead of nil so callers can
safely iterate without nil checks.
---
Nitpick comments:
In `@hack/generate-lib-resources.py`:
- Line 127: Replace the explicit "'interface' in client and client['interface']
== True" check in the dict construction for the 'type' key with a Pythonic
truthy lookup using client.get('interface'); e.g. change the ternary to use ""
if client.get('interface') else "*" so the expression becomes "'type':
'{}{}.{}'.format("" if client.get('interface') else "*", client_short_name,
client['type'])", referencing the same client, client_short_name, and
client['type'] symbols.
In `@lib/resourcebuilder/core.go`:
- Around line 38-45: The current nil-check only skips when ConfigMap.Data is nil
but still calls observeTLSConfiguration for empty maps; update the early return
in the code that handles the ConfigMap (the if block that currently checks "if
cm.Data == nil") to also return when the map is empty (e.g., check len(cm.Data)
== 0) so that observeTLSConfiguration(ctx, cm) is not invoked for empty
ConfigMaps; this change should be applied where cm.Data is tested before calling
the method observeTLSConfiguration.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
hack/generate-lib-resources.pylib/resourcebuilder/core.golib/resourcebuilder/core_test.golib/resourcebuilder/resourcebuilder.golib/resourcebuilder/resourcebuilder_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/resourcebuilder/resourcebuilder.go
ingvagabund
force-pushed
the
configmap-tls-injection
branch
from
66b3ff8 to
3d90a6b
Compare
Contributor
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/resourcebuilder/core.go (1)
223-226: Return an error fromList()instead of silently returning empty results.The current implementation returns
(nil, nil), which silently succeeds if any future code callsList(). This can hide regressions if upstream observer code changes to useList()instead ofGet(). Returning an explicit error makes unexpected call patterns immediately visible.Proposed refactor
func (a *apiServerListerAdapter) List(selector labels.Selector) ([]*configv1.APIServer, error) { - // Not implemented - ObserveTLSSecurityProfile only uses Get() - return nil, nil + // Not implemented for this adapter; fail loudly if call paths change. + return nil, fmt.Errorf("apiServerListerAdapter.List is not implemented") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resourcebuilder/core.go` around lines 223 - 226, The List method on apiServerListerAdapter currently returns (nil, nil) which silently succeeds; change it to return a non-nil error so callers know List is unsupported — update apiServerListerAdapter.List(selector labels.Selector) to return an explicit error (e.g. errors.New or fmt.Errorf) stating that List is not implemented/unsupported for this adapter (or wrap with context like "apiServerListerAdapter: List not implemented"), keeping the Get() behavior untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/generate-lib-resources.py`:
- Line 127: The boolean check " 'interface' in client and client['interface'] ==
True " should be replaced with a more idiomatic lookup using
client.get('interface') (or client.get('interface', False)) when building the
'type' string; update the expression that constructs 'type' (which references
client, client_short_name, and client['type']) to use client.get('interface') so
Ruff E712 is resolved and the logic (empty prefix when interface is truthy, "*"
otherwise) remains the same.
---
Nitpick comments:
In `@lib/resourcebuilder/core.go`:
- Around line 223-226: The List method on apiServerListerAdapter currently
returns (nil, nil) which silently succeeds; change it to return a non-nil error
so callers know List is unsupported — update
apiServerListerAdapter.List(selector labels.Selector) to return an explicit
error (e.g. errors.New or fmt.Errorf) stating that List is not
implemented/unsupported for this adapter (or wrap with context like
"apiServerListerAdapter: List not implemented"), keeping the Get() behavior
untouched.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
hack/generate-lib-resources.pylib/resourcebuilder/core.golib/resourcebuilder/core_test.golib/resourcebuilder/resourcebuilder.golib/resourcebuilder/resourcebuilder_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/resourcebuilder/core_test.go
| 'package': package, | ||
| 'client_short_name': client_short_name, | ||
| 'type': '*{}.{}'.format(client_short_name, client['type']), | ||
| 'type': '{}{}.{}'.format("" if 'interface' in client and client['interface'] == True else "*", client_short_name, client['type']), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the exact content around line 127
echo "=== Content around line 127 ==="
sed -n '125,130p' hack/generate-lib-resources.py
# Verify no explicit equality checks to True/False remain
echo ""
echo "=== Checking for True/False equality comparisons ==="
rg -nP '==\s*True|==\s*False' hack/generate-lib-resources.pyRepository: 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. Use client.get('interface') instead for cleaner, idiomatic Python.
Proposed fix
- 'type': '{}{}.{}'.format("" if 'interface' in client and client['interface'] == True else "*", client_short_name, client['type']),
+ 'type': '{}{}.{}'.format("" if client.get('interface') else "*", client_short_name, client['type']),🧰 Tools
🪛 Ruff (0.15.2)
[error] 127-127: Avoid equality comparisons to True; use client['interface']: for truth checks
Replace with client['interface']
(E712)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/generate-lib-resources.py` at line 127, The boolean check " 'interface'
in client and client['interface'] == True " should be replaced with a more
idiomatic lookup using client.get('interface') (or client.get('interface',
False)) when building the 'type' string; update the expression that constructs
'type' (which references client, client_short_name, and client['type']) to use
client.get('interface') so Ruff E712 is resolved and the logic (empty prefix
when interface is truthy, "*" otherwise) remains the same.
Contributor
|
@ingvagabund: The following tests 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. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
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.Operators with the tls-inject annotation:
How to test it:
oc edit apiserver cluster):tlsSecurityProfilefieldSummary by CodeRabbit
New Features
Tests
Chores