Skip to content

fix: Make Cluster the owner of image registry credential secret #648

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 14, 2024
Merged

fix: Make Cluster the owner of image registry credential secret #648

merged 1 commit into from
May 14, 2024

Conversation

dlipovetsky
Copy link
Contributor

What problem does this PR solve?:
Makes the Cluster resource the owner of image registry credential Secret. CAREN creates this Secret for each Cluster, and it should be deleted when the Cluster is deleted.

Which issue(s) this PR fixes:
Fixes https://jira.nutanix.com/browse/D2IQ-100572

How Has This Been Tested?:

All test cases pass. The test is updated to create a Cluster resource, and to provide the handler a client that can read a Cluster resource.

Special notes for your reviewer:

@dlipovetsky dlipovetsky requested review from supershal and dkoshkin May 9, 2024 22:23
@github-actions github-actions bot added the fix label May 9, 2024
@dlipovetsky dlipovetsky requested a review from deepakm-ntnx May 9, 2024 22:33
@jimmidyson
Copy link
Member

Are there any changes to e2e needed for this too? eg ownership checking of secrets in https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/blob/main/test/e2e/ownerreference_helpers.go#L162 is currently disabled

@dlipovetsky
Copy link
Contributor Author

Are there any changes to e2e needed for this too? eg ownership checking of secrets in https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/blob/main/test/e2e/ownerreference_helpers.go#L162 is currently disabled

Good call-out. I'm investigating this, and will address it in a separate PR.

