fix: execution fails when istio CRD is not installed in the cluster #144

YTGhost · 2024-03-20T07:32:04Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

Enforce to specify --providers, and not default to all providers
Fail soft with a warning if the provider specified is not the provider whose CRDs are not installed

Which issue(s) this PR fixes:

Fixes #138

Does this PR introduce a user-facing change?:

k8s-ci-robot · 2024-03-20T07:32:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: YTGhost
Once this PR has been reviewed and has the lgtm label, please assign youngnick for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2024-03-20T07:32:13Z

Hi @YTGhost. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

YTGhost · 2024-03-20T07:34:19Z

@dpasiukevich @LiorLieberman PTAL

cmd/print.go

Signed-off-by: Liang Deng <[email protected]>

LiorLieberman

Thanks, left some comments.
A general comment, you made two proposed solutions to the same problem. if we change the code to fail soft we might not need to enforce specifying providers.

LiorLieberman · 2024-03-20T08:38:23Z

cmd/print.go

@@ -245,7 +248,7 @@ func newPrintCommand() *cobra.Command {
 		`If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even
 if specified with --namespace.`)

-	cmd.Flags().StringSliceVar(&pr.providers, "providers", i2gw.GetSupportedProviders(),
+	cmd.Flags().StringSliceVar(&pr.providers, "providers", nil,


maybe an empty list instead of nil?
Ca you check if we get the desired functionality with this and marking the flag required (see my previous comment)

LiorLieberman · 2024-03-20T13:25:52Z

pkg/i2gw/providers/istio/resource_reader.go

+	crd := &apiextensionsv1.CustomResourceDefinition{}
+	if err := r.conf.Client.Get(ctx, types.NamespacedName{Name: CRDName}, crd); err != nil {
+		log.Printf("CRD '%s' is not installed in the cluster, will not continue reading resources", CRDName)
+		return res, nil
+	}
+


Sorry this was not added to the previous review.
The intention was to change the call to read ANY CRD. In this case you only cover Istio Gateway but it is also relevant for VirtualServices and any other provider's CRD that will be added in the future.

So either you change istio calls to "fail soft" or you create a general helper that doing it

mlavacca

I lean toward not requiring any provider when calling the CLI, instead, I propose the fail soft as @LiorLieberman proposed. I think the soft fail logic should be introduced one level abve. If a provider fails to read resources, we might want to just log the error and continue with other providers. We should implement this outside the istio provider as we might have the same issue with other Providers (Kong just implemented TCPIngresses conversion, and I think we have the same issue there).

At the moment, logging messages is problematic because of #145, but that's a separate issue.

levikobi

+1 to what @mlavacca and @LiorLieberman said. I also would not want to see an enforcement of specifying the providers flags, as this would be one more friction point that users have to figure out.

Alongside Kong, Apisix also faces this issue now. We need to solve this problem for all the providers (one level above, again, as @mlavacca said).

I think a reasonable solution is to have each provider check if his CRDs are installed; if they're not, return a typed error that specifies that. This error will result in a "fail soft" behavior one level above.

At the end of the run, we can specify which providers participated in the translation instead of describing which didn't. I think that's a better choice since a naive user who has some manifests but doesn't specify providers shouldn't receive multiple error messages about missing CRDs from different providers (which he might not even be familiar with).

LiorLieberman · 2024-03-27T11:21:42Z

@YTGhost This is a very important fix to add.
Do you still have the bandwidth to work on this?

levikobi · 2024-03-27T11:36:28Z

This is a very important fix to add.

+1

YTGhost · 2024-03-27T12:04:14Z

@YTGhost This is a very important fix to add. Do you still have the bandwidth to work on this?

@LiorLieberman Sorry for the delay, I will fix it as soon as possible.

And let me confirm what we need to do now:

Not enforcing the specification of --providers
Implement a general helper that checks whether the list of CRDs passed to it are installed in the cluster. If not installed, it will return a typed error. For this typed error, we will use a soft fail logic to handle it at a higher level.
Introduce this helper to check each current provider.

mlavacca · 2024-03-29T15:18:02Z

@YTGhost This is a very important fix to add. Do you still have the bandwidth to work on this?

@LiorLieberman Sorry for the delay, I will fix it as soon as possible.

And let me confirm what we need to do now:

Not enforcing the specification of --providers

👍

Implement a general helper that checks whether the list of CRDs passed to it are installed in the cluster. If not installed, it will return a typed error. For this typed error, we will use a soft fail logic to handle it at a higher level.

I don't think we need a helper to check the CRDs are installed in the cluster. This is the flow I have in mind:

The providers' readResourcesFromCluster functions returns a typed error when the CRD is not installed in the cluster (instead of a general error)
the caller of readResourcesFromCluster checks the error type. If it is related to missing CRDs, we add the provider to the list of the ones who did not provide translation, as @levikobi suggested.

LiorLieberman · 2024-04-03T09:55:18Z

@YTGhost can we help to speed this up?
It is also ok if you have had limited bandwidth recently, just that this feature is really important so if you did, someone else would implement this.

YTGhost · 2024-04-10T07:22:17Z

@YTGhost can we help to speed this up? It is also ok if you have had limited bandwidth recently, just that this feature is really important so if you did, someone else would implement this.

Yes, sorry, I've been busy with some school matters recently, and don't have enough bandwidth. It would be great if someone could help implement it. Apologies once again.

mlavacca · 2024-04-15T09:12:05Z

closing as superseded by #153

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 20, 2024

k8s-ci-robot requested review from levikobi and mlavacca March 20, 2024 07:32

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 20, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 20, 2024

YTGhost force-pushed the fix-istio-provider branch from c9b11fd to 6210c15 Compare March 20, 2024 07:33

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 20, 2024

LiorLieberman reviewed Mar 20, 2024

View reviewed changes

cmd/print.go Outdated Show resolved Hide resolved

fix: execution fails when istio CRD is not installed in the cluster

050dcb8

Signed-off-by: Liang Deng <[email protected]>

YTGhost force-pushed the fix-istio-provider branch from 6210c15 to 050dcb8 Compare March 20, 2024 08:41

LiorLieberman reviewed Mar 20, 2024

View reviewed changes

mlavacca reviewed Mar 21, 2024

View reviewed changes

levikobi reviewed Mar 27, 2024

View reviewed changes

mlavacca mentioned this pull request Apr 10, 2024

fail soft when crds are not installed #153

Merged

mlavacca closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: execution fails when istio CRD is not installed in the cluster #144

fix: execution fails when istio CRD is not installed in the cluster #144

YTGhost commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

YTGhost commented Mar 20, 2024

LiorLieberman left a comment

LiorLieberman Mar 20, 2024

LiorLieberman Mar 20, 2024

mlavacca left a comment

levikobi left a comment

LiorLieberman commented Mar 27, 2024

levikobi commented Mar 27, 2024

YTGhost commented Mar 27, 2024 •

edited

Loading

mlavacca commented Mar 29, 2024

LiorLieberman commented Apr 3, 2024

YTGhost commented Apr 10, 2024

mlavacca commented Apr 15, 2024

fix: execution fails when istio CRD is not installed in the cluster #144

fix: execution fails when istio CRD is not installed in the cluster #144

Conversation

YTGhost commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

YTGhost commented Mar 20, 2024

LiorLieberman left a comment

Choose a reason for hiding this comment

LiorLieberman Mar 20, 2024

Choose a reason for hiding this comment

LiorLieberman Mar 20, 2024

Choose a reason for hiding this comment

mlavacca left a comment

Choose a reason for hiding this comment

levikobi left a comment

Choose a reason for hiding this comment

LiorLieberman commented Mar 27, 2024

levikobi commented Mar 27, 2024

YTGhost commented Mar 27, 2024 • edited Loading

mlavacca commented Mar 29, 2024

LiorLieberman commented Apr 3, 2024

YTGhost commented Apr 10, 2024

mlavacca commented Apr 15, 2024

YTGhost commented Mar 27, 2024 •

edited

Loading