Skip to content
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

feat(controller-runtime): Implement stream controller #211

Merged

Conversation

adriandieter
Copy link

@adriandieter adriandieter commented Dec 11, 2024

Implements reconciliation of stream resources in the stream controller

Adds a base JetstreamController for nats/jetstream functionality shared between controllers, like initializing a jetStream client for the current call.
Adds test suit using envtest and embedded nats servers.
Fixes tests failing due to missing k8s binaries.

Controllers and tests are based on files generated by operator-sdk.
Adds a minimal test suite for the controllers with a etcd test env and a test nats jetStream server to test against.
See TODOs on what still needs to be implemented.
Use a single use client on every connection.
This should be replaced by a client pool in the future.
Setting  CAs: []string{*ca} resulted in  []string{""} when no CA was set, leading to an error when creating clients.
- Trigger only on generation changes
- Split initialization and create into separate calls to Reconcile
Instead check for preventUpdate. Introduced during refactor.
Prefix with stream to prevent conflict with other resources.
Copy link
Member

@samuelattwood samuelattwood left a comment

Choose a reason for hiding this comment

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

Some very minor comments, but overall looks really good!

We can go ahead and merge this into the feature branch, I'm going to run some tests on my clusters with the new controllers.

internal/controller/jetstream_controller.go Outdated Show resolved Hide resolved
internal/controller/jetstream_controller.go Outdated Show resolved Hide resolved
internal/controller/stream_controller.go Outdated Show resolved Hide resolved

// Takes opts values if present
cfg := &NatsConfig{
CRDConnect: false,
Copy link
Author

Choose a reason for hiding this comment

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

I just noticed that CRDConnect is always false for the clients.

Do you think we need something like this for setting the value according to base config and opts?

Set CRDConnect to the base config value, but set it to false if spec config should overwrite it:

cfg.CRDConnect = c.config.CRDConnect
if opts.Creds != "" ||
	opts.Nkey != "" ||
	len(opts.TLS.RootCAs) > 0 ||
	opts.TLS.ClientCert != "" ||
	opts.TLS.ClientKey != "" {

	cfg.CRDConnect = false
}

@samuelattwood samuelattwood merged commit 0f218f5 into nats-io:feature/controller-runtime Dec 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants