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.

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

Do not return error if secret does not exist #3805

Merged
merged 1 commit into from
Jul 3, 2023
Merged

Do not return error if secret does not exist #3805

merged 1 commit into from
Jul 3, 2023

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Oct 31, 2022 •
edited
Loading

What type of PR is this?
The non existence of the bootstrap secret should not prevent an awsMachine from being deleted

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
This change in behaviour was most likely introduced during the ignition support introduction
https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2271/files#diff-4f8240f4e7ff07dcfa9627342b62772c1fe6ae0943021abe53fbb966db88886e

https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2271/files#diff-0b559bbd149f0e6d54d789235423f66fa8906fdc6ee9c99b9e85db912912011e

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

deleteIgnitionBootstrapDataFromS3 do not return error if secret is not found

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 31, 2022
@k8s-ci-robot k8s-ci-robot added needs-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 31, 2022
@enxebre enxebre mentioned this pull request Oct 31, 2022
Merged
4 tasks
Ankitasw
Ankitasw reviewed Oct 31, 2022
View reviewed changes
pkg/cloud/scope/machine.go Outdated
@@ -275,6 +276,10 @@ func (m *MachineScope) GetRawBootstrapDataWithFormat() ([]byte, string, error) {
secret := &corev1.Secret{}
key := types.NamespacedName{Namespace: m.Namespace(), Name: *m.Machine.Spec.Bootstrap.DataSecretName}
if err := m.client.Get(context.TODO(), key, secret); err != nil {
if apierrors.IsNotFound(err) {
Copy link
Member

Choose a reason for hiding this comment

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

@enxebre is it possible to add a unit test for the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also add a debug log?

Copy link
Member

Choose a reason for hiding this comment

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

@enxebre can these comments be taken care of?

@dlipovetsky
Copy link
Contributor

I agree with the goal of the PR.

However, GetRawBootstrapDataWithFormat is also called by resolveUserData, which is called by createInstance, and I'm sure we expect an error there if the data isn't found.

Moreover, we want to return an error to indicate that the first return parameter should not be used.

@enxebre, what if this function continue to return an error, and any caller that wants to ignore the NotFound error can check for it?

@enxebre
Do not return error if secret does not exist
cbb7032 
The non existence of the bootstrap secret should not prevent an awsMachine from being deleted
@enxebre enxebre force-pushed the fix-awsmachine-deletion branch from ab56ca7 to cbb7032 Compare November 15, 2022 05:41
@enxebre
Copy link
Member Author

enxebre commented Nov 15, 2022

@dlipovetsky update PTAL.

@enxebre
Copy link
Member Author

enxebre commented Nov 23, 2022

cc @Ankitasw

@Ankitasw
Copy link
Member

Ankitasw commented Feb 7, 2023

@enxebre could you please take care of open comment?

@Ankitasw
Copy link
Member

@enxebre would you be able to continue with this PR?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Mar 1, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 30, 2023
@enxebre
Copy link
Member Author

enxebre commented Jun 28, 2023

All comments have been addressed via #3805 (comment)

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 28, 2023
@richardcase
Copy link
Member

/milestone v2.2.0

@k8s-ci-robot k8s-ci-robot added this to the v2.2.0 milestone Jun 30, 2023
@muraee
Copy link
Contributor

muraee commented Jun 30, 2023

/lgtm

@k8s-ci-robot
Copy link
Contributor

@muraee: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Ankitasw
Copy link
Member

Ankitasw commented Jul 3, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Ankitasw
Copy link
Member

Ankitasw commented Jul 3, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023
@k8s-ci-robot k8s-ci-robot merged commit 1a2bd94 into kubernetes-sigs:main Jul 3, 2023
@enxebre
Copy link
Member Author

enxebre commented Jun 25, 2024

/cherry-pick release-2.1.4

@k8s-infra-cherrypick-robot
Copy link

@enxebre: cannot checkout release-2.1.4: error checking out "release-2.1.4": exit status 1 error: pathspec 'release-2.1.4' did not match any file(s) known to git

In response to this:

/cherry-pick release-2.1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 25, 2024
@enxebre
Keep old user data for aws < 4.16
fcae853 
We currently need to keep old userdata secrets in AWS < 4.16 to prevent machineDeployments rollout from failing to delete old Machines.
https://github.com/openshift/hypershift/blob/3efa23b932a59681346a7a432d349cfb6e44b13d/hypershift-operator/controllers/nodepool/nodepool_controller.go#L775-L777
kubernetes-sigs/cluster-api-provider-aws#3805

We regress that behaviour here openshift#3969

This PR fixes that by statically checking the hc release version.
@enxebre enxebre mentioned this pull request Jun 25, 2024
Merged
4 tasks
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 25, 2024
@enxebre
Keep old user data for aws < 4.16
e4bbad8 
We currently need to keep old userdata secrets in AWS < 4.16 to prevent machineDeployments rollout from failing to delete old Machines.
https://github.com/openshift/hypershift/blob/3efa23b932a59681346a7a432d349cfb6e44b13d/hypershift-operator/controllers/nodepool/nodepool_controller.go#L775-L777
kubernetes-sigs/cluster-api-provider-aws#3805

We regress that behaviour here openshift#3969

This PR fixes that by statically checking the hc release version.
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 25, 2024
@enxebre
Keep old user data for aws < 4.16
9e03291 
We currently need to keep old userdata secrets in AWS < 4.16 to prevent machineDeployments rollout from failing to delete old Machines.
https://github.com/openshift/hypershift/blob/3efa23b932a59681346a7a432d349cfb6e44b13d/hypershift-operator/controllers/nodepool/nodepool_controller.go#L775-L777
kubernetes-sigs/cluster-api-provider-aws#3805

We regress that behaviour here openshift#3969

This PR fixes that by statically checking the hc release version.
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 25, 2024
@enxebre
Keep old user data for aws < 4.16
c9986d4 
We currently need to keep old userdata secrets in AWS < 4.16 to prevent machineDeployments rollout from failing to delete old Machines.
https://github.com/openshift/hypershift/blob/3efa23b932a59681346a7a432d349cfb6e44b13d/hypershift-operator/controllers/nodepool/nodepool_controller.go#L775-L777
kubernetes-sigs/cluster-api-provider-aws#3805

We regress that behaviour here openshift#3969

This PR fixes that by statically checking the hc release version.
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 25, 2024
@enxebre
Keep old user data for aws < 4.16
33e245d 
We currently need to keep old userdata secrets in AWS < 4.16 to prevent machineDeployments rollout from failing to delete old Machines.
https://github.com/openshift/hypershift/blob/3efa23b932a59681346a7a432d349cfb6e44b13d/hypershift-operator/controllers/nodepool/nodepool_controller.go#L775-L777
kubernetes-sigs/cluster-api-provider-aws#3805

We regress that behaviour here openshift#3969

This PR fixes that by statically checking the hc release version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ankitasw Ankitasw Ankitasw left review comments

@alexander-demicev alexander-demicev alexander-demicev left review comments

@dlipovetsky dlipovetsky Awaiting requested review from dlipovetsky

@pydctw pydctw Awaiting requested review from pydctw

Assignees

@Ankitasw Ankitasw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.
9 participants
@enxebre @dlipovetsky @Ankitasw @k8s-triage-robot @richardcase @muraee @k8s-ci-robot @k8s-infra-cherrypick-robot @alexander-demicev