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

chore: enable DisablePropagateGenerated feature gate by default #99

Merged

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Oct 14, 2023

As agreed in #87 (comment), this graduates the `DisablePropagateGenerated ``to Beta and enables it by default. This does not fix #87, but I still think we can close the issue as "not a problem" - since the problematic feature is no longer enabled by default.

Should probably be merged/released after #95. This PR is quite easy to rebase, so it can just stay open until we are ready for these changes.

Close #87

PS: The structure/wording of the new feature gates doc paragraph is almost a copy of https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/. Kudos to Kubernetes documentation contributors/maintainers. 🙏

@erikgb erikgb force-pushed the disable-propagate-generated-by-default branch 3 times, most recently from c1584f8 to 7941e89 Compare October 15, 2023 11:58
@erikgb
Copy link
Contributor Author

erikgb commented Oct 15, 2023

I also think this should wait for #100.

@erikgb erikgb force-pushed the disable-propagate-generated-by-default branch from 7941e89 to 6f225f3 Compare October 17, 2023 09:28
@erikgb
Copy link
Contributor Author

erikgb commented Oct 17, 2023

This PR should be ready for review now and eventual merge. I have updated the Helm values file after #100 and the failing test in CI seems like a flake.

@zoetrope zoetrope self-requested a review October 19, 2023 02:08
@ymmt2005
Copy link
Member

ymmt2005 commented Oct 23, 2023

The failed test log:

Namespace webhook should deny moving a sub-namespace under non-root/non-sub namespace
/home/runner/work/accurate/accurate/hooks/namespace_test.go:379
  [FAILED] in [It] - /home/runner/work/accurate/accurate/hooks/namespace_test.go:390 @ 10/17/23 09:34:15.064
• [FAILED] [0.008 seconds]
Namespace webhook [It] should deny moving a sub-namespace under non-root/non-sub namespace
/home/runner/work/accurate/accurate/hooks/namespace_test.go:379

  [FAILED] Unexpected error:
      <*errors.StatusError | 0xc0004b2e60>: 
      admission webhook "namespace.accurate.cybozu.io" denied the request: namespace does not exist: move-root
      {
          ErrStatus: {
              TypeMeta: {Kind: "", APIVersion: ""},
              ListMeta: {
                  SelfLink: "",
                  ResourceVersion: "",
                  Continue: "",
                  RemainingItemCount: nil,
              },
              Status: "Failure",
              Message: "admission webhook \"namespace.accurate.cybozu.io\" denied the request: namespace does not exist: move-root",
              Reason: "Forbidden",
              Details: nil,
              Code: 403,
          },
      }
  occurred
  In [It] at: /home/runner/work/accurate/accurate/hooks/namespace_test.go:390 @ 10/17/23 09:34:15.064

This was caused because the webhook uses a caching client:

Client: mgr.GetClient(),

IIRC, I used the caching client as I was a bit concerned about performance.

Copy link
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

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

LGTM

@ymmt2005 ymmt2005 merged commit 7fcb2aa into cybozu-go:main Oct 23, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should not attempt to modify resources in normal namespace
2 participants