Add DaemonSet and ClusterRole resources #229

chris-codaio · 2018-11-27T19:45:39Z

Brings in a couple of resources from #100

@terraform-providers/ecosystem

alexsomesan

Hi! This is really cool stuff!
Thank you for taking the time to put these two resources together.

I've looked over and there are a few comments here and there, mostly about aligning the codebase to a few standard practices we're trying to stick to.

Please have a look over my comments and in the mean time I will review the DaemonSet part of this PR.

Thanks again for all the hard work!

alexsomesan · 2018-11-28T23:06:57Z

kubernetes/resource_kubernetes_cluster_role.go

+	}
+	log.Printf("[INFO] Reading cluster role %s", name)
+	cRole, err := conn.RbacV1().ClusterRoles().Get(name, metav1.GetOptions{})
+	if err != nil {


Here, we need more elaborate handling of err.
In particular, in case the error is of a NotFound kind, the ID of the resource should be set to d.SetId("") and no error should be returned. This instructs Terraform to remove the resource from the state.
You can check the type of error returned in err by using these client-go helper functions: https://godoc.org/k8s.io/apimachinery/pkg/api/errors

alexsomesan · 2018-11-28T23:14:07Z

kubernetes/resource_kubernetes_cluster_role.go

+	}
+
+	log.Printf("[INFO] Updating cluster role %q: %v", name, cRole)
+	out, err := conn.RbacV1().ClusterRoles().Update(&cRole)


The .Update() method of client-go resources is actually not doing an in-place update, but rather a replace type of operation, meaning it replaces the entire API object with the new one passed in.
This does not map well to Terraform's expectations of an Update action, which should be an in-place modification of the existing resource.

Whenever this is not technically possible or not desired, the proper way to implement this in Terraform is to let it do a Delete/Create sequence. This is done by tagging attributes that cannot be updated with ForceNew: true.

If you do indeed want to implement the Update function, the changes to the resource should be done via .Patch() method of client-go. This takes in a list of JSONPatch style operations and this list is what the Terraform Update function should construct before calling .Patch()

alexsomesan · 2018-11-28T23:15:26Z

kubernetes/resource_kubernetes_cluster_role.go

+
+	log.Printf("[INFO] cluster role %s deleted", name)
+
+	d.SetId("")


Setting ID to "" is not actually needed in Delete functions. Terraform will do that automatically.

Nice - done.

alexsomesan · 2018-11-28T23:16:25Z

kubernetes/resource_kubernetes_cluster_role.go

