🐛clusterctl discovery should ignore provider's resources #5684
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/test pull-cluster-api-e2e-main |
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
k8s-ci-robot
added
the
cncf-cla: yes
|
/retest |
sbueringer
reviewed
Nov 17, 2021
| @@ -237,27 +242,26 @@ func (k *proxy) ListResources(labels map[string]string, namespaces ...string) ([ | |||
| return nil, errors.Wrap(err, "failed to list api resources") | |||
| } | |||
|
|
|||
| // If labels indicates that resources of a specific provider should be listed, exclude CRDs of other providers. | |||
| // Exclude from discovery the objects from the cert-manager/provider's CRDs. | |||
There was a problem hiding this comment.
To confirm my understanding.
We're calling this func in two cases:
- certManagerClient.EnsureLatestVersion
- in this case we're not listing any CRDs
- providerComponents.Delete
- in this case we're not listing any provider CRDs, i.e. we still list Certificate and CertificateRequest (which is what we want)
If I'm correct, maybe we should adjust the godocs of the func slightly, as they could also be interpreted differently (i.e. that the func is only not returning resources of the CRDs of the current component)
There was a problem hiding this comment.
~ right
In both cases we are not listing resources for any CRDs installed by clusterctl (including cert-manager CRDs and providers CRDs).
In other words, we are only concerned about provider components (deployments, RBAC rules, CRD definitions) but not in the resource created on top of that (Clusters, Machines, AWSCluster etc) because the latter are not relevant for upgrade/delete.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
k8s-ci-robot
added
the
approved
|
/cherry-pick release-1.0 |
|
@fabriziopandini: #5684 failed to apply on top of branch "release-1.0": In response to this:
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. |
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 20, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) Reason for backporting: This issue that was solved in db5b183 on the main branch is also effecting older releases of CAPI currently in use thus the patch is applicable on older branches.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 20, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) Reason for backporting: This issue that was solved in db5b183 on the main branch is also effecting older releases of CAPI currently in use thus the patch is applicable on older branches.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 20, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issue faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 22, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issues faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
fabriziopandini
deleted the
clusterctl-discovery-ignore-providers-resources
branch
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 24, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issues faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 24, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issue faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 24, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issue faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 24, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issues faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
Rozzii
pushed a commit
to Nordix/cluster-api
that referenced
this pull request
Nov 24, 2021
This patch was originally introduced in PR kubernetes-sigs#5684. Original name: "clusterctl discovery should ignore provider's resources" Original commit id: db5b183 Original description: While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component. This commit makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself. This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors) This commit also introduces some logic originally implemented in commit f5a9d76 that implements the ability to skip excluded CRD during resource listing. Reason for backporting: The issues that were solved by commit db5b183 and f5a9d76 on the main branch are also effecting older releases of CAPI currently in use thus backporting the "discovery fix" and some related code from f5a9d76 would solve a lot of issues faced by users e.g related to upgrade process as mentioned in the original db5b183 commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
While managing components (for cert-manager or providers) clusterctl implements a discovery function to seek for all the objects part of the component.
This PR makes this code to ignore resources for a provider (e.g Cluster for CAPI, AWSCluster for CAPA, Certificates for cert-manager) given that those resources are not part of the component itself.
This will make operations like upgrade plan or apply and delete resilient to actual state of cert-manager web hooks; in fact, those operations can now work when web-hooks are not functioning (due to provider's deployment already deleted, to provider scaled down to 0, to other errors)