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.

Sign up for GitHub

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

fix: remove default namespace as a requirement to list namespaces v2 #3716

Merged
merged 3 commits into from
Dec 15, 2024
Merged

fix: remove default namespace as a requirement to list namespaces v2 #3716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
648961d
fix: remove default namespace as a requirement to list namespaces v2
erka Dec 10, 2024
81edf65
research the issues with tests
erka Dec 12, 2024
e6e0121
Merge branch 'main' into rd/rpc-no-namespace
erka Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions build/testing/integration/authz/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ func (s clientCallSet) assert(t *testing.T, ctx context.Context, client sdk.SDK)
type clientCall func(*testing.T, context.Context, sdk.SDK) error

func GetNamespace(in *flipt.GetNamespaceRequest) clientCall {
if in.GetNamespaceKey() == "" {
in.Key = "default"
}
return func(t *testing.T, ctx context.Context, s sdk.SDK) error {
_, err := s.Flipt().GetNamespace(ctx, in)
return fmt.Errorf("GetNamespace: %w", err)
Expand Down Expand Up @@ -434,6 +437,7 @@ func CreateRollout(in *flipt.CreateRolloutRequest) clientCall {
return fmt.Errorf("CreateRollout: %w", err)
}
}

func UpdateRollout(in *flipt.UpdateRolloutRequest) clientCall {
return func(t *testing.T, ctx context.Context, s sdk.SDK) error {
_, err := s.Flipt().UpdateRollout(ctx, in)
Expand Down
8 changes: 7 additions & 1 deletion rpc/flipt/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ type Request struct {
Status Status `json:"status"`
}

func WithNoNamespace() func(*Request) {
return func(r *Request) {
r.Namespace = ""
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

im confused about what the difference is between using this and what you did previously by setting WithNamespace("") in the ListNamespaceRequest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@markphelps It would be nice if you double check it with me. Burned once already...

WithNamespace doesn't allow to set empty string for some reason

flipt/rpc/flipt/request.go

Lines 52 to 58 in 2ca2273

func WithNamespace(ns string) func(*Request) {
return func(r *Request) {
if ns != "" {
r.Namespace = ns
}
}
}

As WithNamespace is a public func, I can't remove condition without breaking changes for others. So I've created a new option WithNoNamespace.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are the only ones who use this function as its only used internally to enforce authz, so I think its fine to remove the check for empty string in the function body

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@markphelps Could you please help me how to move forward? The integration tests has a case where the empty namespace should fallback to default namespace. Should I skip this case for authz integration tests or should I continue with WithNoNamespace option and keep all the integration test cases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess ideally we should continue again with the WithNoNamespace option to keep the integration tests. we should likely add one to make sure that they can list namespaces still if WithNoNamespace is used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am stuck. Integration tests for authz don't have calls with ListNamespaceRequest as I see. What is going on? Where is my blind spot? Why namespaced viewer could be affected by this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@erka I will take a look this evening

Copy link
Collaborator

Choose a reason for hiding this comment

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

so the policy for the IT is coming from here: https://github.com/flipt-io/flipt/blob/main/build/testing/integration.go#L704-L839

default allow = false

allow if {
    input.authentication.metadata["is_bootstrap"] == "true"
}

allow if {
	some rule in has_rules

	permit_string(rule.resource, input.request.resource)
	permit_slice(rule.actions, input.request.action)
	permit_string(rule.namespace, input.request.namespace)
}

allow if {
	some rule in has_rules

	permit_string(rule.resource, input.request.resource)
	permit_slice(rule.actions, input.request.action)
	not rule.namespace
}

has_rules contains rules if {
	some role in data.roles
	role.name == input.authentication.metadata["io.flipt.auth.role"]
	rules := role.rules[_]
}

has_rules contains rules if {
	some role in data.roles
	role.name == input.authentication.metadata["io.flipt.auth.k8s.serviceaccount.name"]
	rules := role.rules[_]
}

and the failing test is here: https://github.com/flipt-io/flipt/blob/main/build/testing/integration/authn/auth.go#L145

so there is some reason that the policy is matching, but that doesn't make sense because we are not passing in a role in the request, so has_rules should be returning the empty set, but it must be returning some rules which makes me think the "" role is being matched somewhere in the policy

I think our rego policy for the authz test is not compatible with the change of allowing no namespace in the request

Copy link
Collaborator Author

@erka erka Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A wild guess... if tests are using rest api and getNamespace with empty namespace, the url probably will be /api/v1/namespaces/ and grpc-gateway could remove the last flash and interpret that as call to list namespaces.

flipt/sdk/go/http/flipt.sdk.gen.go

Lines 87 to 95 in 729a965

func (x *FliptClient) GetNamespace(ctx context.Context, v *flipt.GetNamespaceRequest, _ ...grpc.CallOption) (*flipt.Namespace, error) {
var body io.Reader
values := url.Values{}
values.Set("reference", v.Reference)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, x.addr+fmt.Sprintf("/api/v1/namespaces/%v", v.Key), body)
if err != nil {
return nil, err
}
req.URL.RawQuery = values.Encode()

}

func WithNamespace(ns string) func(*Request) {
return func(r *Request) {
if ns != "" {
Expand Down Expand Up @@ -98,7 +104,7 @@ func (req *GetNamespaceRequest) Request() []Request {
}

func (req *ListNamespaceRequest) Request() []Request {
return []Request{NewRequest(ResourceNamespace, ActionRead, WithNamespace(""))}
return []Request{NewRequest(ResourceNamespace, ActionRead, WithNoNamespace())}
}

func (req *CreateNamespaceRequest) Request() []Request {
Expand Down
4 changes: 3 additions & 1 deletion rpc/flipt/request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ import (

func TestListNamespaceRequest_Request(t *testing.T) {
req := &ListNamespaceRequest{}
assert.Equal(t, []Request{NewRequest(ResourceNamespace, ActionRead)}, req.Request())
expected := NewRequest(ResourceNamespace, ActionRead)
expected.Namespace = ""
assert.Equal(t, []Request{expected}, req.Request())
}
Loading