dlipovetsky added a commit to dlipovetsky/capi-runtime-extensions that referenced this pull request May 10, 2024
@dlipovetsky
test: Use namespace constant
2824277 
Thanks to @supershal, who spotted a similar issue in PR nutanix-cloud-native#648.
dlipovetsky added a commit to dlipovetsky/capi-runtime-extensions that referenced this pull request May 10, 2024
@dlipovetsky
test: Match cluster namespace to cluster name
a2bc047 
Thanks to @supershal, who spotted a similar issue in PR nutanix-cloud-native#648.
@dlipovetsky dlipovetsky mentioned this pull request May 10, 2024
Merged
dlipovetsky added a commit that referenced this pull request May 13, 2024
@dlipovetsky
test: Match cluster namespace to cluster name (#652)
22bf205 
**What problem does this PR solve?**:
Matches cluster namespace to cluster name. Thanks to @supershal, who
spotted a similar issue in PR #648.

**Which issue(s) this PR fixes**:
Fixes #

**How Has This Been Tested?**:
<!--
Please describe the tests that you ran to verify your changes.
Provide output from the tests and any manual steps needed to replicate
the tests.
-->

**Special notes for your reviewer**:
<!--
Use this to provide any additional information to the reviewers.
This may include:
- Best way to review the PR.
- Where the author wants the most review attention on.
- etc.
-->
@dlipovetsky
Copy link
Contributor Author

The refactors became rather complicated, and I would like to take them up in a separate PR. With that in mind, I'm force-pushing a minimal set of changes, just enough to correctly set the owner ref, and ensure the test verifies it.

Thank you @jimmidyson and @supershal. I'll make sure to address your comments w.r.t the refactors in the separate PR.

@dlipovetsky
fix: Make Cluster the owner of image registry credential Secret
8a3799a 
This Secret is specific to the Cluster, and should be deleted together
with the Cluster.
@dlipovetsky dlipovetsky force-pushed the set-ownership-secrets branch from bce3480 to 8a3799a Compare May 13, 2024 22:57
@dlipovetsky dlipovetsky enabled auto-merge (squash) May 13, 2024 23:39
@dlipovetsky dlipovetsky merged commit f680979 into nutanix-cloud-native:main May 14, 2024
16 checks passed
@github-actions github-actions bot mentioned this pull request May 14, 2024
Merged
jimmidyson added a commit that referenced this pull request May 21, 2024
@jimmidyson @github-actions
release(main): v0.9.0 (#597)
0ce44c2 
🤖 I have created a release *beep* *boop*
---


## 0.9.0 (2024-05-21)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Exciting New Features 🎉
* feat: expose GenerateNoProxy func by @mhrabovcin in
#594
* feat: Add the ServiceLoadbalancer Addon, with MetalLB as first
provider by @dlipovetsky in
#592
* feat: adds GPU mutation by @faiq in
#591
* feat: Add GenericClusterConfig and add docs on usage with own CC by
@jimmidyson in
#606
* feat: Enable unprivileged ports sysctl in containerd config by
@jimmidyson in
#645
* feat: API for encryption at-rest by @supershal in
#610
* feat: Bump sigs.k8s.io/cluster-api to v1.7.2 by @jimmidyson in
#661
* feat: Pull calico images from quay.io instead of docker hub by
@jimmidyson in
#676
* feat: update cluster autoscaler to v1.30.0 by @dkoshkin in
#681
### Fixes 🔧
* fix: Fix error messages returned by HelmChartGetter by @dlipovetsky in
#598
* fix: use a consistent MachineDeployment class name by @dkoshkin in
#612
* fix: Do not return error if serviceLoadBalancer field is not set by
@dlipovetsky in
#611
* fix: use provided options for serverside apply by @supershal in
#627
* fix: Correct the CSI handler logic by @dlipovetsky in
#603
* fix: Fix the internal ClusterConfig type used for provider-agnostic
logic by @jimmidyson in
#607
* fix: log mutation failure errors by @supershal in
#649
* fix: Always apply containerd patches by @jimmidyson in
#644
* fix: cluster-autoscaler Helm values for workload clusters by @dkoshkin
in
#658
* fix: Make Cluster the owner of image registry credential secret by
@dlipovetsky in
#648
* fix: Upgrade dynamic-credential-provider to v0.5.3 by @jimmidyson in
#677
### Other Changes
* build: Add v0.8 release metadata by @jimmidyson in
#595
* refactor: Clean up API constants, and explain usage by @dlipovetsky in
#588
* docs: Add how to deploy CAREN by @jimmidyson in
#599
* docs: Upgrade hugo to latest by @jimmidyson in
#601
* docs: Update addons docs and tweak release doc by @jimmidyson in
#596
* build: Ensure provider metadata is up to date when releasing by
@jimmidyson in
#600
* docs: Add how to create clusters by @jimmidyson in
#602
* docs: Update docsy module by @jimmidyson in
#605
* refactor: Apply kubebuilder annotations for required/optional
everywhere by @jimmidyson in
#604
* docs: Cluster Autoscaler is deployed on the management cluster by
@dkoshkin in
#608
* docs: Fix missing placeholder in "create nutanix cluster" doc by
@dlipovetsky in
#609
* refactor: Remove unused api/variables package by @dlipovetsky in
#623
* refactor: move label helper functions to utils package by @supershal
in
#626
* build: Use go1.22.3 toolchain to mitigate vulnerabilties by
@jimmidyson in
#628
* build: Temporary lint config fix until next golangci-lint release by
@jimmidyson in
#629
* build: Update license for Nutanix by @jimmidyson in
#456
* test(e2e): Consistent core/bootstrap/control-plane provider versions
by @jimmidyson in
#639
* ci: free up disk space before running tests by @dkoshkin in
#643
* test: Add more context to panic in envtest helper by @dlipovetsky in
#641
* refactor: Use colon to separate context from wrapped error by
@dlipovetsky in
#642
* refactor: Remove unused test helper function by @dlipovetsky in
#647
* test: Add even more context to panic in envtest helper by @dlipovetsky
in
#650
* build: Make module-relative "go list -m" compatible with GOWORK by
@dlipovetsky in
#651
* test: Match cluster namespace to cluster name by @dlipovetsky in
#652
* refactor: Write configuration under /etc/caren by @dlipovetsky in
#656
* build: use a shorter namespace caren-system by @dkoshkin in
#662
* refactor: Use a Credentials struct consistently by @dlipovetsky in
#663
* test: add encryptionAtRest config in capi-quick-start by @supershal in
#659
* test(e2e): Fix up secret ownership checks by @jimmidyson in
#665
* test: Remove hard-coded text focus and label for e2e tests by
@dlipovetsky in
#667
* ci: Use new dependabot multimodule capabilities by @jimmidyson in
#664
* refactor: aggregate types to be used by clients by @dkoshkin in
#672
* test: Add E2E_DRYRUN and E2E_VERBOSE make vars by @dlipovetsky in
#666
* build: Ignore all gitlint rules for dependabot commits by @jimmidyson
in
#675
* build: Update all tools by @jimmidyson in
#678
* test(e2e): Use upstream CRS helpers by @jimmidyson in
#680
* build: Correct dry-run output by @jimmidyson in
#679
* build: Use k8s v1.29.4 as default Kubernetes version by @jimmidyson in
#646

## New Contributors
* @prajnutanix made their first contribution in
#638

**Full Changelog**:
v0.8.1...v0.9.0

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supershal supershal supershal left review comments

@mhrabovcin mhrabovcin mhrabovcin approved these changes

@jimmidyson jimmidyson jimmidyson approved these changes

@dkoshkin dkoshkin Awaiting requested review from dkoshkin

@deepakm-ntnx deepakm-ntnx Awaiting requested review from deepakm-ntnx

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlipovetsky @jimmidyson @mhrabovcin @supershal