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

Setting resource_version on ClusterRole to string throws error: error: strconv.ParseUint: parsing "example-clusterrole": invalid syntax #2351

Closed
rshade opened this issue Apr 4, 2023 · 4 comments · Fixed by #2571
Assignees
Labels
area/providers area/sdk Related to the Kubernetes SDKs (nodejs, Python, etc.) customer/feedback Feedback from customers customer/lighthouse Lighthouse customer bugs and enhancements kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Milestone

Comments

@rshade
Copy link
Contributor

rshade commented Apr 4, 2023

What happened?

Setting resource_version on ClusterRole to string throws error: error: strconv.ParseUint: parsing "examle-clusterrole": invalid syntax. The type is set to str in https://github.com/pulumi/pulumi-kubernetes/blob/master/sdk/python/pulumi_kubernetes/meta/v1/_inputs.py#L1013

Expected Behavior

Unsure, either it should type check early or possibly a better error message.

Steps to reproduce

ClusterRole(
            resource_name="example-clusterrole",
            metadata=ObjectMetaArgs(
                resource_version="example-clusterrole",
                name="kube-system:example"
            ),
            rules=[
                PolicyRuleArgs(
                    api_groups=[""],
                    resources=["namespaces", "pods"],
                    verbs=["get", "list", "watch"],
                )
            ],
        )

Output of pulumi about

kubernetes 3.24.2

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@rshade rshade added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Apr 4, 2023
@kpitzen kpitzen added area/providers kind/enhancement Improvements or new features good-first-issue Start here if you'd like to start contributing to Pulumi and removed kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Apr 7, 2023
@kpitzen
Copy link
Contributor

kpitzen commented Apr 7, 2023

Hey @rshade ! Looking at this issue, it appears the API spec for K8s requires this to be a string input, but that input must also be parsable as an integer (which is unfortunate). I'd consider this an enhancement to existing behavior, rather than a but, but I agree we should improve the experience to make this more clear. What would your preferred behavior be? I'm not sure we can change the type annotations for this as an argument, as that would require special-casing at the schema layer, but maybe that's something we could explore. It would be a relatively smaller change to make the error message more clear (we're parsing from string to unsigned int, so adding prose to the effect of "Please provide a string which can be converted to an integer").

@rshade
Copy link
Contributor Author

rshade commented Apr 7, 2023

@kpitzen Yeah, its definitely some unfortunate kube behavior. I think if we throw the error during parsing, it should be fine, instead of when the kube client does it. Its very difficult from that error to discern where the problem is.

@lblackstone lblackstone removed the good-first-issue Start here if you'd like to start contributing to Pulumi label Jul 14, 2023
@lblackstone
Copy link
Member

The resourceVersion field should be read-only. We may need to update this type in our schema to reflect that.

An opaque value that represents the internal version of this object that can be used by clients to determine when objects have changed. May be used for optimistic concurrency, change detection, and the watch operation on a resource or set of resources. Clients must treat these values as opaque and passed unmodified back to the server. They may only be valid for a particular resource or set of resources. Populated by the system. Read-only. Value must be treated as opaque by clients

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.25/#objectmeta-v1-meta

@lblackstone lblackstone added area/sdk Related to the Kubernetes SDKs (nodejs, Python, etc.) kind/bug Some behavior is incorrect or out of spec and removed kind/enhancement Improvements or new features labels Jul 14, 2023
@mikhailshilkov mikhailshilkov added the customer/feedback Feedback from customers label Jul 26, 2023
@mikhailshilkov mikhailshilkov added this to the 0.93 milestone Aug 7, 2023
@mikhailshilkov mikhailshilkov modified the milestones: 0.93, 0.94 Sep 5, 2023
@EronWright
Copy link
Contributor

Related to the read-only aspect, bear in mind that the resource version is the basis for optimistic concurrency, and an update or patch request may specify it.

@rquitales rquitales assigned EronWright and unassigned rquitales Sep 18, 2023
@EronWright EronWright changed the title Setting resource_version on ClusterRole to string throws error: error: strconv.ParseUint: parsing "examle-clusterrole": invalid syntax Setting resource_version on ClusterRole to string throws error: error: strconv.ParseUint: parsing "example-clusterrole": invalid syntax Sep 18, 2023
@mnlumi mnlumi added the customer/lighthouse Lighthouse customer bugs and enhancements label Sep 19, 2023
EronWright added a commit that referenced this issue Sep 21, 2023
<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes
<!--Give us a brief description of what you've done and what it solves.
-->

This PR improves the handling of the read-only metadata fields of
Kubernetes objects, such as `creationTimestamp` and `resourceVersion`.
The provider now ignores these fields when they appear as resource
inputs, to address a few quirks:
1. Unparseable values do not cause an API Server error during preview.
2. A subsequent refresh does not show a difference to the `__inputs`
property.
3. The behavior is now uniform for standard resource types and for
custom resource types.

An integration test is provided to show that such fields are ignored as
inputs, and are still available as outputs. Manual tests confirmed
identical behavior for custom resource objects.

### Related issues (optional)
Closes #2351

Related:
- #2511
- #2445

<!--Refer to related PRs or issues: #1234, or 'Fixes #1234' or 'Closes
#1234'.
Or link to full URLs to issues or pull requests in other GitHub
repositories. -->
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/providers area/sdk Related to the Kubernetes SDKs (nodejs, Python, etc.) customer/feedback Feedback from customers customer/lighthouse Lighthouse customer bugs and enhancements kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants