From 67b24c41f1880d8429288515be7d97614e081a9a Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Thu, 9 Jan 2025 12:32:23 -0800 Subject: [PATCH] Serialize /auth parameters in model order The serialization order of the parameters to the `/auth` route when generating an `Ingress` was somewhat random based on when features were added. Serialize in the order of the model attributes instead (which is mostly alphabetical). --- src/gafaelfawr/models/kubernetes.py | 23 ++++++++++----------- tests/data/kubernetes/output/ingresses.yaml | 8 +++---- tests/operator/ingress_test.py | 4 ++-- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/gafaelfawr/models/kubernetes.py b/src/gafaelfawr/models/kubernetes.py index 92ad0004..a1766cf5 100644 --- a/src/gafaelfawr/models/kubernetes.py +++ b/src/gafaelfawr/models/kubernetes.py @@ -360,30 +360,29 @@ def to_auth_query(self) -> list[tuple[str, str]]: List of query parameters corresponding to this ingress configuration to pass to the Gafaelfawr ``/ingress/auth`` route. """ - query = [("scope", s) for s in self.scopes.scopes] - query.extend(("only_service", s) for s in self.only_services or []) - if self.service: - query.append(("service", self.service)) - if self.scopes.satisfy != Satisfy.ALL: - query.append(("satisfy", self.scopes.satisfy.value)) + query = [] + if self.auth_type: + query.append(("auth_type", self.auth_type.value)) if self.delegate: if self.delegate.notebook: query.append(("notebook", "true")) elif self.delegate.internal: service = self.delegate.internal.service query.append(("delegate_to", service)) - query.extend( - ("delegate_scope", s) - for s in self.delegate.internal.scopes - ) + scopes = self.delegate.internal.scopes + query.extend(("delegate_scope", s) for s in scopes) if self.delegate.minimum_lifetime: minimum_lifetime = self.delegate.minimum_lifetime minimum_str = str(int(minimum_lifetime.total_seconds())) query.append(("minimum_lifetime", minimum_str)) if self.delegate.use_authorization: query.append(("use_authorization", "true")) - if self.auth_type: - query.append(("auth_type", self.auth_type.value)) + query.extend(("only_service", s) for s in self.only_services or []) + query.extend(("scope", s) for s in self.scopes.scopes) + if self.scopes.satisfy != Satisfy.ALL: + query.append(("satisfy", self.scopes.satisfy.value)) + if self.service: + query.append(("service", self.service)) if self.username: query.append(("username", self.username)) return query diff --git a/tests/data/kubernetes/output/ingresses.yaml b/tests/data/kubernetes/output/ingresses.yaml index 205402b7..74e0bc16 100644 --- a/tests/data/kubernetes/output/ingresses.yaml +++ b/tests/data/kubernetes/output/ingresses.yaml @@ -50,7 +50,7 @@ metadata: nginx.ingress.kubernetes.io/auth-method: "GET" nginx.ingress.kubernetes.io/auth-response-headers: "Authorization,Cookie,X-Auth-Request-Email,X-Auth-Request-Service,X-Auth-Request-Token,X-Auth-Request-User" nginx.ingress.kubernetes.io/auth-signin: "https://foo.example.com/login" - nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?scope=read:all&satisfy=any¬ebook=true&minimum_lifetime=600" + nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?notebook=true&minimum_lifetime=600&scope=read:all&satisfy=any" nginx.ingress.kubernetes.io/configuration-snippet: | {snippet} creationTimestamp: {any} @@ -95,7 +95,7 @@ metadata: annotations: nginx.ingress.kubernetes.io/auth-method: GET nginx.ingress.kubernetes.io/auth-response-headers: "Authorization,Cookie,X-Auth-Request-Email,X-Auth-Request-Service,X-Auth-Request-Token,X-Auth-Request-User" - nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?scope=read:all&scope=read:some&delegate_to=some-service&delegate_scope=read:all&delegate_scope=read:some&auth_type=basic" + nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?auth_type=basic&delegate_to=some-service&delegate_scope=read:all&delegate_scope=read:some&scope=read:all&scope=read:some" nginx.ingress.kubernetes.io/configuration-snippet: | {snippet} creationTimestamp: {any} @@ -135,7 +135,7 @@ metadata: annotations: nginx.ingress.kubernetes.io/auth-method: GET nginx.ingress.kubernetes.io/auth-response-headers: "Authorization,Cookie,X-Auth-Request-Email,X-Auth-Request-Service,X-Auth-Request-Token,X-Auth-Request-User" - nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?scope=read:all&delegate_to=some-service&delegate_scope=read:all&use_authorization=true" + nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?delegate_to=some-service&delegate_scope=read:all&scope=read:all&use_authorization=true" nginx.ingress.kubernetes.io/configuration-snippet: | add_header "X-Foo" "bar"; {snippet} @@ -255,7 +255,7 @@ metadata: annotations: nginx.ingress.kubernetes.io/auth-method: GET nginx.ingress.kubernetes.io/auth-response-headers: "Authorization,Cookie,X-Auth-Request-Email,X-Auth-Request-Service,X-Auth-Request-Token,X-Auth-Request-User" - nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?scope=read:all&only_service=portal&only_service=vo-cutouts&service=uws" + nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?only_service=portal&only_service=vo-cutouts&scope=read:all&service=uws" nginx.ingress.kubernetes.io/configuration-snippet: | {snippet} creationTimestamp: {any} diff --git a/tests/operator/ingress_test.py b/tests/operator/ingress_test.py index f6fe4563..ed94e968 100644 --- a/tests/operator/ingress_test.py +++ b/tests/operator/ingress_test.py @@ -80,7 +80,7 @@ async def test_replace( expected_url = ( "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth" - "?scope=read:all&service=tap&auth_type=basic" + "?auth_type=basic&scope=read:all&service=tap" ) expected["metadata"]["annotations"][ "nginx.ingress.kubernetes.io/auth-url" @@ -111,7 +111,7 @@ async def test_replace( expected_url = ( "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth" - "?scope=read:all&service=tap&auth_type=bearer" + "?auth_type=bearer&scope=read:all&service=tap" ) expected["metadata"]["annotations"][ "nginx.ingress.kubernetes.io/auth-url"