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

ValidatingAdmissionPolicy resource support #2576

Draft
wants to merge 18 commits into
base: v3-major-release
Choose a base branch
from

Conversation

aayushsss1
Copy link
Contributor

Description

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Release Note

Release note for CHANGELOG:

...

References

Closes #2250

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@BBBmau
Copy link
Contributor

BBBmau commented Aug 29, 2024

Just a heads-up @aayushsss1 since we are currently working on migrating the provider over to the plugin framework we would actually prefer to have this be supported under plugin-framework instead of SDKv2. Let me know if you have any questions, thanks again for picking this up!

@aayushsss1
Copy link
Contributor Author

@BBBmau Understood! Is it very different compared to the earlier SDKv2?

@BBBmau
Copy link
Contributor

BBBmau commented Aug 29, 2024

@BBBmau Understood! Is it very different compared to the earlier SDKv2?

@aayushsss1 You'd want to include the resource in the internal/framework/provider directory.

You can see an example of how this is currently being tested on my fork here: https://github.com/BBBmau/terraform-provider-kubernetes/tree/add-codegen/internal/framework/provider

Some plugin-framework resource docs can also be found here: https://developer.hashicorp.com/terraform/plugin/framework/resources#add-resource-to-provider

While you work on it feel free to reach out for help!

@aayushsss1
Copy link
Contributor Author

@BBBmau Understood! Is it very different compared to the earlier SDKv2?

@aayushsss1 You'd want to include the resource in the internal/framework/provider directory.

You can see an example of how this is currently being tested on my fork here: https://github.com/BBBmau/terraform-provider-kubernetes/tree/add-codegen/internal/framework/provider

Some plugin-framework resource docs can also be found here: https://developer.hashicorp.com/terraform/plugin/framework/resources#add-resource-to-provider

While you work on it feel free to reach out for help!

@BBBmau I just have a small doubt, the metadata schema uses the earlier sdk v2, do I create a new separate metadata schema based on the plugin framework for the validating_admission_policy resource?

@BBBmau
Copy link
Contributor

BBBmau commented Sep 5, 2024

@BBBmau I just have a small doubt, the metadata schema uses the earlier sdk v2, do I create a new separate metadata schema based on the plugin framework for the validating_admission_policy resource?

@aayushsss1 I'm not sure what you're referring to when it comes to the earlier sdk v2, the metadata schema would be implemented the same as how we see it in the linked fork:
https://github.com/BBBmau/terraform-provider-kubernetes/blob/218df7e4bb7d233dd07b5053e089be75cd692f36/internal/framework/provider/appsv1/deployment_schema_gen.go#L25-L85

Let me know if the link answers your question, I could've missed something in regards to metadata schema uses the earlier sdkv2

@aayushsss1
Copy link
Contributor Author

@BBBmau I just have a small doubt, the metadata schema uses the earlier sdk v2, do I create a new separate metadata schema based on the plugin framework for the validating_admission_policy resource?

@aayushsss1 I'm not sure what you're referring to when it comes to the earlier sdk v2, the metadata schema would be implemented the same as how we see it in the linked fork: https://github.com/BBBmau/terraform-provider-kubernetes/blob/218df7e4bb7d233dd07b5053e089be75cd692f36/internal/framework/provider/appsv1/deployment_schema_gen.go#L25-L85

Let me know if the link answers your question, I could've missed something in regards to metadata schema uses the earlier sdkv2

Understood! I was referring to this earlier - https://github.com/hashicorp/terraform-provider-kubernetes/blob/main/kubernetes/schema_metadata.go

@aayushsss1
Copy link
Contributor Author

@BBBmau I've updated the schema in line with the plugin framework. Please have a look at the structure, and let me know if any changes have to be made, I'll get started with the crud and model in the mean time!

},
},
},
"spec": schema.SingleNestedAttribute{
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks to be good, I did notice that it's missing the status block, this can be seen from the api reference: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#validatingadmissionpolicy-v1-admissionregistration-k8s-io

Did you happen to use the code generation tool for this? Be sure to remove _gen from the end of all the files if you didn't use the generation tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed the files and I've added the status field!

go.mod Outdated
@@ -1,39 +1,39 @@
module github.com/hashicorp/terraform-provider-kubernetes

go 1.21
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason for the change go.mod and go.sum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BBBmau I've reduced the number of changes to the go.mod, the main change is I've added the github.com/hashicorp/terraform-plugin-codegen-kubernetes/, which in turn has updated the plugin framework and other terraform dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah the inclusion of github.com/hashicorp/terraform-plugin-codegen-kubernetes/ isn't necessary since it's a tool that you would install separately to help in generating plugin-framework resources.

You can go ahead and revert changes that came from adding it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But isn't github.com/hashicorp/terraform-plugin-codegen-kubernetes/autocrud required for the CRUD operations?

I had to also utilize it here -

type ValidatingAdmissionPolicy struct {
APIVersion string
Kind string
clientGetter autocrud.KubernetesClientGetter
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah you're right. Apologies!

It's also worth noting that after reviewing the issue further that that once this PR is complete it unfortunately won't be merged until v3.0.0 due to this resource being supported by default in k8s 1.30. We intend to do a major version bump in order to support this since we currently only support up to v1.28. I've added it as part of v3..0.0 milestone as of now.

Of course you can still work on it but wanted to let you know in case you felt that this was on a deadline. We appreciate you working on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks for letting me know! If there are any other issues that require immediate attention I'd be glad to help out!

@BBBmau
Copy link
Contributor

BBBmau commented Nov 14, 2024

just a heads-up, we'll be working on v3 major release soon if you'd like to continue working on this. All we ask is that you rebase so that it's on v3-major-release branch

If not it's no worries!

@aayushsss1
Copy link
Contributor Author

@BBBmau I'll continue my work on this, thanks for the heads up!

@BBBmau BBBmau changed the base branch from main to v3-major-release November 15, 2024 17:50
commit 095ee23cee2395859baef681f75331b88235aec4
Author: aayushsss1 <[email protected]>
Date:   Fri Nov 22 16:13:42 2024 -0500

    updated go mod and sum

commit 2de833a1cce9e27a8d981845c103d7b6374cf5ce
Author: aayushsss1 <[email protected]>
Date:   Fri Nov 22 16:01:19 2024 -0500

    changes reverted

    changelog updated

    updated changelog

    updated and rebased

    go mod updated

commit 625c52a
Author: aayushsss1 <[email protected]>
Date:   Fri Nov 22 16:08:43 2024 -0500

    updated and rebased

commit 50b0647
Author: aayushsss1 <[email protected]>
Date:   Fri Nov 22 16:07:30 2024 -0500

    updated changelog

commit 8fa05e2
Author: aayushsss1 <[email protected]>
Date:   Fri Nov 22 16:06:29 2024 -0500

    changelog updated

commit 9cbdc24
Author: aayushsss1 <[email protected]>
Date:   Fri Nov 22 16:01:19 2024 -0500

    changes reverted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add ValidatingAdmissionPolicy resource from k8s v1.28.0
4 participants