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

📖 Adds a cluster controller document #1390

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented Sep 5, 2019

Signed-off-by: Chuck Ha chuckh@vmware.com

What this PR does / why we need it:
This PR adds some formalization around what the cluster controller is responsible for and the contracts it defines within its code.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #1370

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 5, 2019
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved
docs/book/src/architecture/controllers/cluster.md Outdated Show resolved Hide resolved

| secret name | field name | content |
|:---:|:---:|:---:|
|`<cluster-name>-kubeconfig`|none required|none required|
Copy link
Member

Choose a reason for hiding this comment

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

The filed name should be value and the content should be a base64 encoded kubeconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's worth adding that check to ClusterReconciler.reconcileKubeconfigOr do you think it's enough to look for a properly named secret? Right now that function is only checking for the existence of the secret.

But yes, this is the expected value, I'll definitely add these changes, thanks

Copy link
Member

Choose a reason for hiding this comment

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

It should definitely error today if the value isn't found, I can dig up the code later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only checks for the existence of the secret, not the contents.

_, err := secret.Get(r.Client, cluster, secret.Kubeconfig)
switch {
case apierrors.IsNotFound(err):
return kubeconfig.CreateSecret(ctx, r.Client, cluster)
case err != nil:
return errors.Wrapf(err, "failed to retrieve Kubeconfig Secret for Cluster %q in namespace %q", cluster.Name, cluster.Namespace)
}
return nil

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/assign @detiber

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2019

* `ready` - a boolean field that is true when the infrastructure is ready to be used.
* `apiEndpoints` - a slice of strings that identifies each control plane node's apiserver endpoint or a slice with only
one endpoint that is a load balancer for all control plane nodes.
Copy link
Member

Choose a reason for hiding this comment

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

Should ErrorMessage and ErrorReason also be listed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah indeed it should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon further inspection these are not required, but I will add another section outlining how these are used.

Signed-off-by: Chuck Ha <chuckh@vmware.com>
@chuckha
Copy link
Contributor Author

chuckha commented Sep 24, 2019

@vincepri @detiber I believe this is good to go pending any more feedback. Will update the machine controller document in a similar way

@detiber
Copy link
Member

detiber commented Sep 24, 2019

/lgtm

I think we can iterate on any additional changes after merge.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2019
@k8s-ci-robot k8s-ci-robot merged commit 427fc72 into kubernetes-sigs:master Sep 24, 2019
@chuckha chuckha deleted the cluster-doc branch September 24, 2019 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants