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

[erka]

Ninja has more tricks :(

related #3711

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.55%. Comparing base (2e3fd06) to head (e6e0121).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3716   +/-   ##
=======================================
  Coverage   64.55%   64.55%           
=======================================
  Files         170      170           
  Lines       17163    17163           
=======================================
  Hits        11079    11079           
  Misses       5393     5393           
  Partials      691      691           

Flag	Coverage Δ
unittests	64.55% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[markphelps]

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

[erka]

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

[markphelps]

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

[erka]

@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?

[markphelps]

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

[erka]

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?

[markphelps]

@erka I will take a look this evening

[markphelps]

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

[erka]

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()

[erka]

@markphelps any way to get this to main?

[markphelps]

Thank you for the hard work debugging this! I think we will also update the generated sdk to not allow empty keys but this change should work too