eks: cluster version update #3123

bonifaido · 2020-08-19T14:19:03Z

Q	A
Bug fix?	no
New feature?	yes
API breaks?	no
Deprecations?	no
Related tickets	partially #3094
License	Apache 2.0

What's in this PR?

Basic implementation of EKS cluster version update.

Why?

Additional context

Testable until the CLI implementation becomes available with:

curl -k -X 'PUT' -H 'Accept: application/json' \
-H "Authorization: Bearer ${PIPELINE_API_TOKEN}" \
-H 'Content-Type: application/json' \
https://127.0.0.1:9090/pipeline/api/v1/orgs/1/clusters/1/update \
-d '{"version":"1.17"}'

Checklist

Implementation tested (with at least one cloud provider)
Code meets the Developer Guide
OpenAPI and Postman files updated (if needed)

pregnor

There is only 1 required change among my comments, the cluster status error message handling, otherwise it looks generally good with a couple of questions/remarks from me to discuss.

src/cluster/eks_update_cluster_version.go

internal/cluster/distribution/eks/eksprovider/workflow/update_eks_version_activity.go

internal/cluster/distribution/eks/eksprovider/workflow/wait_update_eks_version_activity.go

internal/cluster/distribution/eks/eksprovider/workflow/save_cluster_version_activity.go

src/cluster/eks_update_cluster_version.go

pregnor

My requested changes were implemented.

