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

Adds AuthenticationFilter Support to Kubernetes Provider #791

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Dec 9, 2022

Adds AuthenticationFilter API support to the Kubernetes provider:

  • api/v1alpha1/authenticationfilter_types.go: Adds missing Authentication -> AuthenticationFilter changes from Adds Request Authentication API #733.
  • api/v1alpha1/zz_generated.deepcopy.go: Regenerates DeepCopy methods due to AuthenticationFilter API changes.
  • internal/envoygateway/scheme.go: Adds the v1alpha1, e.g. AuthenticationFilter, group to the scheme.
  • internal/gatewayapi/helpers.go: Adds ValidateAuthenticationFilterRef() helper function to validate AuthenticationFilter references from an HTTPRoute.
  • internal/gatewayapi/helpers_test.go: Adds ValidateAuthenticationFilterRef() unit tests.
  • internal/gatewayapi/translator.go: Adds AuthenticationFilter to the gatewayapi Resources type. Adds NewResources() helper function.
  • internal/gatewayapi/zz_generated.deepcopy.go: Regenerates DeepCopy methods due to API changes.
  • internal/provider/kubernetes/config/crd/kustomization.yaml: Adds AuthenticationFilter CRD to the list of installed CRDs.
  • internal/provider/kubernetes/config/rbac/role.yaml: Generated by make manifests, updating EG RBAC for AuthenticationFilter.
  • internal/provider/kubernetes/controller.go: Add a watch for instances of AuthenticationFilter. Updates the resourceMappings type to map HTTPRoutes<>AuthenticationFilters. Adds AuthenticationFilters referenced by managed HTTPRoutes to the resourceTree.
  • internal/provider/kubernetes/rbac.go: Updates EG RBAC kubebuilder tags for managing instances of AuthenticationFilter.
  • internal/provider/kubernetes/helpers.go: Removes namespace check from validateBackendRef(). Now that [provider] refactoring kubernetes provider to single reconciler #702 adds full ReferenceGrant support, an HTTPRoute backend should be allowed to reference a Service in a different namespace if the necessary ReferenceGrant exists (xref).
  • internal/provider/kubernetes/kubernetes_test.go: Adds AuthenticationFilter test case to Kube provider integration tests.
  • internal/provider/kubernetes/predicates.go Adds predicate to filter AuthenticationFilter instancess from the controller's watch.
  • internal/provider/kubernetes/routes.go: Updates processHTTPRoutes() to process AuthenticationFilters referenced by managed HTTPRoutes.
  • internal/provider/kubernetes/routes_test.go: Renamed from route_test.go to match routes.go. Adds a unit test for processHTTPRoutes()`.

xref:#790

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans
Copy link
Contributor Author

danehans commented Dec 9, 2022

Marking as Draft while I resolve the conformance test failure.

@danehans danehans marked this pull request as draft December 9, 2022 18:57
@danehans danehans marked this pull request as ready for review December 9, 2022 19:31
@danehans
Copy link
Contributor Author

danehans commented Dec 9, 2022

Commit 2f01f4d removes the following changes from the initial commit:

`internal/provider/kubernetes/helpers.go`: Removes namespace check from `validateBackendRef()`. Now that #702 adds full ReferenceGrant support, an HTTPRoute backend should be allowed to reference a Service in a different namespace if the necessary ReferenceGrant exists ([xref](https://github.com/envoyproxy/gateway/blob/main/internal/provider/kubernetes/routes.go#L101-L111)).`

Due to failed conformance test:

...
helpers.go:301: Route gateway-conformance-infra/invalid-nonexistent-backend-ref expected 1 Parents got 0
    httproute-invalid-backendref-nonexistent.go:44: 
        	Error Trace:	/home/runner/work/gateway/gateway/test/conformance/helpers.go:303
        	            				/home/runner/work/gateway/gateway/test/conformance/helpers.go:174
        	            				/home/runner/work/gateway/gateway/test/conformance/httproute-invalid-backendref-nonexistent.go:44
        	            				/home/runner/work/gateway/gateway/test/conformance/suite.go:206
        	            				/home/runner/work/gateway/gateway/test/conformance/suite.go:170
        	Error:      	Received unexpected error:
        	            	timed out waiting for the condition
        	Test:       	TestGatewayAPIConformance/HTTPRouteInvalidNonExistentBackendRef
        	Messages:   	error waiting for HTTPRoute to have parents matching expectations

