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

errors.IsUnauthorized(err) no longer works with controller-runtime 0.15.0 #2513

Closed
bobdonat opened this issue Sep 25, 2023 · 12 comments
Closed
Labels
kind/support Categorizes issue or PR as a support question. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@bobdonat
Copy link

bobdonat commented Sep 25, 2023

After calling client.Get we are checking for Unauthorized using errors.IsUnauthorized(err). This no longer works with controller-runtime 0.15.0. err is a wrapped error now which seems to be the root of the problem. errors is imported from k8s.io/apimachinery/pkg/api/errors. Is this expected behavior? I did not see any mention of this breakage in the release notes.

@bobdonat
Copy link
Author

Will we see the same behavior with IsForbidden, IsConflict, etc.?

@bobdonat
Copy link
Author

bobdonat commented Sep 25, 2023

We have this code which handled the IsUnauthorized case with controller-runtime 0.13.2 but does not with controller-runtime 0.15.0. This code used to bypass the if block but now does not with 0.15.0 when unauthorized.

	err := s.AdminClient.Get(s.Context, vmcName, &vmc)
	if client.IgnoreNotFound(err) != nil && !apierrors.IsUnauthorized(err)) {
		s.Log.Errorf("Failed to get the VMC resources %s/%s from the admin cluster: %v", constants.VerrazzanoMultiClusterNamespace, s.ManagedClusterName, err)
		return false, err
	}

@troy0820
Copy link
Member

/kind support

  1. This was discussed in the kubernetes slack indicating that there should not be errors that were being received from the cache.
  2. The error is not wrapped as apimachinery checks these https://github.com/kubernetes/kubernetes/blob/9410de78b23eb4f88de900552d90638bc67ffcd4/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go#L810
  3. If it was giving errors before, it was a bug.

Moving from v0.13.2 to v0.15.0 indicated some breaking changes that the users need to be aware of. Making a big jump to two minor version can face serious impact of change around this library and others that are shared between controller-runtime and k8s.io/*

You may need to read from a different client APIReader to see if those changes are getting the errors you desire instead of the cache that the client.Client is reading from being that they should have access to the internal store where it reads from.

@k8s-ci-robot k8s-ci-robot added the kind/support Categorizes issue or PR as a support question. label Sep 25, 2023
@bobdonat
Copy link
Author

bobdonat commented Sep 25, 2023

Had to use this local function instead of just calling apierrors.IsUnauthorized(err) to get the same behavior as 0.13.2.

	groupErr := &discovery.ErrGroupDiscoveryFailed{}
	if errors.As(err, &groupErr) {
		for _, err := range groupErr.Groups {
			if apierrors.IsUnauthorized(err) {
				return true
			}
		}
	}

Seems like a breaking change to me that is not documented.

I have seen others had to use something similar to replace apierrors.IsNotFound(err).

@troy0820
Copy link
Member

This looks related #2354 and the fixed was merged in main #2472

@troy0820
Copy link
Member

This landed in v0.16.2 release

@bobdonat
Copy link
Author

Will this be backported to 0.15?

Looks related but the PR mentions fixes use of meta.IsNoMatchError() which is not what I am using

@troy0820
Copy link
Member

I'm not sure, if I make the backport, it will still need to be released by the maintainers.

@k8s-triage-robot
Copy link

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

This bot triages un-triaged issues 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 issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue 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 Jan 30, 2024
@k8s-triage-robot
Copy link

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 29, 2024
@k8s-triage-robot
Copy link

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

This bot triages issues 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 issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

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

/close not-planned

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 30, 2024
@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

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

This bot triages issues 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 issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

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

/close not-planned

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support Categorizes issue or PR as a support question. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

4 participants