The topic of the proper separation of the cluster version and node pool upgrade flows are still up for discussion, but even if we decide to change the location of separation/branching we can implement it in another PR if we wish.
(It can still be discussed and implemented here, I'm just not sure this PR is the right way to do it, maybe it calls for a separate discussion or an ADR proposal.)

.gen/pipeline/api/openapi.yaml

apis/pipeline/pipeline.yaml

sagikazarmark · 2020-08-28T07:54:33Z

apis/pipeline/pipeline.yaml

+            security:
+                - bearerAuth: []
+            tags:
+                - cluster-update


Can it be clusters instead?

I guess it would collide with the existing cluster update operation, but I don't care about that much. We don't have a BC promise for the generated code.

fixed, thanks.

apis/pipeline/pipeline.yaml

sagikazarmark · 2020-08-28T07:58:10Z

apis/pipeline/pipeline.yaml

@@ -7503,6 +7527,13 @@ components:
                    type: string
                    example: 10.20.30.40

+        ClusterUpdateRequest:


Can we make it look like the node pool upgrade request?

The nodepool update request looks a bit bad to me, it has cloud or distribution for example. Or do you mean the EKS part?

I mean this:

UpdateNodePoolRequest: oneOf: - $ref: '#/components/schemas/EksUpdateNodePoolRequest'

We should have a similar structure for clusters IMHO.

pregnor · 2020-08-28T08:19:13Z

apis/pipeline/pipeline.yaml

+                202:
+                    description: Accepted
+                default:
+                    $ref: '#/components/responses/Error'


Note: this is out of scope for this PR, but we should update the the 409 Conflict error and message here, because it refers to the secret by default.
https://github.com/banzaicloud/pipeline/pull/3123/files#diff-2b6bfc5b5d040867ca5e5f7b1df191f1R11868-R11869

Also we should update the 403 Access Denied message at the default values, because it seems to be misphrased You are unauthorized to perform this action access resource..
https://github.com/banzaicloud/pipeline/pull/3123/files#diff-2b6bfc5b5d040867ca5e5f7b1df191f1R11908-R11909

apis/pipeline/pipeline.yaml

sagikazarmark · 2020-08-28T08:54:17Z

apis/pipeline/pipeline.yaml

+            security:
+                - bearerAuth: []
+            tags:
+                - cluster-update


I guess it would collide with the existing cluster update operation, but I don't care about that much. We don't have a BC promise for the generated code.

sagikazarmark · 2020-08-28T08:56:16Z

internal/cluster/distribution/eks/eksprovider/workflow/eks.go

+
+// EKSAPIFactory provides an interface for instantiating AWS
+// EKS API objects.
+type EKSAPIFactory interface {


Is this a cadence library thing? We should keep in mind that we want to generate these, so we should follow the patterns that we set out in the closed repo.

This is just a wrapper to not to rely directly on the AWS lib, for mocking. @pregnor am I right?

That's correct, we use it for test mocking purposes to be able to provide a mocked API in place of an eksiface.EKSAPI, we did it similarly with the CloudFormation factories and client/service interfaces.

Context for people out of loop: It helps the Cadence activities to have a constructor argument/field for factories returning client/service interfaces to the corresponding AWS APIs so we don't use eks.New(awsSession) because it makes test mocking much more difficult as it creates a real API object, this way we can use a EKSFactoryMock{} object in place of the real factory wrapping the New() call which allows us to write extensive mock tests as we can control the returned API mock object and thus intercept/mock all API calls.

sagikazarmark · 2020-08-28T08:58:04Z

internal/cluster/distribution/eks/eksworkflow/activity_update_cluster_version.go

+	"github.com/banzaicloud/pipeline/internal/cluster/distribution/eks/eksprovider/workflow"
+)
+
+const UpdateClusterVersionActivityName = "eks-update-version"


I guess this is fine for now, but I'd much rather use the proxy pattern we used for other AWS API calls.

sagikazarmark

LGTM

Signed-off-by: Nandor Kracser <[email protected]>

Co-authored-by: Márk Sági-Kazár <[email protected]>

bonifaido self-assigned this Aug 19, 2020

bonifaido added distribution/eks Amazon EKS kind/enhancement New feature or request labels Aug 19, 2020

bonifaido force-pushed the eks-update-version branch 2 times, most recently from e77691f to b4d7c70 Compare August 24, 2020 07:28

bonifaido marked this pull request as ready for review August 24, 2020 07:38

bonifaido requested review from sagikazarmark, pregnor and sancyx August 24, 2020 07:44

bonifaido force-pushed the eks-update-version branch 2 times, most recently from a07c1dc to 3fc6511 Compare August 24, 2020 11:13

pregnor requested changes Aug 24, 2020

View reviewed changes

bonifaido force-pushed the eks-update-version branch from 9ed819e to ee4b64b Compare August 25, 2020 10:37

pregnor approved these changes Aug 25, 2020

View reviewed changes

bonifaido marked this pull request as draft August 26, 2020 07:31

bonifaido force-pushed the eks-update-version branch from bbfbfa8 to e3423eb Compare August 26, 2020 12:22

bonifaido marked this pull request as ready for review August 26, 2020 12:32

bonifaido force-pushed the eks-update-version branch 3 times, most recently from 6daa273 to 0f925a7 Compare August 26, 2020 13:13

pregnor approved these changes Aug 27, 2020

View reviewed changes

.gen/pipeline/api/openapi.yaml Show resolved Hide resolved

.gen/pipeline/api/openapi.yaml Show resolved Hide resolved

apis/pipeline/pipeline.yaml Show resolved Hide resolved

bonifaido force-pushed the eks-update-version branch from 0f925a7 to 270b051 Compare August 28, 2020 06:27

sagikazarmark requested changes Aug 28, 2020

View reviewed changes

pregnor reviewed Aug 28, 2020

View reviewed changes

sagikazarmark reviewed Aug 28, 2020

View reviewed changes

bonifaido force-pushed the eks-update-version branch 3 times, most recently from 88fcd76 to ec16b63 Compare August 28, 2020 11:10

sagikazarmark approved these changes Aug 28, 2020

View reviewed changes

bonifaido force-pushed the eks-update-version branch from ec16b63 to 16fc98a Compare August 28, 2020 11:40

bonifaido and others added 2 commits August 28, 2020 13:44

eks: add update cluster version workflow

3400a5d

Signed-off-by: Nandor Kracser <[email protected]>

update openapi descriptions

3d1b72b

Co-authored-by: Márk Sági-Kazár <[email protected]>

bonifaido force-pushed the eks-update-version branch from 16fc98a to 3d1b72b Compare August 28, 2020 11:45

pregnor approved these changes Aug 28, 2020

View reviewed changes

bonifaido merged commit c880cb6 into master Aug 28, 2020

bonifaido deleted the eks-update-version branch August 28, 2020 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eks: cluster version update #3123

eks: cluster version update #3123

bonifaido commented Aug 19, 2020 •

edited

Loading

pregnor left a comment

pregnor left a comment

sagikazarmark Aug 28, 2020

sagikazarmark Aug 28, 2020

bonifaido Aug 28, 2020

sagikazarmark Aug 28, 2020

bonifaido Aug 28, 2020

sagikazarmark Aug 28, 2020

pregnor Aug 28, 2020 •

edited

Loading

sagikazarmark Aug 28, 2020

sagikazarmark Aug 28, 2020

bonifaido Aug 28, 2020

pregnor Aug 28, 2020 •

edited

Loading

sagikazarmark Aug 28, 2020

sagikazarmark left a comment

eks: cluster version update #3123

eks: cluster version update #3123

Conversation

bonifaido commented Aug 19, 2020 • edited Loading

What's in this PR?

Why?

Additional context

Checklist

pregnor left a comment

Choose a reason for hiding this comment

pregnor 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

pregnor Aug 28, 2020 • edited Loading

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

pregnor Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

bonifaido commented Aug 19, 2020 •

edited

Loading

pregnor Aug 28, 2020 •

edited

Loading

pregnor Aug 28, 2020 •

edited

Loading