+	log.Printf("[INFO] Checking cluster role %s", name)
+	_, err = conn.RbacV1().ClusterRoles().Get(name, metav1.GetOptions{})
+	if err != nil {
+		if statusErr, ok := err.(*errors.StatusError); ok && statusErr.ErrStatus.Code == 404 {


Can you please make use of the client-go error helpers I mentioned earlier, instead of the direct comparisons in this line?

alexsomesan · 2018-11-28T23:20:37Z

kubernetes/resource_kubernetes_cluster_role_test.go

+		resources  = ["pods", "pods/log"]
+		verbs = ["get", "list"]
+	}
+}`, name)


Please make sure all these inline HCL blocks are terraform fmt formatted and the last closing bracket of the resource is always followed by a new line character.

This is needed to we can eventually make use of a new linter / formatter in CI: https://github.com/katbyte/terrafmt

alexsomesan · 2018-11-28T23:23:17Z

kubernetes/resource_kubernetes_daemonset.go

+							Optional:    true,
+							ForceNew:    true,
+							MaxItems:    1,
+							Elem: &schema.Resource{


Can you please reuse the labelSelectorFields function for populating this Schema section?
You can find it in kubernetes/schema_label_selector.go

alexsomesan · 2018-11-28T23:24:59Z

kubernetes/resource_kubernetes_daemonset.go

+							Description: "An object that describes the pod that will be created. The DaemonSet will create exactly one copy of this pod on every node that matches the template's node selector (or on every node if no node selector is specified). More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#pod-template",
+							Required:    true,
+							MaxItems:    1,
+							Elem: &schema.Resource{


Same here, please reuse the podTemplateFields() function to populate this schema section.

The function is in kubernetes/schema_pod_template.go

chris-codaio · 2018-11-29T05:01:03Z

Thanks for the review! All current comments addressed with a new commit. PTAL when you can.

chris-codaio · 2018-11-30T17:05:30Z

@alexsomesan mind taking another look at this when you have a chance? I've addressed all of your comments.

alexsomesan · 2018-11-30T17:37:25Z

@chrisleck I'll try to review as soon as I get a chance.

chris-codaio · 2018-12-12T22:57:25Z

@alexsomesan - pinging to make sure this is still on your radar to review. Don't want to be pushy, but I do have a project blocked on this :)

RobMaskell · 2018-12-12T23:11:47Z

Just to add my $0.02, I'm not exactly blocked by this but I think I have a bug to report but as I'm currently provisioning the ClusterRole in an unconventional way I wanted to test with this first.

alexsomesan · 2018-12-12T23:30:22Z

@chrisleck I haven't forgotten you! I'll get to this PR shortly. I'm at KubeCon this week so operating at low capacity here.

chris-codaio · 2018-12-12T23:39:14Z

@alexsomesan - thanks for the update and enjoy the conference!

bukzor · 2018-12-22T01:15:27Z

I don't have enough context to know whether this matters for this PR, but I went and used the functionality added here and found two missing bits of API. volume.host_path.type is unsupported, as well as volume_mount.mount_propagation.

chris-codaio · 2019-01-10T00:09:30Z

alexsomesan · 2019-01-10T00:28:31Z

:) sure can haz! Apologies, I’ve been caught in provider migrations to TF 0.12. I will look at your changes first thing tomorrow morning (UTC+1).

…

On Thu 10. Jan 2019 at 01:09, Christopher Eck ***@***.***> wrote: <https://camo.githubusercontent.com/0ace7e8ff88fa7213d20d7c8726b7cc3c4d08c09/68747470733a2f2f692e63687a6267722e636f6d2f66756c6c2f373731303438353234382f684331393645323042> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABC-m-a3_7Ew7dkO_WU7LYLmBsUC86U8ks5vBoS7gaJpZM4Y2NWx> .

alexsomesan · 2019-01-10T15:09:56Z

@chrisleck, ~~are there any acceptance tests for the DaemonSet resource?~~ Nevermind, I found them. The diff view was folding them away.

alexsomesan · 2019-01-10T16:05:54Z

@chrisleck I'm running into a few small issues trying to test this.
First, I need to make this change to get it to build:

diff --git a/kubernetes/structures_daemonset.go b/kubernetes/structures_daemonset.go
index c553bf0..dfcae87 100644
--- a/kubernetes/structures_daemonset.go
+++ b/kubernetes/structures_daemonset.go
@@ -77,7 +77,7 @@ func expandDaemonSetSpec(daemonset []interface{}) (appsv1.DaemonSetSpec, error)
        if err != nil {
                return obj, err
        }
-       obj.Template = template
+       obj.Template = *template

        return obj, nil
 }

After that, I'm seeing this test failure:

~/workspace/terraform-provider-kubernetes(add-daemonset-and-clusterrole*) » TESTARGS="-run '^TestAccKubernetesClusterRole_basic'" make testacc                                        alex@alexs-macbook
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run '^TestAccKubernetesClusterRole_basic' -timeout 120m
?   	github.com/terraform-providers/terraform-provider-kubernetes	[no test files]
=== RUN   TestAccKubernetesClusterRole_basic
--- FAIL: TestAccKubernetesClusterRole_basic (0.46s)
    testing.go:538: Step 1 error: Error applying: 1 error occurred:
        	* kubernetes_cluster_role.test: 1 error occurred:
        	* kubernetes_cluster_role.test: Failed to update ClusterRole: invalid resource name "/tf-acc-test-alex-dv1z7k82d2": [may not contain '/']

FAIL
FAIL	github.com/terraform-providers/terraform-provider-kubernetes/kubernetes	1.182s
make: *** [testacc] Error 1
------------------------------------------------------------

chris-codaio · 2019-01-11T23:35:22Z

Hmm - not seeing that. I'll try re-syncing to head and make sure we're clean.

chris-codaio · 2019-01-12T03:07:36Z

Ah - turns out I had pushed an older branch :(

chris-codaio · 2019-01-12T03:07:57Z

Updated with a fresh sync to master and validated tests are running. PTAL :)

deedubs · 2019-01-16T15:25:00Z

This is great @chrisleck. With these landing, I believe we'll reach a tipping point where we can start to consider terraform a strong contender for managing core cluster state on k8s.

pdecat · 2019-01-16T15:41:22Z

kubernetes/resource_kubernetes_daemonset.go

+						},
+						"template": {
+							Type:        schema.TypeList,
+							Description: "An object that describes the pod that will be created. The DaemonSet will create exactly one copy of this pod on every node that matches the template's node selector (or on every node if no node selector is specified). More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#pod-template",


This description should probably link to the DaemonSet specific
https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/#pod-template documentation or to the more general pod documentation https://kubernetes.io/docs/concepts/workloads/pods/pod-overview/#pod-templates

chrisdewar · 2019-01-21T04:05:58Z

Would be good to get this merged if all issues have been addressed.

orkaa · 2019-01-22T10:02:31Z

Hi! Really looking forward to getting this merged since I'm just setting up a new cluster the terraform way. ❤️

How soon can I expect this to be released? I'm deciding if I want to wait or develop a workaround in the meantime.

Thanks!

alexsomesan

Acceptance tests are all green in CI.
This looks good overall.
I will merge and follow up with a few small touch-ups (removing redundant assignments like out := &appsv1.DaemonSet{})

Thanks for your good work!

jodersky · 2019-02-05T21:50:39Z

Awesome that this got merged! Is there already a release planned with these new features?

ranimufid · 2019-02-14T17:25:00Z

@alexsomesan, may we please know roughly when the upcoming release with this merged content is going to happen?

alexsomesan · 2019-02-14T17:32:26Z

I was hoping to get more of the pending changes in before a release.

But I can also do a release on Monday since this is something a lot of people are waiting for.

ranimufid · 2019-02-14T17:38:09Z

But I can also do a release on Monday since this is something a lot of people are waiting for.

That would be greatly appreciated @alexsomesan!

alexsomesan · 2019-02-14T17:40:56Z

Ok, will do :)

Natalique · 2019-02-18T16:33:09Z

Ok, will do :)

Thanks, can't wait! @alexsomesan

yacut · 2019-02-18T17:08:43Z

@alexsomesan I've already tested it. It works great. Thanks!

ghost added the size/XXL label Nov 27, 2018

alexsomesan reviewed Nov 28, 2018

View reviewed changes

chris-codaio force-pushed the add-daemonset-and-clusterrole branch from 40739ff to 93be6bc Compare November 29, 2018 05:04

RobMaskell mentioned this pull request Jan 2, 2019

Feature request: {Cluster,""}RoleBinding/{Cluster,""}Role support #30

Closed

chris-codaio added 2 commits January 11, 2019 15:36

Add DaemonSet and ClusterRole resources

1d1db50

Address feedback

5047a18

chris-codaio force-pushed the add-daemonset-and-clusterrole branch from 93be6bc to 5047a18 Compare January 12, 2019 03:07

ghost added the documentation label Jan 12, 2019

pdecat reviewed Jan 16, 2019

View reviewed changes

Update doc pointer

e7b9986

alexsomesan approved these changes Jan 22, 2019

View reviewed changes

alexsomesan merged commit f9354bd into hashicorp:master Jan 22, 2019

chris-codaio deleted the add-daemonset-and-clusterrole branch January 23, 2019 05:28

alexsomesan mentioned this pull request Jan 31, 2019

Feature request: support the creation of ClusterRole, Role and RoleBinding #205

Closed

Natalique mentioned this pull request Feb 19, 2019

kubernetes_cluster_role doesn't allow granting privileges #322

Closed

ghost locked and limited conversation to collaborators Apr 21, 2020


		log.Printf("[INFO] cluster role %s deleted", name)

		d.SetId("")

Add DaemonSet and ClusterRole resources #229

Add DaemonSet and ClusterRole resources #229

Conversation

chris-codaio commented Nov 27, 2018 • edited Loading

alexsomesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-codaio commented Nov 29, 2018

chris-codaio commented Nov 30, 2018

alexsomesan commented Nov 30, 2018

chris-codaio commented Dec 12, 2018

RobMaskell commented Dec 12, 2018

alexsomesan commented Dec 12, 2018

chris-codaio commented Dec 12, 2018

bukzor commented Dec 22, 2018

chris-codaio commented Jan 10, 2019

alexsomesan commented Jan 10, 2019 via email

alexsomesan commented Jan 10, 2019 • edited Loading

alexsomesan commented Jan 10, 2019

chris-codaio commented Jan 11, 2019

chris-codaio commented Jan 12, 2019

chris-codaio commented Jan 12, 2019

deedubs commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdewar commented Jan 21, 2019

orkaa commented Jan 22, 2019

alexsomesan left a comment

Choose a reason for hiding this comment

jodersky commented Feb 5, 2019 • edited Loading

ranimufid commented Feb 14, 2019

alexsomesan commented Feb 14, 2019

ranimufid commented Feb 14, 2019

alexsomesan commented Feb 14, 2019

Natalique commented Feb 18, 2019

yacut commented Feb 18, 2019

chris-codaio commented Nov 27, 2018 •

edited

Loading

alexsomesan commented Jan 10, 2019 •

edited

Loading

jodersky commented Feb 5, 2019 •

edited

Loading