xref: #792

@@ -11,7 +11,8 @@ import (
gwapiv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
gwapiv1b1 "sigs.k8s.io/gateway-api/apis/v1beta1"

"github.com/envoyproxy/gateway/api/config/v1alpha1"
egcfgv1a1 "github.com/envoyproxy/gateway/api/config/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

can we combine these two into one ?

Copy link
Contributor Author

@danehans danehans Dec 12, 2022

Choose a reason for hiding this comment

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

Shouldn't we keep them separate to simplify moving the Gateway API extensions, e.g. AuthenticationFilter, out of EG into a separate repo in the future? If we decide to remove the config API group, I think that should be done in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

trying to highlight the fact that current dir structure is confusing - either we have api/extensions/v1alpha1 or just api/v1alpha1 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Per your request, I removed the security group from the Request Authentication design. I'll add this to the community meeting agenda to get input from other maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xref #809 to make a decision on API type grouping. IMO a separate PR should be submitted to resolve this issue.

@danehans danehans force-pushed the authen_api_kube branch 2 times, most recently from 4086819 to 8039e92 Compare December 12, 2022 18:50
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2022

Codecov Report

Merging #791 (16b425d) into main (d56ea6a) will decrease coverage by 0.35%.
The diff coverage is 54.78%.

@@            Coverage Diff             @@
##             main     #791      +/-   ##
==========================================
- Coverage   63.31%   62.95%   -0.36%     
==========================================
  Files          46       46              
  Lines        6157     6257     +100     
==========================================
+ Hits         3898     3939      +41     
- Misses       2019     2070      +51     
- Partials      240      248       +8     
Impacted Files Coverage Δ
internal/gatewayapi/translator.go 83.67% <0.00%> (-0.62%) ⬇️
internal/gatewayapi/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
internal/provider/kubernetes/predicates.go 61.21% <46.66%> (-1.46%) ⬇️
internal/provider/kubernetes/controller.go 49.90% <64.58%> (-2.20%) ⬇️
internal/provider/kubernetes/routes.go 61.62% <73.33%> (+3.88%) ⬆️
internal/gatewayapi/helpers.go 77.12% <82.35%> (+0.65%) ⬆️
internal/provider/kubernetes/helpers.go 78.22% <0.00%> (+2.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

.gitignore Show resolved Hide resolved
Signed-off-by: danehans <daneyonhansen@gmail.com>
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@danehans danehans merged commit f420fe1 into envoyproxy:main Dec 19, 2022
@danehans danehans deleted the authen_api_kube branch December 19, 2022 20:08
arkodg added a commit to arkodg/gateway that referenced this pull request Dec 19, 2022
Caused by merging envoyproxy#791
without rebasing envoyproxy#816

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
danehans pushed a commit that referenced this pull request Dec 19, 2022
* Fix package import for HTTPRouteFilterExtensionRef

Caused by merging #791
without rebasing #816

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix test

was failing with below error
```
    --- FAIL: TestProcessHTTPRoutes/httproute_with_one_authenticationfilter (0.00s)
        routes_test.go:221:
            	Error Trace:	/Users/arkodebdasgupta/go-workspace/src/github.com/envoyproxy/gateway/internal/provider/kubernetes/routes_test.go:221
            	Error:      	Received unexpected error:
            	            	List on GroupVersionKind gateway.networking.k8s.io/v1beta1, Kind=HTTPRoute specifies selector on field gatewayHTTPRouteIndex, but no index with name gatewayHTTPRouteIndex has been registered for GroupVersionKind gateway.networking.k8s.io/v1beta1, Kind=HTTPRoute
            	Test:       	TestProcessHTTPRoutes/httproute_with_one_authenticationfilter
```

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
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.

None yet

3 participants