- 
                Notifications
    
You must be signed in to change notification settings  - Fork 11
 
ESO-182: Updates to use different TLS Secret name and disables cluster resource reconcile based on user config #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0714206
              a8c7f31
              94f8165
              bc10341
              84b4046
              ca2c73f
              d82cfc0
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -156,7 +156,11 @@ func main() { | |
| metricsServerOptions.KeyName = metricsKeyFileName | ||
| } | ||
| metricsTLSOpts = append(metricsTLSOpts, func(c *tls.Config) { | ||
| certPool := x509.NewCertPool() | ||
| certPool, err := x509.SystemCertPool() | ||
| if err != nil { | ||
| setupLog.Info("unable to load system certificate pool", "error", err) | ||
| certPool = x509.NewCertPool() | ||
| } | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [claude-generated] Critical: Hard exit on SystemCertPool() failure could break deployments. Consider adding a fallback: certPool, err := x509.SystemCertPool()
if err != nil {
    setupLog.Info("system cert pool unavailable, using empty pool", "error", err)
    certPool = x509.NewCertPool()
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. like claude mentioned above, is it really a fatal error if SystemCertPool() cannot be fetched that we need to do a hard exit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel yes, atleast in RHEL which is our base image, the system CA certificates will always be present, and if at all failure occurs it would be a genuine failure and we don't want proceed I think.  | 
||
| openshiftCACert, err := os.ReadFile(openshiftCACertificateFile) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to read OpenShift CA certificate") | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -119,11 +119,8 @@ spec: | |
| initialDelaySeconds: 5 | ||
| periodSeconds: 10 | ||
| resources: | ||
| limits: | ||
| cpu: 500m | ||
| memory: 128Mi | ||
| requests: | ||
| cpu: 10m | ||
| memory: 64Mi | ||
| cpu: 100m | ||
| memory: 1Gi | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [claude-generated] Critical: CPU request increased from 10m to 100m (10x) and memory from 64Mi to 1Gi (16x). Please provide: 
 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inline with above claude comment, curious to know 1Gi memory need? Most of it is cache or is there any other significant use of memory in this operator? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was decided based on the observation added in this comment. With just one additional day-2 operator, the memory reached 512Mib when the operator started. And I think it's safe to keep 1Gi considering the large clusters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i have not seen the memory go beyond 62MiB for the operator POD.. i started a  Also, we already had limit as 128Mi which went through QA testing. But even with 64Mi limit there are no OOMKilled.  | 
||
| serviceAccountName: controller-manager | ||
| terminationGracePeriodSeconds: 10 | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -48,6 +48,10 @@ const ( | |||||||||
| // externalsecretsDefaultNamespace is the namespace where the `external-secrets` operand required resources | ||||||||||
| // will be created, when ExternalSecretsConfig.Spec.Namespace is not set. | ||||||||||
| externalsecretsDefaultNamespace = "external-secrets" | ||||||||||
| 
     | 
||||||||||
| // certmanagerTLSSecretWebhook is the TLS secret created by cert-manager for the webhook component. A different | ||||||||||
| // name is used to avoiding clash with the secret created by the inbuilt cert-controller component. | ||||||||||
| 
         
      Comment on lines
    
      +52
     to 
      +53
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix grammar in comment. The comment contains a grammatical error. Apply this diff: -	// certmanagerTLSSecretWebhook is the TLS secret created by cert-manager for the webhook component. A different
-	// name is used to avoiding clash with the secret created by the inbuilt cert-controller component.
+	// certmanagerTLSSecretWebhook is the TLS secret created by cert-manager for the webhook component. A different
+	// name is used to avoid clash with the secret created by the inbuilt cert-controller component.📝 Committable suggestion
 
        Suggested change
       
    
 🤖 Prompt for AI Agents | 
||||||||||
| certmanagerTLSSecretWebhook = "external-secrets-webhook-cm" | ||||||||||
| ) | ||||||||||
| 
     | 
||||||||||
| var ( | ||||||||||
| 
          
            
          
           | 
    ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docker is used instead of$(CONTAINER_TOOL) because, by default $ (CONTAINER_TOOL) is set to podman, and buildx is supported by docker. Hence just here docker is hardcoded.