Merge 1.3.0 to main#79
Merged
Merged
Conversation
* feat: `x509certificate2` removal (#71) * Update generated docs * chore(lint): Fix PR review lint. * Update generated docs * test: unit tests for SeparateChain/IncludeCertChain conflict resolution in JobBase Adds StorePropertiesParsingTests covering the four flag combinations so that the override logic (SeparateChain forced to false when IncludeCertChain=false) is caught at the unit level, not only by integration tests. * Update generated docs --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
github-actions
Bot
added
documentation
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. License Issues.github/workflows/dotnet-security-scan.yml
.github/workflows/secret-scanning.yml
.github/workflows/unit-tests.yml
OpenSSF ScorecardScorecard details
Scanned Files
|
Integration Test Results (K8s v1.29.0)152 tests 152 ✅ 3m 19s ⏱️ Results for commit c2edf79. |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Merges release 1.3.0 changes into main, adding a comprehensive test suite (unit + integration), expanding docs/manifests for updated store behaviors, and introducing CI/security GitHub workflows to validate and scan the repo.
Changes:
- Added extensive unit/integration tests plus test helpers for certificate/key-format scenarios and K8S CSR inventory modes.
- Updated manifests and documentation to reflect K8SCert cluster-wide CSR inventory, IncludeCertChain limitations, and deprecated properties.
- Added GitHub Actions workflows for tests, quality gates, dependency/security scanning, SBOM generation, and issue templates.
Reviewed changes
Copilot reviewed 65 out of 114 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| kubernetes-orchestrator-extension.Tests/K8SCertStoreTests.cs | Adds unit tests around CSR status handling, parsing, and inventory mode logic. |
| kubernetes-orchestrator-extension.Tests/Jobs/StorePropertiesParsingTests.cs | Adds tests validating SeparateChain/IncludeCertChain conflict resolution behavior. |
| kubernetes-orchestrator-extension.Tests/Jobs/CertificateFormatTests.cs | Adds certificate format/key/chain parsing tests (DER/PEM/PKCS12). |
| kubernetes-orchestrator-extension.Tests/Integration/K8SCertStoreIntegrationTests.cs | Adds K8SCert integration tests for single vs cluster-wide CSR inventory and discovery. |
| kubernetes-orchestrator-extension.Tests/Integration/IntegrationTestBase.cs | Introduces shared integration base with namespace isolation and cleanup helpers. |
| kubernetes-orchestrator-extension.Tests/Integration/Fixtures/IntegrationTestFixture.cs | Adds integration fixture for kubeconfig loading, client creation, and PAM resolver mocking. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8STLSSecrCollection.cs | Adds xUnit collection fixture for K8STLSSecr integration tests. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8SSecretCollection.cs | Adds xUnit collection fixture for K8SSecret integration tests. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8SPKCS12Collection.cs | Adds xUnit collection fixture for K8SPKCS12 integration tests. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8SNSCollection.cs | Adds xUnit collection fixture for K8SNS integration tests. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8SJKSCollection.cs | Adds xUnit collection fixture for K8SJKS integration tests. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8SClusterCollection.cs | Adds xUnit collection fixture for K8SCluster integration tests. |
| kubernetes-orchestrator-extension.Tests/Integration/Collections/K8SCertCollection.cs | Adds xUnit collection fixture for K8SCert integration tests. |
| kubernetes-orchestrator-extension.Tests/Helpers/KeyTypeTestData.cs | Provides shared Theory test data for supported key types to reduce duplication. |
| kubernetes-orchestrator-extension.Tests/Helpers/CertificateTestHelper.cs | Adds certificate/key/keystore/CSR generation helpers for tests. |
| kubernetes-orchestrator-extension.Tests/Helpers/CachedCertificateProvider.cs | Adds thread-safe caching for expensive certificate generation during tests. |
| kubernetes-orchestrator-extension.Tests/Attributes/SkipUnlessTheoryAttribute.cs | Adds env-var-gated Theory attribute for integration/conditional tests. |
| kubernetes-orchestrator-extension.Tests/Attributes/SkipUnlessAttribute.cs | Adds env-var-gated Fact attribute for integration/conditional tests. |
| integration-manifest.json | Updates store property metadata (K8SCert inventory modes, deprecations, IncludeCertChain notes). |
| docsource/k8stlssecr.md | Fixes naming references and documents storepath/alias patterns. |
| docsource/k8ssecret.md | Fixes naming references and documents storepath/alias patterns. |
| docsource/k8spkcs12.md | Documents supported key types and updates default discovery patterns. |
| docsource/k8sns.md | Clarifies K8SNS overview and improves discovery/config sections. |
| docsource/k8sjks.md | Documents supported key types and corrects discovery text. |
| docsource/k8scluster.md | Clarifies K8SCluster overview wording. |
| docsource/k8scert.md | Adds detailed K8SCert inventory modes, config matrix, and limitations. |
| docsource/content.md | Updates overall docs and adds supported key type matrix. |
| TestConsole/Program.cs | Comments out sample serialization XML snippet (keeps as reference). |
| TESTING_QUICKSTART.md | Adds a quickstart guide for running tests/coverage and CI expectations. |
| TESTING.md | Adds a comprehensive testing guide covering unit/integration/coverage/limitations. |
| MAKEFILE_GUIDE.md | Documents Makefile targets for testing, debugging, and workflow parity. |
| Keyfactor.Orchestrators.K8S.sln | Adds test project and solution folders; expands configuration platforms. |
| CHANGELOG.md | Adds 1.3.0 release notes (features/fixes/chores). |
| .github/workflows/unit-tests.yml | Adds unit test workflow with matrix (.NET 8/10), coverage, and artifacts. |
| .github/workflows/secret-scanning.yml | Adds TruffleHog secret scanning workflow. |
| .github/workflows/sbom-generation.yml | Adds CycloneDX SBOM generation workflow and release attachment. |
| .github/workflows/pr-quality-gate.yml | Adds PR quality gate including semantic title validation and keyword scans. |
| .github/workflows/license-compliance.yml | Adds dependency license reporting workflow. |
| .github/workflows/integration-tests.yml | Adds kind-based integration test workflow and cleanup/diagnostics. |
| .github/workflows/dotnet-security-scan.yml | Adds NuGet vulnerability checks and outdated package reporting. |
| .github/workflows/dependency-submission.yml | Adds dependency graph submission workflow. |
| .github/workflows/dependency-review.yml | Adds PR dependency review workflow (vulns + license checks). |
| .github/workflows/code-quality.yml | Adds format/analyzer-based code quality checks. |
| .github/workflows/autochangelog.yml | Removes disabled/legacy autochangelog workflow stub. |
| .github/workflows/README.md | Adds documentation for CI workflows and artifacts/troubleshooting. |
| .github/labeler.yml | Adds PR auto-labeling rules by file patterns/branch naming. |
| .github/kind-config.yaml | Adds minimal kind cluster config for integration tests. |
| .github/dependabot.yml | Expands dependabot config (actions, gomod, nuget + grouping/labels). |
| .github/WORKFLOWS_SUMMARY.md | Adds high-level summary of security/quality workflows and settings. |
| .github/SETUP_COMPLETE.md | Adds “setup complete” checklist/details for GHAS + templates/workflows. |
| .github/SECURITY_WORKFLOWS.md | Adds detailed documentation for security/quality workflows. |
| .github/ISSUE_TEMPLATE/security_vulnerability.yml | Adds security vulnerability issue form template. |
| .github/ISSUE_TEMPLATE/feature_request.yml | Adds feature request issue form template. |
| .github/ISSUE_TEMPLATE/documentation.yml | Adds documentation/question issue form template. |
| .github/ISSUE_TEMPLATE/config.yml | Configures issue templates and support/security contact links. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Adds bug report issue form template. |
Comments suppressed due to low confidence (1)
docsource/k8sjks.md:1
- Correct typo: 'toy' → 'you'.
## Overview
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+92
| - name: Validate PR title | ||
| uses: amannn/action-semantic-pull-request@v5 | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| types: | | ||
| feat | ||
| fix | ||
| docs | ||
| style | ||
| refactor | ||
| perf | ||
| test | ||
| chore | ||
| ci | ||
| requireScope: false | ||
| subjectPattern: ^[A-Z].+$ |
There was a problem hiding this comment.
This workflow enforces Conventional Commit PR titles, but the current PR title ("Merge 1.3.0 to main") will fail the check, which can block release-merge PRs. Consider exempting merge/release PRs (e.g., guard the step with an if: that skips titles starting with "Merge" or skips for PRs created by automation/bots), or broaden the allowed title patterns to include your release merge convention.
Comment on lines
+137
to
+155
| - name: Check for prohibited keywords | ||
| run: | | ||
| # Check for common placeholder/debug keywords that shouldn't be committed | ||
| prohibited_keywords=( | ||
| "TODO" | ||
| "FIXME" | ||
| "HACK" | ||
| "XXX" | ||
| "debugger" | ||
| "console.log" | ||
| ) | ||
|
|
||
| found_issues=false | ||
| for keyword in "${prohibited_keywords[@]}"; do | ||
| if git diff origin/main...HEAD | grep -i "$keyword"; then | ||
| echo "::warning::Found prohibited keyword: $keyword" | ||
| found_issues=true | ||
| fi | ||
| done |
There was a problem hiding this comment.
git diff origin/main...HEAD assumes origin/main is present in the checkout. On PR workflows, actions/checkout may not fetch the base branch ref, which can cause this step to fail or behave unexpectedly. Prefer diffing against the PR base SHA/ref provided by GitHub (e.g., ${{ github.event.pull_request.base.sha }}) or explicitly git fetch origin ${{ github.base_ref }} before diffing.
Comment on lines
+78
to
+80
| try { job.PublicInitializeStore(config); } catch { /* expected */ } | ||
|
|
||
| // Assert |
There was a problem hiding this comment.
Catching all exceptions here can mask unexpected failures (and also makes it unclear whether an exception was actually thrown as intended). Prefer using Record.Exception(...) and asserting it is non-null (or asserting a specific exception type/message), or catching only the expected exception type(s) from kubeconfig parsing/client creation.
Suggested change
| try { job.PublicInitializeStore(config); } catch { /* expected */ } | |
| // Assert | |
| var exception = Record.Exception(() => job.PublicInitializeStore(config)); | |
| // Assert | |
| Assert.NotNull(exception); |
| // Arrange | ||
| var csrName = $"test-single-approved-{Guid.NewGuid():N}"; | ||
| await CreateTestCsr(csrName, approve: true); | ||
| await Task.Delay(2000); // Wait for certificate to be issued |
There was a problem hiding this comment.
Fixed delays in integration tests are prone to CI flakiness (clusters vary in how quickly CSRs become issued). A more reliable approach is to poll the CSR resource until Status.Certificate is populated (or a timeout is reached), which reduces intermittent failures and unnecessary waiting when issuance is fast.
| await CreateTestCsr(approvedCsr1, approve: true); | ||
| await CreateTestCsr(approvedCsr2, approve: true); | ||
| await CreateTestCsr(pendingCsr, approve: false); | ||
| await Task.Delay(2000); // Wait for certificates to be issued |
There was a problem hiding this comment.
Fixed delays in integration tests are prone to CI flakiness (clusters vary in how quickly CSRs become issued). A more reliable approach is to poll the CSR resource until Status.Certificate is populated (or a timeout is reached), which reduces intermittent failures and unnecessary waiting when issuance is fast.
| // Arrange | ||
| var csrName = $"test-no-pk-cw-{Guid.NewGuid():N}"; | ||
| await CreateTestCsr(csrName, approve: true); | ||
| await Task.Delay(2000); |
There was a problem hiding this comment.
Fixed delays in integration tests are prone to CI flakiness (clusters vary in how quickly CSRs become issued). A more reliable approach is to poll the CSR resource until Status.Certificate is populated (or a timeout is reached), which reduces intermittent failures and unnecessary waiting when issuance is fast.
| /// <summary> | ||
| /// Gets or sets the name of the environment variable to check. | ||
| /// </summary> | ||
| public string EnvironmentVariable { get; set; } |
There was a problem hiding this comment.
With nullable reference types enabled, EnvironmentVariable being non-nullable but uninitialized will produce warnings, and returning null from Skip can also produce nullability warnings depending on xUnit's annotations. Consider initializing EnvironmentVariable to string.Empty (matching SkipUnlessTheoryAttribute), and align the Skip override's nullability with the base property (or use return null!; explicitly if the base signature is non-nullable but xUnit treats null as 'do not skip').
| { | ||
| } | ||
|
|
||
| public override string Skip |
There was a problem hiding this comment.
With nullable reference types enabled, EnvironmentVariable being non-nullable but uninitialized will produce warnings, and returning null from Skip can also produce nullability warnings depending on xUnit's annotations. Consider initializing EnvironmentVariable to string.Empty (matching SkipUnlessTheoryAttribute), and align the Skip override's nullability with the base property (or use return null!; explicitly if the base signature is non-nullable but xUnit treats null as 'do not skip').
| **Purpose:** Comprehensive unit testing across .NET versions | ||
|
|
||
| **What it does:** | ||
| - Runs all 134 unit tests |
There was a problem hiding this comment.
These hard-coded test counts appear inconsistent with the counts documented elsewhere in this PR (e.g., TESTING.md / TESTING_QUICKSTART.md). Consider removing exact numbers, deriving them from dotnet test --list-tests, or wording this as 'runs all unit tests / integration tests' to avoid the docs going stale.
|
|
||
| **What it does:** | ||
| - Creates kind (Kubernetes in Docker) cluster with K8s v1.29 | ||
| - Runs all 55 integration tests |
There was a problem hiding this comment.
These hard-coded test counts appear inconsistent with the counts documented elsewhere in this PR (e.g., TESTING.md / TESTING_QUICKSTART.md). Consider removing exact numbers, deriving them from dotnet test --list-tests, or wording this as 'runs all unit tests / integration tests' to avoid the docs going stale.
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.
Merge release-1.3 to main - Automated PR