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

yaml/v2: apply the namespace uniformly to the resource name #2897

Merged
merged 6 commits into from
Mar 21, 2024

Conversation

EronWright
Copy link
Contributor

@EronWright EronWright commented Mar 20, 2024

Proposed changes

This PR strengthens the normalization of the parsed Kubernetes objects to better interoperate with #2881.
Specifically:

  1. Applies the default namespace as necessary on each object, so that they may be reliably targeted by the depends-on resource.
  2. Stabilizes the resource id (e.g. default/my-map versus my-map).
  3. Validates that the objects are well-formed more eagerly than during Register.

For example, given the below manifest and assuming that the default namespace is default, we would expect a DependsOn option from Certificate to Issuer. Observe that namespace isn't explicitly set on the issuer.

name: issue-2870-cert-manager
runtime: yaml
resources:
  example:
    type: kubernetes:yaml/v2:ConfigGroup
    properties:
      yaml: |
        apiVersion: cert-manager.io/v1
        kind: Issuer
        metadata:
          name: test-selfsigned
        spec:
          selfSigned: {}
        ---
        apiVersion: cert-manager.io/v1
        kind: Certificate
        metadata:
          name: selfsigned-cert
          annotations:
            config.kubernetes.io/depends-on: cert-manager.io/namespaces/default/Issuer/test-selfsigned
        spec:
          dnsNames:
            - example.com
          secretName: selfsigned-cert-tls
          issuerRef:
            name: test-selfsigned

To apply the default namespace correctly, one must know whether a given kind is namespace-scoped. The provider naturally uses the discovery client, and also looks for a matching CRD amongst the parsed objects (since they're not yet installed).

Testing

New test cases were added for Normalize, and some test cases were moved from Register because the validation is performed more eagerly.

New sub-tests were added for IsNamespacedKind to cover the local CRD enhancement.

Related issues (optional)

Closes #2870

Copy link
Contributor

@rquitales rquitales left a comment

Choose a reason for hiding this comment

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

LGTM with 1 minor non-blocking feedback.

provider/pkg/clients/clients.go Outdated Show resolved Hide resolved
Base automatically changed from eronwright/issue-2881 to master March 21, 2024 21:44
@EronWright EronWright added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 21, 2024
@EronWright EronWright marked this pull request as ready for review March 21, 2024 21:57
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 87.30159% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 27.55%. Comparing base (e006410) to head (490d06e).

Files Patch % Lines
provider/pkg/provider/yaml/v2/yaml.go 85.71% 2 Missing and 2 partials ⚠️
provider/pkg/provider/yaml/v2/configfile.go 66.66% 1 Missing and 1 partial ⚠️
provider/pkg/provider/yaml/v2/configgroup.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2897      +/-   ##
==========================================
+ Coverage   27.19%   27.55%   +0.35%     
==========================================
  Files          53       53              
  Lines        7780     7818      +38     
==========================================
+ Hits         2116     2154      +38     
+ Misses       5486     5485       -1     
- Partials      178      179       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EronWright EronWright enabled auto-merge (squash) March 21, 2024 22:22
@EronWright EronWright merged commit 5b47b32 into master Mar 21, 2024
18 checks passed
@EronWright EronWright deleted the eronwright/issue-2870 branch March 21, 2024 22:53
@EronWright
Copy link
Contributor Author

EronWright commented Mar 22, 2024

Unfortunately there's a problem with this, in preview mode, the CRDs aren't actually installed and this breaks multi-CG use cases. The solution will either be to eat the error in preview mode, or to somehow resolve the CRDs across components. A follow-up PR:
#2904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yaml/v2: apply the namespace uniformly to the resource name
2 participants