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

Fix name override issues #1246

Merged
merged 5 commits into from
Mar 19, 2024
Merged

Fix name override issues #1246

merged 5 commits into from
Mar 19, 2024

Conversation

olim7t
Copy link
Contributor

@olim7t olim7t commented Mar 16, 2024

What this PR does:
1) Handle the case where a DC's metadata.name cannot be used directly as a Kubernetes resource name:

datacenters:
   - metadata:
      name: Data_Center_1

I changed the validation webhook to reject this. I think this is the best solution because we already provide the datacenterName override for cases where the CQL name couldn't be used here. We could sanitize metadata.name when it's not valid, but that would bring even more confusion IMO.

2) Fix the naming of some secondary resources (ConfigMaps, etc.) that were incorrectly using the unsanitized override. For example, given:

datacenters:
  - metadata:
    name: dc1
  datacenterName: Data_Center_1

The operator would generate resource names such as ...-Data_Center_1-metrics-agent-config which is obviously wrong. I've changed those to sanitize the override: ...-data-center-1-metrics-agent-config.

On a related note, I also fixed the metrics agent configMap, which was using the wrong cluster name.

Which issue(s) this PR fixes:
Fixes #1141
Fixes #1138

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

- reject cluster creation if a DC has an invalid name
- always use sanitized DC name override when naming secondary resources
- use cluster name override for metrics agent configMap

Fixes k8ssandra#1138
Fixes k8ssandra#1141
@olim7t olim7t changed the title Reject cluster creation if a DC has an invalid name (fixes #1141) Fix name override issues Mar 18, 2024
@olim7t olim7t marked this pull request as ready for review March 18, 2024 16:53
@olim7t olim7t requested a review from a team as a code owner March 18, 2024 16:53
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 57.43%. Comparing base (8189711) to head (f6c4330).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1246      +/-   ##
==========================================
- Coverage   57.45%   57.43%   -0.02%     
==========================================
  Files         103      103              
  Lines       10794    10801       +7     
==========================================
+ Hits         6202     6204       +2     
- Misses       4057     4060       +3     
- Partials      535      537       +2     
Files Coverage Δ
...pis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go 72.72% <100.00%> (+1.45%) ⬆️
...ollers/k8ssandra/cassandra_telemetry_reconciler.go 63.63% <100.00%> (ø)
controllers/k8ssandra/per_node_config.go 56.75% <100.00%> (ø)
pkg/nodeconfig/generate.go 100.00% <100.00%> (ø)
pkg/reaper/vector.go 63.63% <100.00%> (ø)
pkg/stargate/deployments.go 92.96% <100.00%> (ø)
pkg/stargate/vector.go 63.63% <100.00%> (ø)
pkg/telemetry/vector.go 97.76% <100.00%> (ø)
...elemetry/cassandra_agent/cassandra_agent_config.go 24.24% <75.00%> (+2.36%) ⬆️

... and 4 files with indirect coverage changes

@olim7t
Copy link
Contributor Author

olim7t commented Mar 18, 2024

I don't think the CI issue is related. I didn't touch anything related to backup/restore, and it's also timing out on main.

Copy link
Contributor

@emerkle826 emerkle826 left a comment

Choose a reason for hiding this comment

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

👍

@emerkle826
Copy link
Contributor

@olim7t Need to add a Changelog entry too I guess

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@olim7t olim7t merged commit 9039c02 into k8ssandra:main Mar 19, 2024
61 of 62 checks passed
@olim7t olim7t deleted the sanitize-dc branch March 19, 2024 21:11
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.

DC names are not properly sanitized Metrics agent ConfigMap should use cluster name override
2 participants