Skip to content

Conversation

bharath-b-rh
Copy link
Contributor

@bharath-b-rh bharath-b-rh commented Aug 20, 2025

PR has following changes:

  • Revisits the APIs of external-secrets-operator
  • Renames externalsecrets.operator.openshift.io to externalsecretsconfigs.operator.openshift.io based on the user feedback. This improves the UX by removing the confusions with the upstream API externalsecrets.external-secrets.io which has the same name.
  • New params are added for configuration proxy settings.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 20, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 20, 2025

@bharath-b-rh: This pull request references ESO-101 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 story to target the "4.20.0" version, but no target version was set.

In response to this:

PR has following changes:

  • Revisits the APIs of external-secrets-operator
  • Renames externalsecrets.operator.openshift.io to externalsecretsconfigs.operator.openshift.io based on the user feedback. This improves the UX by removing the confusions with the upstream API externalsecrets.external-secrets.io which has the same name.
  • Removes params controllerConfig and externalSecretsConfig in the Spec of externalsecretsconfigs.operator.openshift.io. And all the params part of removed aforementioned params are moved directly under Spec.
  • New params are added for configuration proxy settings.

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.

@bharath-b-rh
Copy link
Contributor Author

/cc @TrilokGeer @mytreya-rh

// lastTransitionTime is the last time the condition transitioned from one status to another.
// +kubebuilder:validation:Type=string
// +kubebuilder:validation:Format=date-time
LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the timestamp better to be with in the ControllerStatus (or actually within the "Condition") to ensure that user knows which controller status was last modified, and when.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I feel, it indicating the last time status was updated irrespective of the condition it changed would be helpful. WDYT?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so here we would assume the user would look at the overall status on the "ExternalSecretsManager" through the ConditionStatus, and if need be, can go to "ExternalSecretsConfig" (the only controller as of now), to check the metav1.Condition

In future, if there is one more controller, the user checks the status on ExternalSecretsManager, but cannot distinguish which of the controllers status changed last right away.
However, it can be checked on individual Controllers' CR.

i am OK with either approach, as the information exists on controller's CR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, also the status contains the ObservedGeneration, and we can capture time present in Controller CR, here too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last time status was updated irrespective of the condition it changed

For a SRE, to identify which controller updated recently, the first-level information would rely on the ControllerStatus which should contain the time. It'd be of a great value while understanding the controller status information.

status contains the ObservedGeneration

The observedGeneration is best to debug where a controller was able to process the CR updates (spec changes) and the status of that specfic CR reflects the recent information. This is quite an interesting solution that enables better debugging.

It is better to elaborate the usecases that can give better insights on why the changes are being proposed.

@mytreya-rh
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 5, 2025

@bharath-b-rh: This pull request references ESO-101 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 story to target the "4.21.0" version, but no target version was set.

In response to this:

PR has following changes:

  • Revisits the APIs of external-secrets-operator
  • Renames externalsecrets.operator.openshift.io to externalsecretsconfigs.operator.openshift.io based on the user feedback. This improves the UX by removing the confusions with the upstream API externalsecrets.external-secrets.io which has the same name.
  • New params are added for configuration proxy settings.

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.

@emmajiafan
Copy link

