Add TLS profile fields to Authorino CR - #335
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughTLS configuration now supports optional minimum and maximum versions and cipher suites for listener and OIDC TLS settings. These values are validated in CRD schemas, copied safely, translated into deployment arguments, and covered by reconciliation tests. ChangesTLS configuration support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthorinoCR
participant buildAuthorinoArgs
participant AuthorinoProcess
AuthorinoCR->>buildAuthorinoArgs: provide TLS versions and cipher suites
buildAuthorinoArgs->>AuthorinoProcess: append TLS command-line flags
AuthorinoProcess->>AuthorinoProcess: retain certificate and key flags
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
- Coverage 57.20% 56.77% -0.43%
==========================================
Files 13 13
Lines 1458 1469 +11
==========================================
Hits 834 834
- Misses 529 537 +8
- Partials 95 98 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/reconcilers/deployment.go`:
- Around line 268-296: The new TLS version and cipher suite arguments in
deployment.go are being appended even when TLS is disabled, unlike the existing
cert path/key flags. Update the argument-building logic in the deployment
reconciliation code so the listener and OIDC TLS min/max version and cipher
suite flags are only added when the same TLS enabled condition used for the cert
path flags passes. Use the existing enabled check around the deployment args
assembly and the relevant FlagTlsMinVersion, FlagTlsMaxVersion,
FlagTlsCipherSuites, FlagOidcTlsMinVersion, FlagOidcTlsMaxVersion, and
FlagOidcTlsCipherSuites symbols to keep behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5d2b3fb-41f5-4728-8cba-12fa77b773e4
📒 Files selected for processing (9)
api/v1beta1/authorino_types.goapi/v1beta1/zz_generated.deepcopy.gobundle/manifests/authorino-operator.clusterserviceversion.yamlbundle/manifests/operator.authorino.kuadrant.io_authorinos.yamlconfig/crd/bases/operator.authorino.kuadrant.io_authorinos.yamlconfig/deploy/manifests.yamlconfig/install/manifests.yamlpkg/reconcilers/consts.gopkg/reconcilers/deployment.go
✅ Files skipped from review due to trivial changes (2)
- api/v1beta1/zz_generated.deepcopy.go
- bundle/manifests/authorino-operator.clusterserviceversion.yaml
| MaxVersion string `json:"maxVersion,omitempty"` | ||
| // TLS cipher suites (IANA names). | ||
| // +optional | ||
| CipherSuites []string `json:"cipherSuites,omitempty"` |
There was a problem hiding this comment.
Are omitempty and "" Enum trying to do opposite things?
With this, we can now have invalid CipherSuites or an odd combination of Min/Max that will crash the Authorino pod. Should we safeguard it (crd rules or operator-side check)?
There was a problem hiding this comment.
omitempty means the field is omitted from JSON when empty, and "" in the Enum allows an empty string to pass validation when the field is present. They work together to allow the field to be optional.
5b08e63 to
fc51985
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/reconcilers/authorino_reconciler_test.go (1)
87-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse Ginkgo/Gomega for BDD-style tests per coding guidelines.
The coding guidelines for
**/*_test.gofiles specify using Ginkgo/Gomega for BDD-style tests. The newly addedTestBuildAuthorinoArgsand its subtests use the standardtestingpackage witht.Runinstead. Consider rewriting these as Gomega assertions within a GinkgoDescribe/Contextblock to comply with repository conventions.As per coding guidelines:
**/*_test.go: Use Ginkgo/Gomega for BDD style tests with envtest from controller-runtime for testing against a minimal Kubernetes API server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/reconcilers/authorino_reconciler_test.go` around lines 87 - 223, The new TestBuildAuthorinoArgs coverage uses the standard testing package with t.Run, but this test file should follow the repository’s Ginkgo/Gomega BDD style. Rewrite the scenarios around buildAuthorinoArgs, hasArg, and getArgValue into Ginkgo Describe/Context/It blocks using Gomega expectations, preserving the same cases for TLS disabled, enabled, nil-enabled defaults, and empty fields omitted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sync-authorino-manifests.yaml:
- Around line 39-42: The change check in the sync-authorino-manifests workflow
only uses git diff, so it can miss newly created untracked artifacts from the
manifest generation step. Update the Check for meaningful changes logic to also
detect untracked files, similar to the verify-manifests guard, and keep the skip
decision false when new files appear. Use the existing check step in
sync-authorino-manifests.yaml as the place to combine tracked-diff and
untracked-file detection so make manifests bundle helm-build always triggers a
PR when it produces new artifacts.
---
Nitpick comments:
In `@pkg/reconcilers/authorino_reconciler_test.go`:
- Around line 87-223: The new TestBuildAuthorinoArgs coverage uses the standard
testing package with t.Run, but this test file should follow the repository’s
Ginkgo/Gomega BDD style. Rewrite the scenarios around buildAuthorinoArgs,
hasArg, and getArgValue into Ginkgo Describe/Context/It blocks using Gomega
expectations, preserving the same cases for TLS disabled, enabled, nil-enabled
defaults, and empty fields omitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5cab56a-13f7-4ad9-989f-5c676a5f0470
📒 Files selected for processing (13)
.github/workflows/sync-authorino-manifests.yamlapi/v1beta1/authorino_types.goapi/v1beta1/zz_generated.deepcopy.gobundle/manifests/authorino-operator.clusterserviceversion.yamlbundle/manifests/authorino.kuadrant.io_authconfigs.yamlbundle/manifests/operator.authorino.kuadrant.io_authorinos.yamlcharts/authorino-operator/templates/manifests.yamlconfig/crd/bases/operator.authorino.kuadrant.io_authorinos.yamlconfig/deploy/manifests.yamlconfig/install/manifests.yamlpkg/reconcilers/authorino_reconciler_test.gopkg/reconcilers/consts.gopkg/reconcilers/deployment.go
✅ Files skipped from review due to trivial changes (4)
- bundle/manifests/authorino-operator.clusterserviceversion.yaml
- api/v1beta1/zz_generated.deepcopy.go
- config/crd/bases/operator.authorino.kuadrant.io_authorinos.yaml
- pkg/reconcilers/consts.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1beta1/authorino_types.go
- config/install/manifests.yaml
- bundle/manifests/operator.authorino.kuadrant.io_authorinos.yaml
- pkg/reconcilers/deployment.go
d338855 to
c06198f
Compare
guicassolato
left a comment
guicassolato
left a comment
There was a problem hiding this comment.
Missing the Helm charts. Can you please run make manifests bundle helm-build – helm-build being the part to care about?
Move TLS version and cipher suite configuration into the existing per-listener Tls struct (Listener.Tls and OIDCServer.Tls) instead of top-level AuthorinoSpec fields. The authorino-operator translates these into per-listener CLI flags (--tls-min-version, --oidc-tls-min-version, etc.) only when TLS is enabled for that listener. Includes unit tests for buildAuthorinoArgs covering TLS enabled, disabled, nil-defaults-to-enabled, and empty-values-omitted cases. Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
c06198f to
e338dd0
Compare
that's done, thanks @guicassolato |
4 participants
blocked by: Kuadrant/authorino#647
Summary
tlsMinVersionandtlsCipherSuitesfields toAuthorinoSpec--tls-min-versionand--tls-cipher-suitesCLI flags to the Authorino containerContext
Part of OCP5 TLS profile compliance. The kuadrant-operator sets these fields on the Authorino CR after reading the cluster's TLS security profile.
Depends on: Kuadrant/authorino#647
Test plan
buildAuthorinoArgs()includes flags when fields are setSummary by CodeRabbit
New Features
Tests