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

Used the kmeta.ChildName() to generate OIDC service account name #7471

Closed
wants to merge 3 commits into from
Closed

Used the kmeta.ChildName() to generate OIDC service account name #7471

wants to merge 3 commits into from

Conversation

MeenuyD
Copy link
Contributor

@MeenuyD MeenuyD commented Nov 23, 2023

Refactor OIDC service account naming in GetOIDCServiceAccountNameForResource

This updates the method GetOIDCServiceAccountNameForResource in eventing/pkg/auth/serviceaccount.go to refactor the way OIDC service account names are generated. Instead of combining Group/Kind and the name of the parent object directly, it now uses kmeta.ChildName to generate the service account names. This change is made to prevent names from exceeding the maximum allowed length.
#7470

@knative-prow knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 23, 2023
Copy link

knative-prow bot commented Nov 23, 2023

Hi @MeenuyD. Thanks for your PR.

I'm waiting for a knative 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.

@knative-prow knative-prow bot requested review from creydr and matzew November 23, 2023 14:37
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@knative-prow knative-prow bot 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 Nov 23, 2023
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

Hey @MeenuyD it looks like the serviceaccount unit tests are failing:

--- FAIL: TestGetOIDCServiceAccountNameForResource (0.00s)
    --- FAIL: TestGetOIDCServiceAccountNameForResource/should_return_SA_name_in_correct_format (0.00s)
        serviceaccount_test.go:63: GetServiceAccountName() = nameoidc-group-kind, want oidc-group-kind-name
    --- FAIL: TestGetOIDCServiceAccountNameForResource/should_return_SA_name_in_lower_case (0.00s)
        serviceaccount_test.go:63: GetServiceAccountName() = my-brokeroidc-eventing.knative.dev-broker, want oidc-eventing.knative.dev-broker-my-broker

Could you fix them?

@creydr
Copy link
Member

creydr commented Nov 23, 2023

Wow this was fast @MeenuyD 🚀
FYI: you can run the unit tests locally: go test ./pkg/... and in general the DEVELOPMENT.md provides some helpful resources to get started (if something is unclear we're happy to review PRs on improving these docs too).

@creydr creydr linked an issue Nov 23, 2023 that may be closed by this pull request
Copy link

knative-prow bot commented Nov 24, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MeenuyD
Once this PR has been reviewed and has the lgtm label, please ask for approval from cali0707. 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:

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

pkg/auth/serviceaccount.go Outdated Show resolved Hide resolved
@@ -60,7 +60,7 @@ func TestGetOIDCServiceAccountNameForResource(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := GetOIDCServiceAccountNameForResource(tt.gvk, tt.objectMeta); got != tt.want {
t.Errorf("GetServiceAccountName() = %v, want %v", got, tt.want)
t.Errorf("GetOIDCServiceAccountNameForResource() = %v, want %v", got, tt.want)
Copy link
Member

Choose a reason for hiding this comment

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

@MeenuyD I think you need to change the want fields in the array of tests above. If you look at the test failures here, you'll see that they are failing because you changed the format of the string being returned by the function, and they are expecting the old format. So, just update the expected values and they should pass!

Copy link

knative-prow bot commented Nov 25, 2023

@MeenuyD: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit-tests_eventing_main 7ad1518 link true /test unit-tests

Your PR dashboard.

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. I understand the commands that are listed here.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023
@knative-prow-robot
Copy link
Contributor

PR needs rebase.

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.

@creydr
Copy link
Member

creydr commented Dec 4, 2023

@MeenuyD any updates on this? Can we help you with anything specific on this?

@MeenuyD
Copy link
Contributor Author

MeenuyD commented Dec 4, 2023

Hello @creydr why the test cases are failing

@creydr
Copy link
Member

creydr commented Dec 4, 2023

Hello @creydr why the test cases are failing

In the unit test, the want defines the expected output (line 48 or 57). Since the output changed through this PR, the expected output in the tests need to be adjusted.

func TestGetOIDCServiceAccountNameForResource(t *testing.T) {
tests := []struct {
name string
gvk schema.GroupVersionKind
objectMeta metav1.ObjectMeta
want string
}{
{
name: "should return SA name in correct format",
gvk: schema.GroupVersionKind{
Group: "group",
Version: "version",
Kind: "kind",
},
objectMeta: metav1.ObjectMeta{
Name: "name",
Namespace: "namespace",
},
want: "oidc-group-kind-name",
},
{
name: "should return SA name in lower case",
gvk: eventingv1.SchemeGroupVersion.WithKind("Broker"),
objectMeta: metav1.ObjectMeta{
Name: "my-Broker",
Namespace: "my-Namespace",
},
want: "oidc-eventing.knative.dev-broker-my-broker",
},
}

To get more details, click on the "details" column of the failing test comment (#7471 (comment))

And your PR needs a rebase

@creydr
Copy link
Member

creydr commented Dec 12, 2023

@MeenuyD do you need any more information for this?

@creydr
Copy link
Member

creydr commented Dec 20, 2023

Closing this, as done in #7521

@creydr creydr closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use kmeta.ChildName() to generate OIDC service account name
4 participants