@CodeRabbit review

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2025
Comment on lines 583 to 601
type ProxyConfig struct {
// httpProxy is the URL of the proxy for HTTP requests.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=2048
// +kubebuilder:validation:Optional
HTTPProxy string `json:"httpProxy,omitempty"`

// httpsProxy is the URL of the proxy for HTTPS requests.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=2048
// +kubebuilder:validation:Optional
HTTPSProxy string `json:"httpsProxy,omitempty"`

// noProxy is a comma-separated list of hostnames and/or CIDRs and/or IPs for which the proxy should not be used.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=4096
// +kubebuilder:validation:Optional
NoProxy string `json:"noProxy,omitempty"`
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel 2048 for http_proxy, https_proxy and 4096 for noProxy are too high, we can allow 512 and 2048 respectively.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will external-secrets have outgoing traffic to any of the services other than apiserver, and the bitwarden provider within the cluster?
This is where the NoProxy will come in to play right?
If we have analysis on that, maybe we can reduce the size.
But i wonder why the Max value is not specified on the cluster level settings
i feel even if we specify Max limits, its better to be on the higher side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's correct. We also have Vault, CyberArk Conjur, 1Password ... providers which all provide kubernetes native solution.

But i wonder why the Max value is not specified on the cluster level settings

The ask All strings should set a minimum and maximum length is being suggested from recently, and I think hence cluster proxy might have not set it.

// enabled indicates whether the feature is active.
// +kubebuilder:validation:Enum:="true";"false"
// +kubebuilder:validation:Required
Enabled string `json:"enabled,omitempty"`
Copy link

@mytreya-rh mytreya-rh Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both Name, and Enabled are required. But only Enabled has omitempty marshalling. Why is that so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have missed it, let me update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion incorporated.

// +patchStrategy=merge
// +listType=map
// +listMapKey=name
ControllerStatuses []ControllerStatus `json:"controllerStatuses,omitempty"`
Copy link

@mytreya-rh mytreya-rh Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When all of the status is already on the ExternalSecretsConfig CR, why do we want to duplicate a part of it here?
Would like to understand the specific use case or ask behind this status mirroring.
The purpose of this CR is:

externalsecretsmanager.operator.openshift.io is another CR object, which is made available for configuring global options and to enable optional or TechPreview features.

So the main use case is for the individual controllers (as of now, ExternalSecretsConfig) to watch this CR and update the common config.
So, i am wondering do we even need a controller specifically for this (externalsecretsmanager) CR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently operator just has one operand, the externalsecretsmanager itself was introduced for extensibility when new operands are introduced to act as a centralized config and provide a overview of the operands being managed. And for the same reason a subset of condition fields from each operand CR is listed here.

Comment on lines 371 to 374
ExternalSecretsImage string `json:"externalSecretsImage,omitempty"`

// BitwardenSDKServerImage is the name of the image and the tag used for deploying bitwarden-sdk-server.
BitwardenSDKServerImage string `json:"bitwardenSDKServerImage,omitempty"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to store the actual image by reading the deployment, or are we updating the environment variable?
If we are updating the environment variable, i think we should document it because its not a typical status as the user would expect. (For example, there can be confusion if for some reason the actual image is updated out side of operator's controller, but the ExternalSecretsImage consistently shows the ENV_VAR as the image. As opposed to showing the actual image in deployment while the operator reconciles it)
We have documented this But i think we can word it better to be explicit that its not really a status.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status indicates which image is used for the particular deployment. This will be available as ENV var in the operator, and the same will be used to update the respective component's deployment spec. Should the ENV var be updated, the status will also be updated with the new image name.

}

// ObjectReference is a reference to an object with a given name, kind and group.
type ObjectReference struct {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation to not use the type certmanagerv1.ObjectReference and recreate it here?
To specify the MaxLengths or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that and for the CEL validations, which is now set on IssuerRef field.

// ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/
// +listType=atomic
// +kubebuilder:validation:MinItems:=0
// +kubebuilder:validation:MaxItems:=10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure as how many taints production clusters may have.
@TrilokGeer any idea if 10 will be sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number is quite tricky to derive, for example, worst-case, considering the k8s list https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ and the openshift list (3), it hits the maximum of 10. Please note, there is consideration whether a taint can be considered for toleration as it is an approximation. Any update to this value would require a new z release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post a bit of search of differnt type of special hardware nodes available in the kubernetes ecosystem, and comparative analysis of managed providers, 50 seems to be better deterministic value. There is still a catch related to size, where etcd will fail the request if the size exceeds 1.5MB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion incorporated.

Comment on lines 583 to 601
type ProxyConfig struct {
// httpProxy is the URL of the proxy for HTTP requests.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=2048
// +kubebuilder:validation:Optional
HTTPProxy string `json:"httpProxy,omitempty"`

// httpsProxy is the URL of the proxy for HTTPS requests.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=2048
// +kubebuilder:validation:Optional
HTTPSProxy string `json:"httpsProxy,omitempty"`

// noProxy is a comma-separated list of hostnames and/or CIDRs and/or IPs for which the proxy should not be used.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=4096
// +kubebuilder:validation:Optional
NoProxy string `json:"noProxy,omitempty"`
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will external-secrets have outgoing traffic to any of the services other than apiserver, and the bitwarden provider within the cluster?
This is where the NoProxy will come in to play right?
If we have analysis on that, maybe we can reduce the size.
But i wonder why the Max value is not specified on the cluster level settings
i feel even if we specify Max limits, its better to be on the higher side.

Comment on lines 680 to 690
- Enabling the `bitwarden secret manager` plugin is optional, and user can enable or disable it in `externalsecretsconfigs.operator.openshift.io`.
Currently, when the feature is disabled, the following resources created for the plugin are not removed, requiring manual cleanup.
- Certificates
- Deployment
- Service
- ServiceAccount

Cleanup will not cause any functionality degradation for services dependent on the secrets fetched from `bitwarden secret manager`, unless
the secret value has been modified. The applications dependent on the fetched secrets can continue to consume it. The Kubernetes Secret
resources containing the fetched values, or custom resources like SecretStore and ExternalSecrets, will not be deleted; users must
manually clean those up.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should prevent the cleanup of bitwarden resources or even better, its disabling in the ExternalSecretsConfig CR as long as there are SecretStore/ClusterSecretStore referring to the provider.
Or, there maybe chances of unexpected outages due to rotated secrets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it's good to delete the bitwarden resources if a user doesn't want to use the plugin and also from security standpoint where they want to restrict bitwarden usage. And when we have NetworkPolicy in place, and if they disallow bitwarden endpoint, having these bitwarden resources is of no use too. So if the user is disabling the bitwarden plugin, I think it's implicit oen should also cleanup the SecretStore and ExternalSecret using the plugin. We can document this. Please let me know WDYT

// +kubebuilder:validation:Optional
Labels map[string]string `json:"labels,omitempty"`

CommonConfigs `json:",inline,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the significance of the CommonConfigs when compared to the existing implementation? Is the intent to reduce code duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's for reducing the code duplication.

type Feature struct {
Name string `json:"name"`
Enabled bool `json:"enabled"`
// name of the optional feature.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate on the user stories that actually requires this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any optional features currently, but this was added as place holder in TP proposal, when we introduce any new features.

// lastTransitionTime is the last time the condition transitioned from one status to another.
// +kubebuilder:validation:Type=string
// +kubebuilder:validation:Format=date-time
LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last time status was updated irrespective of the condition it changed

For a SRE, to identify which controller updated recently, the first-level information would rely on the ControllerStatus which should contain the time. It'd be of a great value while understanding the controller status information.

status contains the ObservedGeneration

The observedGeneration is best to debug where a controller was able to process the CR updates (spec changes) and the status of that specfic CR reflects the recent information. This is quite an interesting solution that enables better debugging.

It is better to elaborate the usecases that can give better insights on why the changes are being proposed.

// +kubebuilder:validation:Optional
// +mapType=atomic
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
CommonConfigs `json:",inline,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is it possible to rename the CommonConfigs to a better categorization of config like Deployment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the concern is on the UX, CommonConfigs will not be available as a field in CRD, it will be an inline expansion. Please let me know if it needs to be renamed.

// reference is not provided and CertManagerConfig is configured. The key names in secret for certificate
// must be `tls.crt`, for private key must be `tls.key` and for CA certificate key name must be `ca.crt`.
// +kubebuilder:validation:Optional
SecretRef *SecretReference `json:"secretRef,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What'd be the behaviour if the secretReference is updated as a day2 operation? or is day2 operation allowed on this configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, secret is required for the bitwarden plugin, which can be enabled day-2 and the secret too. And this secrets will be used in volumemount when deployment object is created for bitwarden.

Enabled string `json:"enabled,omitempty"`
// +kubebuilder:default:="false"
// +kubebuilder:validation:Required
Enabled string `json:"enabled"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be implied that the presence of the configuration enables this rather than an explicit enabled flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but this could be also used to explicitly disable a feature which is enabled by default.

// ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/
// +listType=atomic
// +kubebuilder:validation:MinItems:=0
// +kubebuilder:validation:MaxItems:=10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number is quite tricky to derive, for example, worst-case, considering the k8s list https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ and the openshift list (3), it hits the maximum of 10. Please note, there is consideration whether a taint can be considered for toleration as it is an approximation. Any update to this value would require a new z release.

// ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/
// +listType=atomic
// +kubebuilder:validation:MinItems:=0
// +kubebuilder:validation:MaxItems:=10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post a bit of search of differnt type of special hardware nodes available in the kubernetes ecosystem, and comparative analysis of managed providers, 50 seems to be better deterministic value. There is still a catch related to size, where etcd will fail the request if the size exceeds 1.5MB.

@emmajiafan
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2025
@bharath-b-rh
Copy link
Contributor Author

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 18, 2025
`external-secrets` in `externalsecretsconfigs.operator.openshift.io`, or they can set common configurations for all operands managed by
the operator in `externalsecretsmanager.operator.openshift.io`.

For proxying HTTPS connections, CA certificates are required for validating the HTTPS proxy server. Operator will create a `ConfigMap` in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be the name of the ConfigMaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any meaningful can be used like openshift-trusted-ca-bundle, it's a implementation choice.

the operator in `externalsecretsmanager.operator.openshift.io`.

For proxying HTTPS connections, CA certificates are required for validating the HTTPS proxy server. Operator will create a `ConfigMap` in
its own and in operand's namespace with a label `config.openshift.io/inject-trusted-cabundle="true"` to leverage OpensShift offered
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Operator won't create the configmap in it's own namespace, rather OLM will do that.

Copy link
Contributor Author

@bharath-b-rh bharath-b-rh Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operator will create the configmap is the proposal at this line. And later it's indicated, OLM bundle can be used.
I will update this to remove operator creating ConfigMap for its own use.

trust bundle which will be made available in operator and operand pods through a volume mount. The CA certificate trust bundle must be made
available at `/etc/pki/tls/certs` as [suggested](https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/crypto/x509/root_linux.go;l=22) in golang.

The `ConfigMap` and the `VolumeMount` configurations for the operator can be part of the OLM bundle manifests and for operand, operator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The OLM and the Operator will always create a blank configmap with the injection label and mount it in the containers, even when the proxy is not enabled, right?

  2. Do OLM and Cluster Network Operator conflict while updating the same configmap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as stated in the previous comment response.

trust bundle which will be made available in operator and operand pods through a volume mount. The CA certificate trust bundle must be made
available at `/etc/pki/tls/certs` as [suggested](https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/crypto/x509/root_linux.go;l=22) in golang.

The `ConfigMap` and the `VolumeMount` configurations for the operator can be part of the OLM bundle manifests and for operand, operator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondring if the Operator needs the VolumeMount (proxy settings) on its own? Or It just has to propagate the proxy settings and CA certificate down to its operand?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to have feature, where the trusted CA bundle can be used for verifying the metrics client too.

available at `/etc/pki/tls/certs` as [suggested](https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/crypto/x509/root_linux.go;l=22) in golang.

The `ConfigMap` and the `VolumeMount` configurations for the operator can be part of the OLM bundle manifests and for operand, operator
must create and watch the `ConfigMap` in operand's namespace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the operator might not need to watch the ConfigMap because the Volume will be updated as and when the ConfigMap updates. Moreover, the Operator should not reconcile the ConfigMap data; it should just create it (if not present in the operand namespace)

https://kubernetes.io/docs/concepts/storage/volumes/#configmap

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operator should watch for the ConfigMap existence, not the content, because content is updated by the CNO, functionality of trusted-ca feature.

// httpProxy is the URL of the proxy for HTTP requests.
// This field can have a maximum of 2048 characters.
// +kubebuilder:validation:MinLength:=0
// +kubebuilder:validation:MaxLength:=2048
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding a note here that there is no length restriction in Clusterwide proxy config: https://github.com/openshift/api/blob/04f99ea228f5158f149fa2b99b53f3ac0f6274c6/config/v1/types_proxy.go#L37-L49

@TrilokGeer
Copy link
Contributor

As discussed over on-call review, bitwarden sdk deployment can adopt a different structure as
plugins:
bitwardensdkmanager:

Based on inputs from @bharath-b-rh about out-of-tree provider plan upstream, it is also worthwhile to not extend the pluggable configurations to the operator and provide flexibility for users to bring provider plugins of their choice.

@mytreya-rh
Copy link

mytreya-rh commented Sep 26, 2025

/lgtm

cc: @TrilokGeer Thanks for the in-call review yesterday. Summarizing the changes for your feedback.

Discussed:

  1. Update to clearly state use cases, goals and non-goals
  2. Validation of various API fields
  3. Provision to configure operator specific PROXY settings, and to also include RHCOS and user specified CA bundles into the operand to be able to work with the custom proxies.
  4. Provision to configure the secretRef from which Bitwarden Certificate can be loaded.
  5. removal of unused fields like OptionalFeature, PurgeStrategy etc.

New:

  1. Addition of a default 5 min periodic reconcile to handle race conditions that occur with gitops like usecases. ESO-203
  2. Provision to configure the periodic reconciliation between 2 mins to 5 Hrs.

We will consider the resource cleanup separately in a later EP.
@bharath-b-rh kindly add if i missed anything in the summary

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2025
Copy link
Contributor

openshift-ci bot commented Sep 26, 2025

@bharath-b-rh: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@mytreya-rh
Copy link

/approve

Copy link
Contributor

openshift-ci bot commented Sep 30, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mytreya-rh
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants