FPA and geneva certificate via CertCreation step#5909
Open
geoberle wants to merge 5 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geoberle The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Implements the manage gate for CreateCertificate steps, allowing pipelines to conditionally skip certificate creation based on a config-driven "Enabled"/"Disabled" value. When manage is nil (default), the step proceeds as before, preserving backward compatibility. Upstream: Azure/ARO-Tools#261 Ref: https://redhat.atlassian.net/browse/ARO-28053
Move FPA certificate creation from the svc-cluster.bicep ARM template
to a dedicated CreateCertificate pipeline step in svc-pipeline.yaml,
using the new manage field to control whether the step executes.
The config manage field changes from a boolean to a string enum
(Enabled/Disabled). A CEL validation rule ensures the SAN is
constructed as {name}.{svcParentZoneName} when set.
Ref: https://redhat.atlassian.net/browse/ARO-28053
… pipeline step Move the Geneva Actions certificate creation from geneva-identities.bicep to a CreateCertificate pipeline step in global-pipeline.yaml and global-pipeline-stg.yaml, using the manage field to gate execution. The certificate's outputs (PublicKey, Thumbprint, KeyIdentifier, NotBefore, NotAfter) were previously consumed by the Entra app's keyCredentials array when useSNI was false. This removes that consumption entirely: - MSFT environments use SNI for authentication (trustedSubjectNameAndIssuers) and never needed keyCredentials. - Dev environments don't authenticate to the Geneva Actions app, so keyCredentials were wired up but unused. If dev ever needs to authenticate, the certificate must be attached through another mechanism — the public key cannot be read from ARM/bicep (the KV secret contains the PFX blob but ARM only exposes metadata, and the public key is too large for a tag). The geneva-identities ARM step now depends on geneva-actions-cert, which transitively depends on both KV issuer steps. Also extracts a shared certificateManage schema definition (Enabled/Disabled) referenced by both FPA and Geneva Actions certs. Ref: https://redhat.atlassian.net/browse/ARO-28053
Contributor
There was a problem hiding this comment.
Pull request overview
This PR shifts certificate creation for FPA and multiple Geneva certificates out of Bicep templates and into CreateCertificate pipeline steps, adding a new optional manage field to allow config-driven skipping. It also bumps ARO-Tools dependencies to pick up the upstream Manage *Value support and updates config/schema (including CEL validations) to support the new certificate inputs.
Changes:
- Add
managesupport toCreateCertificateexecution/validation intooling/templatize, including unit tests. - Migrate FPA + Geneva Actions + Geneva logs certificates from Bicep modules to explicit
CreateCertificatepipeline steps, leaving only Key Vault secret-access role assignments in Bicep where needed. - Update config schema + rendered configs for new
manageenum and certificate fields (san,contentType), and bump Go module deps for the new upstream tooling.
Reviewed changes
Copilot reviewed 32 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tooling/yamlwrap/go.mod | Bump ARO-Tools/tools/yamlwrap dependency to newer upstream revision. |
| tooling/yamlwrap/go.sum | Update sums for bumped ARO-Tools deps. |
| tooling/templatize/pkg/pipeline/certificate.go | Add manage-driven skipping logic for CreateCertificate steps. |
| tooling/templatize/pkg/pipeline/certificate_test.go | Add unit tests for shouldSkipCertificateStep. |
| tooling/templatize/cmd/pipeline/validate/options.go | Validate CreateCertificate.manage variable refs when present. |
| tooling/templatize/go.mod | Bump ARO-Tools/* deps; add indirect armresourcegraph update. |
| tooling/templatize/go.sum | Update sums for bumped deps (incl. armresourcegraph). |
| tooling/secret-sync/go.mod | Bump ARO-Tools/tools/secret-sync dependency revision. |
| tooling/secret-sync/go.sum | Update sums for bumped ARO-Tools deps. |
| tooling/pipeline-documentation/go.mod | Bump ARO-Tools/pipelines dependency revision. |
| tooling/pipeline-documentation/go.sum | Update sums for bumped ARO-Tools deps. |
| tooling/helmtest/go.mod | Bump ARO-Tools deps used by helmtest tooling. |
| tooling/helmtest/go.sum | Update sums for bumped ARO-Tools deps. |
| tooling/hcpctl/go.mod | Bump cmdutils and armresourcegraph dependency versions. |
| tooling/hcpctl/go.sum | Update sums for bumped deps (incl. armresourcegraph). |
| tooling/grafanactl/go.mod | Bump grafanactl + cmdutils deps; add indirect armresourcegraph. |
| tooling/grafanactl/go.sum | Update sums for bumped deps (incl. armresourcegraph). |
| tooling/aro-hcp-exporter/go.mod | Bump armresourcegraph dependency version. |
| tooling/aro-hcp-exporter/go.sum | Update sums for bumped armresourcegraph dependency. |
| test/go.mod | Bump ARO-Tools/config + prow-job-executor deps and related indirects. |
| test/go.sum | Update sums for bumped ARO-Tools deps (incl. armresourcegraph). |
| mgmt-agent/go.mod | Bump ARO-Tools/testutil dependency revision. |
| mgmt-agent/go.sum | Update sums for bumped ARO-Tools/testutil dependency. |
| dev-infrastructure/templates/svc-cluster.bicep | Remove in-template FPA/Geneva cert creation; keep CSI secret access grants. |
| dev-infrastructure/templates/mgmt-cluster.bicep | Remove Geneva cert creation; keep CSI secret access grants for expected secrets. |
| dev-infrastructure/templates/geneva-identities.bicep | Reduce to Entra app registration only; remove cert and keyCredentials wiring. |
| dev-infrastructure/svc-pipeline.yaml | Add fpa-cert + geneva-rp-logs-cert CreateCertificate steps. |
| dev-infrastructure/mgmt-pipeline.yaml | Add geneva-rp-logs-cert + geneva-cluster-logs-cert CreateCertificate steps. |
| dev-infrastructure/global-pipeline.yaml | Add geneva-logs-cert + geneva-actions-cert CreateCertificate steps; update dependencies. |
| dev-infrastructure/global-pipeline-stg.yaml | Same as global pipeline for staging variant. |
| dev-infrastructure/configurations/svc-cluster.tmpl.bicepparam | Remove obsolete cert-creation parameters now handled by pipeline steps. |
| dev-infrastructure/configurations/mgmt-cluster.tmpl.bicepparam | Remove obsolete Geneva cert-creation parameters now handled by pipeline steps. |
| dev-infrastructure/configurations/geneva-identities.tmpl.bicepparam | Drop removed cert params; pass Geneva Actions SAN for SNI trust config. |
| dev-infrastructure/configurations/geneva-identities-stg.tmpl.bicepparam | Same as above for staging “V2” config. |
| config/rendered/dev/pers/westus3.yaml | Render new manage enum + san/contentType fields for cert configs. |
| config/rendered/dev/perf/westus3.yaml | Same rendered config updates for perf stamp. |
| config/rendered/dev/dev/westus3.yaml | Same rendered config updates for dev stamp. |
| config/rendered/dev/cspr/westus3.yaml | Same rendered config updates for cspr stamp. |
| config/rendered/dev/ci01/centralus.yaml | Same rendered config updates for CI stamp. |
| config/rendered/dev/ci00/centralus.yaml | Same rendered config updates for CI stamp. |
| config/config.yaml | Update defaults/cloud overrides to use manage: Enabled/Disabled and new cert fields. |
| config/config.schema.json | Add certificateManage enum, new cert fields, and CEL validations for SAN construction. |
Comment on lines
+3566
to
+3569
| { | ||
| "rule": "self.firstPartyAppCertificate.san == '' || self.firstPartyAppCertificate.san == self.firstPartyAppCertificate.name + '.' + self.dns.svcParentZoneName", | ||
| "message": "firstPartyAppCertificate.san must equal firstPartyAppCertificate.name + '.' + dns.svcParentZoneName" | ||
| }, |
Comment on lines
+3570
to
+3573
| { | ||
| "rule": "self.geneva.actions.certificate.san == '' || self.geneva.actions.certificate.san == self.geneva.actions.certificate.name + '.' + self.dns.globalCertificatesDomain", | ||
| "message": "geneva.actions.certificate.san must equal geneva.actions.certificate.name + '.' + dns.globalCertificatesDomain" | ||
| }, |
Comment on lines
+3574
to
3577
| { | ||
| "rule": "self.geneva.logs.adminCertSan == '' || self.geneva.logs.adminCertSan == self.geneva.logs.adminCertName + '.' + self.geneva.logs.adminCertificateDomain", | ||
| "message": "geneva.logs.adminCertSan must equal geneva.logs.adminCertName + '.' + geneva.logs.adminCertificateDomain" | ||
| } |
geoberle
force-pushed
the
create-certificate-manage
branch
from
778eeb0 to
1fe79fa
Compare
geoberle
force-pushed
the
create-certificate-manage
branch
from
1fe79fa to
f608a3c
Compare
Collaborator
Author
|
/hold |
Comment on lines
+270
to
272
| contentType: x-pkcs12 | ||
| manage: Enabled | ||
| certificateIssuer: Self |
| name: genevaAction | ||
| issuer: OneCertV2-PrivateCA | ||
| manage: true | ||
| san: "" |
Comment on lines
+365
to
+367
| contentType: x-pkcs12 | ||
| environment: Test | ||
| manageCertificates: true | ||
| manage: Enabled |
Comment on lines
+365
to
+367
| contentType: x-pkcs12 | ||
| environment: Test | ||
| manageCertificates: true | ||
| manage: Enabled |
Comment on lines
+3580
to
3591
| { | ||
| "rule": "self.firstPartyAppCertificate.san == '' || self.firstPartyAppCertificate.san == self.firstPartyAppCertificate.name + '.' + self.dns.svcParentZoneName", | ||
| "message": "firstPartyAppCertificate.san must equal firstPartyAppCertificate.name + '.' + dns.svcParentZoneName" | ||
| }, | ||
| { | ||
| "rule": "self.geneva.actions.certificate.san == '' || self.geneva.actions.certificate.san == self.geneva.actions.certificate.name + '.' + self.dns.globalCertificatesDomain", | ||
| "message": "geneva.actions.certificate.san must equal geneva.actions.certificate.name + '.' + dns.globalCertificatesDomain" | ||
| }, | ||
| { | ||
| "rule": "self.geneva.logs.adminCertSan == '' || self.geneva.logs.adminCertSan == self.geneva.logs.adminCertName + '.' + self.geneva.logs.adminCertificateDomain", | ||
| "message": "geneva.logs.adminCertSan must equal geneva.logs.adminCertName + '.' + geneva.logs.adminCertificateDomain" | ||
| } |
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.
What
managefield toCreateCertificatepipeline steps, allowing pipelines to conditionally skip certificate creation based on anEnabled/Disabledconfig valuesvc-cluster.bicepto aCreateCertificatepipeline step insvc-pipeline.yamlgeneva-identities.bicepto aCreateCertificatepipeline step inglobal-pipeline.yaml/global-pipeline-stg.yamlsvc-cluster.bicepandmgmt-cluster.biceptoCreateCertificatepipeline steps insvc-pipeline.yamlandmgmt-pipeline.yamlmgmt-cluster.bicepto aCreateCertificatepipeline step inmgmt-pipeline.yamlkeyCredentialsfrom the Geneva Actions Entra app — SNI is the only auth path for MSFT envs, dev doesn't need authmanageCertificates(bool) config field, replaced bymanage(string enum)Details
Upstream dependency: Bumps
ARO-Tools/pipelinesto pick up theManage *Valuefield onCreateCertificateStep(Azure/ARO-Tools#261).Pipeline runtime (
tooling/templatize): ExtractedshouldSkipCertificateStepfunction. Whenmanageis set, the step resolves its value from config and either proceeds (Enabled), skips with a log message (Disabled), or errors on invalid values. Whenmanageis nil (default), the step proceeds unconditionally — backward compatible.FPA certificate migration: The FPA certificate was previously created inside
svc-cluster.bicepvia akey-vault-cert.bicepmodule, gated by amanageFpaCertificateboolean parameter. Extracted into a standalonefpa-certCreateCertificatepipeline step, matching the pattern of existing cert steps (frontend-cert,admin-api-cert,sessiongate-cert). No outputs from the FPA cert module were consumed downstream.Geneva Actions certificate migration: The Geneva Actions certificate was created inside
geneva-identities.bicepviakey-vault-cert.bicep, gated bygenevaActionsManageCertificates. Extracted to ageneva-actions-certstep in bothglobal-pipeline.yamlandglobal-pipeline-stg.yaml. All cert output consumption (PublicKey,Thumbprint,KeyIdentifier,NotBefore,NotAfter) and thekeyCredentialswiring to the Entra app are removed. SNI is the only auth mechanism for MSFT envs; dev does not need to authenticate to the app. WhenuseSNIis false, the app has no auth mechanism — if dev ever needs it, the certificate must be attached through another mechanism (the public key cannot be read from ARM/bicep).Geneva logs admin certificate migration: The Geneva logs admin certificate was created inside
geneva-identities.bicepviakey-vault-cert-with-access.bicep. Extracted to ageneva-logs-certstep in both global pipelines. Thegeneva-identities.biceptemplate is now reduced to only the Entra app registration — all cert-related params, theev2MSIresource,globalKeyVaultName, and thecsvToArrayimport are removed.Geneva RP/cluster logs certificate migration: The svc-cluster and mgmt-cluster Geneva RP logs certs (and mgmt-cluster cluster logs cert) were created via
key-vault-cert-with-access.bicep, which bundles cert creation with a secret access role assignment. The cert creation is extracted to pipeline steps (geneva-rp-logs-cert,geneva-cluster-logs-cert). The secret access grants are preserved as standalonekeyvault-secret-access.bicepmodules — these are needed at runtime by the AKS CSI secrets provider to read the cert secrets. This matches the existing pattern used byfrontend-cert,admin-api-cert, andsessiongate-cert.Config changes:
firstPartyAppCertificate.manage:boolean→stringenum (Disabled). New fieldssan,contentType.geneva.actions.certificate.manage:boolean→stringenum (Enabled). New fieldssan,contentType.geneva.logs.manage: newstringenum (Enabled/Disabled), replacesmanageCertificates(bool, removed). New fieldcontentTypeatgeneva.logslevel.geneva.logs.certificateDomain,geneva.logs.certificateIssuer: removed from bicep params (no longer consumed by bicep), still in config for CEL validation and pipeline step refs.certificateManagedefinition (Enabled/Disabled), referenced by all cert configs.name + svcParentZoneName), Geneva Actions (name + globalCertificatesDomain), and Geneva logs admin (adminCertName + adminCertificateDomain).Ref: ARO-28053
Why
Testing
Testing is required for feature completion and tests should be part of the pull
request along with the feature changes.
Describe the testing provided. If you did not add tests, provide a clear
justification.
Special notes for your reviewer
PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.