Introduce ansattporten - #463
Open
hermanwh wants to merge 3 commits into
Open
Conversation
hermanwh
marked this pull request as draft
August 17, 2026 13:28
hermanwh
marked this pull request as ready for review
August 17, 2026 13:29
Contributor
|
Thank you for the contribution! 🙌 I'll take a closer look as soon as I can |
tronghn
approved these changes
Aug 20, 2026
tronghn
left a comment
tronghn
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me! This is in line with the other implementations.
I do think we could refactor certain parts to make it easier to add new kinds in the future without all the switch statements, but that's way out of scope for this change.
Contributor
|
Wait with merge. Depends on nais/liberator#339. |
Author
I've updated to include the frontchannelLogoutURI field. Will rebase, amend commits and remove Kartverket build pipeline once the Liberator PR is merged 🙌 |
The ansattporten feature defaults to false, similar to Maskinporten. If ansattporten is enabled, the corresponding well-known URL must be configured. Implementation is identical as the existing support for IDPortenClient. The most relevant differences in the CRDs are that IntegrationType is always set to Ansattporten for AnsattportenClient, and that the deprecated RedirectURI is ommitted in favor of RedirectURIs. Note that frontchannelLogoutURI is required for AnsattportenClient despite being optional for IDPortenClient. It is unknown why the latter is optional, and it should probably be made required in the future. The Liberator CRD for AnsattportenClient must be installed in the K8s-cluster before the ansattporten feature is enabled in Digdirator. Co-authored-by: Lars Sorensen <lars.sorensen@kartverket.no>
hermanwh
force-pushed
the
introduce-ansattporten
branch
from
August 24, 2026 10:48
9e78adf to
9b03e9f
Compare
Author
|
I've force pushed new changes with the latest Liberator version and without the Kartverket build pipeline. cc @tronghn |
There was a problem hiding this comment.
Pull request overview
Adds optional, feature-gated Ansattporten client support across reconciliation, configuration, secrets, metrics, Helm deployment, documentation, and tests.
Changes:
- Adds Ansattporten controller, registration, labels, metrics, and secrets.
- Adds configuration, Helm values, RBAC, samples, and documentation.
- Adds fixtures, integration tests, and dependency updates.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Summary | Review notes |
|---|---|---|
README.md |
Documents Ansattporten support. | — |
pkg/secrets/secrets.go |
Generates Ansattporten secrets. | — |
pkg/secrets/secrets_test.go |
Tests secret generation. | — |
pkg/secrets/consts.go |
Adds secret key constants. | — |
pkg/metrics/metrics.go |
Adds Ansattporten metrics. | — |
pkg/fixtures/cluster.go |
Adds cluster fixtures. | — |
pkg/fixtures/clients.go |
Adds client fixtures. | — |
pkg/digdir/types/types.go |
Adds the integration type. | — |
pkg/config/config.go |
Adds feature and metadata configuration. | — |
pkg/clients/labels.go |
Adds Ansattporten labels. | — |
pkg/clients/labels_test.go |
Tests label generation. | — |
pkg/clients/instance.go |
Adds registration and client helpers. | — |
pkg/clients/instance_test.go |
Tests Ansattporten behavior. | — |
go.sum |
Updates dependency checksums. | — |
go.mod |
Updates the Liberator dependency. | — |
controllers/common/testdata/ansattporten/update-response.json |
Adds update response fixture. | — |
controllers/common/testdata/ansattporten/list-response.json |
Adds list response fixture. | — |
controllers/common/testdata/ansattporten/list-response-exists.json |
Adds existing-client fixture. | — |
controllers/common/testdata/ansattporten/create-response.json |
Adds create response fixture. | — |
controllers/common/test/testenv.go |
Registers Ansattporten in test environments. | — |
controllers/common/test/handlers.go |
Adds test API handling. | — |
controllers/common/secrets.go |
Routes Ansattporten secret generation. | — |
controllers/ansattportenclient/ansattportenclient_controller.go |
Implements the controller. | — |
controllers/ansattportenclient/ansattportenclient_controller_test.go |
Adds controller integration tests. | — |
config/samples/ansattportenclient.yaml |
Adds a sample resource. | — |
cmd/digdirator/main.go |
Feature-gates controller setup. | — |
charts/values.yaml |
Adds Helm configuration and CIDRs. | Moderate: Add Ansattporten CIDRs to the relevant egress policies. |
charts/templates/secrets.yaml |
Configures feature environment variables. | — |
charts/templates/rbac.yaml |
Adds Ansattporten RBAC permissions. | — |
charts/Feature.yaml |
Adds feature configuration metadata. | Nit: Link to Ansattporten well-known documentation instead of ID-porten documentation. |
Suppressed comments (2)
pkg/metrics/metrics.go:129
- The new failure counter is registered, but
charts/templates/prometheusrule.ymlstill only alerts on ID-porten and Maskinporten failures. With the chart's defaultalerts: true, failed Ansattporten reconciliations will not produce the corresponding operational warning, unlike the other supported client types. Add an Ansattporten failed-processing alert alongside the existing rules.
AnsattportenClientsFailedProcessingCount = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "ansattporten_client_failed_processing_count",
Help: "Number of ansattporten clients that failed processing",
pkg/metrics/metrics.go:558
- This lists
AnsattportenClientresources unconditionally even though the feature is disabled by default and the PR documents that the CRD may not be installed until the feature is enabled. In that supported state, every refresh returns a KubernetesNoMatcherror and the empty list leaves the gauge at zero while producing continuous error logs. Make this collection conditional on the feature/CRD being available, as well as the controller registration.
if err = m.reader.List(ctx, &ansattportenClientsList); err != nil {
log.Error("failed to list ansattporten clients", "error", err)
}
AnsattportenClientsTotal.Set(float64(len(ansattportenClientsList.Items)))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| config: | ||
| type: bool | ||
| ansattporten.wellKnownUrl: | ||
| description: Ansattporten well-known URL. Required when ansattporten.enabled is true. See https://docs.digdir.no/docs/idporten/oidc/oidc_func_wellknown.html |
Comment on lines
+44
to
+46
| cidrs: | ||
| - "139.105.36.165/32" | ||
| - "139.105.36.133/32" |
Author
|
Copilot makes a fair point regarding the cidrs in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
charts/values.yaml:46
- These Ansattporten CIDRs are never consumed by the chart:
charts/templates/netpol.yamlonly allows the ID-porten and Maskinporten ranges for both the operator and replicated workload egress policies. With the feature enabled, discovery/token traffic to Ansattporten will therefore be blocked by the NetworkPolicies; add this range to each relevant egress policy (or remove the unused values).
cidrs:
- "139.105.36.165/32"
- "139.105.36.133/32"
Comment on lines
+555
to
+558
| if err = m.reader.List(ctx, &ansattportenClientsList); err != nil { | ||
| log.Error("failed to list ansattporten clients", "error", err) | ||
| } | ||
| AnsattportenClientsTotal.Set(float64(len(ansattportenClientsList.Items))) |
Author
There was a problem hiding this comment.
I suppose the same is true for both Features.IDPorten and Features.Maskinporten as well.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (1)
charts/values.yaml:46
- These Ansattporten CIDRs are not consumed by the chart:
charts/templates/netpol.yamlonly ranges the ID-porten and Maskinporten CIDRs in the Digdirator and replicated workload egress policies. With Ansattporten enabled, the controller cannot reach the configured well-known endpoint, and Ansattporten-enabled workloads get no provider egress rule. Add this range to each applicable egress policy (and a corresponding workload policy) or remove the unused values.
cidrs:
- "139.105.36.165/32"
- "139.105.36.133/32"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ansattporten feature defaults to false, similar to Maskinporten. If ansattporten is enabled, the corresponding well-known URL must be configured.
Implementation is identical as the existing support for IDPortenClient. The most relevant differences in the CRDs are that IntegrationType is always set to Ansattporten for AnsattportenClient, and that the deprecated RedirectURI is ommitted in favor of RedirectURIs.
The Liberator CRD for AnsattportenClient must be installed in the K8s-cluster before the ansattporten feature is enabled in Digdirator.