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

BUG 2082099: Fix finalizer string too long #1178

Merged
merged 1 commit into from Sep 28, 2022

Conversation

Vincent056
Copy link
Contributor

@Vincent056 Vincent056 commented Sep 20, 2022

/kind bug

What this PR does / why we need it:

This PR shorten the finalizer string if it is over the limit size of 63

Does this PR have test?

Yes

Does this PR introduce a user-facing change?

No

Fix the finalizer string too long, shorten the length of the node name if the finalizer string combined length is over the size of 63

####Which issue(s) this PR fixes:

Fixes: BZ 2082099

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 20, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @Vincent056. 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.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 20, 2022
@saschagrunert
Copy link
Member

/ok-to-test

Thank you for the fix @Vincent056!

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 20, 2022
if !ok {
return nil, errors.New("cannot determine node name")
}
// Make sure the length of finalizer is not longer than 63 characters
if len(nodeName) > 55 {
Copy link
Member

Choose a reason for hiding this comment

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

We can make it a const to fix the CI.

@jhrozek
Copy link
Contributor

jhrozek commented Sep 20, 2022

An e2e test would also be nice.

@Vincent056 Vincent056 changed the title WIP BUG 2082099: Fix finalizer string too long BUG 2082099: Fix finalizer string too long Sep 21, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 21, 2022
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

just some nits

@@ -33,6 +33,11 @@ import (
"sigs.k8s.io/security-profiles-operator/internal/pkg/util"
)

const (
// node name length limit
nodeNameLenLimit = 55
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a complete nit, but can you change a comment to include that the finalizer can be up to 63 chars in lenght and 55 + len("-deleted") = 63? We might also want to use a constant instead of -deleted so that both are next to each other.

"sigs.k8s.io/security-profiles-operator/internal/pkg/config"
)

// Expected shorten the node name if length exceed the limit
Copy link
Contributor

Choose a reason for hiding this comment

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

yup, validation error

@jhrozek
Copy link
Contributor

jhrozek commented Sep 21, 2022

internal/pkg/nodestatus/nodestatus_test.go:23: File is not `gci`-ed with --skip-generated -s standard,default,prefix(sigs.k8s.io/security-profiles-operator) (gci)
	"github.com/stretchr/testify/require"

So far I've had the best results with fixing the gci issues by running gci manually, something like:

go install github.com/daixiang0/gci@latest
gci write path/to/file

There is also the -l option that allows you to specify "local" imports that should be sorted last, but I could never get the flag combination to work, so I sort them manually. Normally the SPO expects the imports to be sorted:

  • stdlib, alphabetically
  • third party, alphabetically
  • local, alphabetically
internal/pkg/nodestatus/nodestatus_test.go:60: File is not `gofumpt`-ed (gofumpt)

/build/golangci-lint run --fix --build-tags no_bpf should do the trick

Don't forget to remove and reinstall golangci-lint if you have an old version locally.

@Vincent056 Vincent056 force-pushed the finalizer branch 2 times, most recently from 766162f to 0810659 Compare September 22, 2022 03:20
@Vincent056
Copy link
Contributor Author

@jhrozek It seems like t.Parallel called after t.Setenv; cannot set environment variables in parallel tests [recovered], and not setting parallel is causing verify to fail

@jhrozek
Copy link
Contributor

jhrozek commented Sep 22, 2022

@jhrozek It seems like t.Parallel called after t.Setenv; cannot set environment variables in parallel tests [recovered], and not setting parallel is causing verify to fail

Can you try adding //nolint:paralleltest with the explanation you used to suppress the warning? There are some other uses of this stanza in SPO, you can git grep for its usage to see exactly where and how it's used.

@Vincent056 Vincent056 force-pushed the finalizer branch 2 times, most recently from 390f407 to 8c491ab Compare September 22, 2022 20:26
Shorten finalizer string if it is over the limit
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

/lgtm
I restarted the flatcar test, the failure looks like a install-time fluke

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhrozek, Vincent056

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Sep 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit 4e141f2 into kubernetes-sigs:main Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants