Skip to content

Update coroot operator with new TLS Support configs#44

Merged
def merged 6 commits intocoroot:mainfrom
vishnukumarkvs:add-tls-support
May 1, 2026
Merged

Update coroot operator with new TLS Support configs#44
def merged 6 commits intocoroot:mainfrom
vishnukumarkvs:add-tls-support

Conversation

@vishnukumarkvs
Copy link
Copy Markdown
Contributor

@vishnukumarkvs vishnukumarkvs commented Apr 20, 2026

Adding TLS configurations to coroot operator which we already support now for coroot components in latest releases

@vishnukumarkvs vishnukumarkvs marked this pull request as draft April 20, 2026 18:14
@vishnukumarkvs
Copy link
Copy Markdown
Contributor Author

vishnukumarkvs commented Apr 22, 2026

Bumped controller-gen to 0.20.1 (latest) in Makefile as 0.16.1 version is not compatible with go 1.25 version

make generate
Downloading sigs.k8s.io/controller-tools/cmd/controller-gen@v0.16.1
# golang.org/x/tools/internal/tokeninternal
../../../../go/pkg/mod/golang.org/x/tools@v0.24.0/internal/tokeninternal/tokeninternal.go:64:9: invalid array length -delta * delta (constant -256 of type int64)
make: *** [/Users/kvsvishnukumar/VishnuKvs/Workspace/myrepos/coroot-operator/bin/controller-gen] Error 1

Not facing above issue with latest version

vishnukumarkvs
vishnukumarkvs commented Apr 24, 2026
View reviewed changes
Comment thread config/samples/coroot_tls.yaml
Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 24, 2026

Choose a reason for hiding this comment

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

I ran k3s in ubuntu vm. I have used openssl and created certs and keys. Used this custom resource and deployed coroot to start in TLS mode

I ran make run to test this operator code

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 24, 2026

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 24, 2026

Choose a reason for hiding this comment

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

cc: @def

@vishnukumarkvs
Copy link
Copy Markdown
Contributor Author

vishnukumarkvs commented Apr 24, 2026

Hi @def ,

I have tested these changes and it looks good. Pls review this MR.

Also, I need your input for below things.

  1. When deploying via operator, I used 8443 as default port for https. We have two env vars. HTTPS_LISTEN and HTTP_DISABLED. In coroot CR, if we just pass httpDisabled: true, it will use 8443 (cr.Spec.Service.HTTPSPort = 8443) and will set env var HTTPS_LISTEN to ":8443". Is this ok?

  2. When in HTTP_DISABLED mode, the below entry in coroot k8s service is useless

  ports:
  - name: http
    port: 8080
    protocol: TCP
    targetPort: http

So, should I add code to remove this or shall we keep it for fallback?

@vishnukumarkvs vishnukumarkvs marked this pull request as ready for review April 24, 2026 19:47
def
def requested changes Apr 24, 2026
View reviewed changes
Comment thread api/v1/coroot_types.go Outdated
// Whether to skip verification of the Coroot server's TLS certificate.
TLSSkipVerify bool `json:"tlsSkipVerify,omitempty"`
// Secret containing the CA certificate to verify the Coroot server's certificate.
CASecret *corev1.SecretKeySelector `json:"caSecret,omitempty"`
Copy link
Copy Markdown
Member

@def def Apr 24, 2026

Choose a reason for hiding this comment

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

I feels like we need to configure CASecret in NodeAgentSpec only. I was a wrong decision to configure TLSSkipVerify right here (let's keep it here for back compatibility)

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 26, 2026

Choose a reason for hiding this comment

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

updated here: 7d7589b

Comment thread controller/cluster_agent.go Outdated
var caSecret *corev1.SecretKeySelector
if cr.Spec.AgentsOnly != nil && cr.Spec.AgentsOnly.CorootURL != "" {
corootURL = cr.Spec.AgentsOnly.CorootURL
tlsSkipVerify = cr.Spec.AgentsOnly.TLSSkipVerify
Copy link
Copy Markdown
Member

@def def Apr 24, 2026

Choose a reason for hiding this comment

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

let's set it to cr.Spec.AgentsOnly.TLSSkipVerify or cr.Spec.NodeAgent.TLS.TLSSkipVerify (if TLS != nil)

Comment thread controller/cluster_agent.go Outdated
if cr.Spec.AgentsOnly != nil && cr.Spec.AgentsOnly.CorootURL != "" {
corootURL = cr.Spec.AgentsOnly.CorootURL
tlsSkipVerify = cr.Spec.AgentsOnly.TLSSkipVerify
caSecret = cr.Spec.AgentsOnly.CASecret
Copy link
Copy Markdown
Member

@def def Apr 24, 2026

Choose a reason for hiding this comment

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

this should be set only from cr.Spec.NodeAgent.TLS

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 25, 2026

Choose a reason for hiding this comment

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

I think using NodeAgent TLS config inside ClusterAgent would be confusing. This will remove the redundancy and we can just add one CA Secret in NodeAgent Spec but wont it confuse?

Copy link
Copy Markdown
Member

@def def Apr 25, 2026

Choose a reason for hiding this comment

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

Surely we need to keep both: one for NodeAgent, another for ClusterAgent. I meant AgentsOnly should contain only the Coroot URL. Other agent configuration (such as resource requests/limits) should come from NodeAgent and ClusterAgent, so it would be good to have TLS configs in those sections as well.

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 26, 2026

Choose a reason for hiding this comment

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

yes, updated in this commit: 7d7589b

Comment thread controller/coroot.go Outdated
if cr.Spec.Service.HTTPSPort != 0 {
env = append(env, corev1.EnvVar{Name: "HTTPS_LISTEN", Value: httpsListen})
_, port, _ := strings.Cut(httpsListen, ":")
p, _ := resource.ParseQuantity(port)
Copy link
Copy Markdown
Member

@def def Apr 24, 2026

Choose a reason for hiding this comment

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

i'm not sure that using resource.ParseQuantity is necessary

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 25, 2026

Choose a reason for hiding this comment

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

Used strconv, here: dd340c8

Comment thread controller/node_agent.go Outdated
Comment on lines +37 to +38
tlsSkipVerify = cr.Spec.AgentsOnly.TLSSkipVerify
caSecret = cr.Spec.AgentsOnly.CASecret
Copy link
Copy Markdown
Member

@def def Apr 24, 2026

Choose a reason for hiding this comment

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

the same as for ClusterAgent

Comment thread controller/coroot.go Outdated
Comment on lines 338 to 346
Copy link
Copy Markdown
Member

@def def Apr 24, 2026

Choose a reason for hiding this comment

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

Let's add this only if !cr.Spec.HTTPDisabled ?

Copy link
Copy Markdown
Contributor Author

@vishnukumarkvs vishnukumarkvs Apr 25, 2026

Choose a reason for hiding this comment

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

Added in b9ad4a0 with other related ingress changes

@vishnukumarkvs vishnukumarkvs requested a review from def May 1, 2026 05:21
@vishnukumarkvs
Copy link
Copy Markdown
Contributor Author

vishnukumarkvs commented May 1, 2026

Hi @def , let me know if any other changes are required.

@def
Copy link
Copy Markdown
Member

def commented May 1, 2026

@vishnukumarkvs thank you, and apologies for the delay on my end

@def def merged commit 0341002 into coroot:main May 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@def def def approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vishnukumarkvs @def