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

Add cloud config for tags #152

Merged
merged 1 commit into from Nov 10, 2020

Conversation

nicolehanjing
Copy link
Member

@nicolehanjing nicolehanjing commented Nov 5, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add cloud config (CRD) for tags
Structure:

├── hack
│   ├── boilerplate.go.txt
│   ├── tools.go
│   ├── update-codegen.sh
|   ├── verify-codegen.sh
└── pkg
    ├── apis
    │   └── config
    │       └── v1alpha1
    │           ├── doc.go
    │           ├── register.go
    │           ├── types.go
    │           └── zz_generated.deepcopy.go
    |       ├── doc.go
    |       ├── register.go
    └── generated
        ├── clientset ... 
        ├── informers ... 
        └── listers ... 

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add cloud config for tags

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 5, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 5, 2020
@nicolehanjing nicolehanjing force-pushed the nicoleh-cloudconfig branch 2 times, most recently from 8867558 to 08d8000 Compare November 5, 2020 21:19
hack/tools.go Show resolved Hide resolved
config:v1alpha1 \
--output-base "$(dirname "${BASH_SOURCE[0]}")/../../.." \
--go-header-file "${SCRIPT_ROOT}"/hack/boilerplate.go.txt

Copy link
Member

Choose a reason for hiding this comment

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

remove extra new lines at the bottom here


// GroupName is the group name used in this package
const (
GroupName = "aws.k8s.io"
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if the API here should be specific to configs, so something like aws.config.k8s.io. I also think we want to avoid api groups ending in k8s.io unless it's an official kubernetes API.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking it should be config.aws.io @nckturner any opinions here? @nckturner @randomvariable @justinsb any opinions here?

func addKnownTypes(scheme *runtime.Scheme) error {
scheme.AddKnownTypes(SchemeGroupVersion,
&AWSCloudConfig{},
&AWSCloudConfigList{},
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the List kind here

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// AWSCloudConfigList is a list of AWSCloudConfig resources
type AWSCloudConfigList struct {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the list type

// AWSCloudConfigSpec is the custom spec for an AWSCloudConfigresource
type AWSCloudConfigSpec struct {
// Tags is a list of tags storing configurations
Tags []Tag `json:"tags"`
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if instead of explcitly calling out tags we just name this field clusterName (type string). And we document the semantics of cluster name and how it's used in the various controllers.

Copy link
Member Author

Choose a reason for hiding this comment

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

that sounds reasonable to me, will update!

# --output-base because this script should also be able to run inside the vendor dir of
# k8s.io/kubernetes. The output-base is needed for the generators to output into the vendor dir
# instead of the $GOPATH directly. For normal projects this can be dropped.
bash "${CODEGEN_PKG}"/generate-groups.sh "deepcopy,client,informer,lister" \
Copy link
Member

Choose a reason for hiding this comment

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

I think we can skip generating client, informer and lister, we just need deepcopy I think

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, will update!

@nicolehanjing nicolehanjing force-pushed the nicoleh-cloudconfig branch 2 times, most recently from a4bc1d4 to 51b4384 Compare November 6, 2020 19:08
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 6, 2020

// GroupName is the group name used in this package
const (
GroupName = "config.aws.io"
Copy link
Member Author

Choose a reason for hiding this comment

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

changed the group name to "config.aws.io" for now, but would love to get opinions from @nckturner @randomvariable @justinsb :)

// AWSCloudConfigSpec is the custom spec for an AWSCloudConfigresource
type AWSCloudConfigSpec struct {
// ClusterName is the name of the cluster as presented to kube-controller-manager
ClusterName string `json:"clustername"`
Copy link
Member

Choose a reason for hiding this comment

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

clusterName

Copy link
Member Author

Choose a reason for hiding this comment

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

That will give me an error message "struct field clusterName has json tag but is not exported"
Or do we want to remove the tag here?

clusterName string 

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I should have clarified, the go field should be ClusterName, but the json field should be clusterName, not clustername.


// AWSCloudConfigSpec is the custom spec for an AWSCloudConfigresource
type AWSCloudConfigSpec struct {
// ClusterName is the name of the cluster as presented to kube-controller-manager
Copy link
Member

Choose a reason for hiding this comment

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

// clusterName is the name of the cluster as presented to cloud-controller-manager

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// +genclient
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this since we're not generating clients?

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha!

@nicolehanjing nicolehanjing force-pushed the nicoleh-cloudconfig branch 2 times, most recently from 58c33ca to 9de2353 Compare November 6, 2020 23:45
@nicolehanjing nicolehanjing changed the title [WIP] Add cloud config for tags Add cloud config for tags Nov 6, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2020
@andrewsykim
Copy link
Member

andrewsykim commented Nov 7, 2020

@nicolehanjing I think we're missing the internal types

EDIT: nevermind, we don't need internal types in this case

@nicolehanjing nicolehanjing force-pushed the nicoleh-cloudconfig branch 4 times, most recently from f3e56d7 to e4ff6b7 Compare November 9, 2020 23:28
type AWSCloudConfig struct {
metav1.TypeMeta `json:",inline"`

Spec AWSCloudConfigSpec `json:"spec"`
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinkg we should name this field config instead, spec is more for declaring desired state/behavior https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status


// AWSCloudConfigSpec is the custom spec for an AWSCloudConfigresource
type AWSCloudConfigSpec struct {
// ClusterName is the name of the cluster as presented to cloud-controller-manager
Copy link
Member

Choose a reason for hiding this comment

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

In this PR or the next tagging PR, we should add docs here for how the clusterName is used in tags

Copy link
Member Author

Choose a reason for hiding this comment

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

updated! I added the following comments here

pkg/apis/config/doc.go Show resolved Hide resolved
}

// AWSCloudConfigSpec is the custom spec for an AWSCloudConfigresource
type AWSCloudConfigSpec struct {
Copy link
Member

Choose a reason for hiding this comment

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

Remove Spec from this name

Copy link
Member

Choose a reason for hiding this comment

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

Or just name this CloudConfig, I think the "AWS" part is redundant since it is under AWSCloudConfig already

Copy link
Member

Choose a reason for hiding this comment

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

Or just Config would be fine I think


// AWSCloudConfigSpec is the custom spec for an AWSCloudConfigresource
type AWSCloudConfigSpec struct {
// ClusterName is the name of the cluster as presented to cloud-controller-manager
Copy link
Member

Choose a reason for hiding this comment

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

// clusterName is the name of the cluster and should be unique in any given AWS account. 
// This name will be used when naming AWS resources such as load balancers. It is also
//  expected as tag values for every AWS resource that represents this cluster. The expected
//  tagging format is:
//
//      kubernetes.io/cluster=<clusterName>
//  
//  Resources without this tag will not be seen by the AWS cloud provider. Changing the cluster name is not supported.

(or something like this)

Copy link
Member Author

Choose a reason for hiding this comment

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

wow, thanks for the detailed docs, will update!

pkg/apis/config/v1alpha1/types.go Outdated Show resolved Hide resolved

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// AWSCloudConfig stores the provided configuration for a cloud provider.
Copy link
Member

Choose a reason for hiding this comment

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

// AWSCloudConfig represents configuration information that can be passed into the AWS v2 cloud provider.

Copy link
Member

Choose a reason for hiding this comment

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

s/can be/will be

// ClusterName is the name of the cluster as presented to cloud-controller-manager
// We support two tags:
// 1) kubernetes.io/cluster/=<clusterName>
// 2) for shared resoueces between clusters, kubernetes.io/cluster/<clusterName>=""
Copy link
Member

Choose a reason for hiding this comment

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

We don't support this (yet)

}

// Config contains configuration information read by the AWS v2 cloud provider
type Config struct {
Copy link
Member

Choose a reason for hiding this comment

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

I change my mind again, I think this should be named AWSConfig, sorry about that.

Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

Few more nits, overall looking good

var SchemeGroupVersion = schema.GroupVersion{Group: config.GroupName, Version: "v1alpha1"}

// Kind takes an unqualified kind and returns back a Group qualified GroupKind
func Kind(kind string) schema.GroupKind {
Copy link
Member

Choose a reason for hiding this comment

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

Let's re-add this later when we actually need it.

}

// Resource takes an unqualified resource and returns a Group qualified GroupResource
func Resource(resource string) schema.GroupResource {
Copy link
Member

Choose a reason for hiding this comment

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

Same here -- let's re-add this later when we need it

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, updated!

pkg/apis/config/v1alpha1/types.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

I think this is a reasonable starting point for the config API. @justinsb @nckturner @randomvariable I would like to do a more formal API review on this at some point once it's more complete (but before we remove the alpha gate). Thanks @nicolehanjing

@@ -0,0 +1,48 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

Should this script be running somewhere in our CI? We should add it in a follow-up PR

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I think so, created a PR #153

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, nicolehanjing

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 Nov 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit b390ec1 into kubernetes:master Nov 10, 2020
@nicolehanjing nicolehanjing deleted the nicoleh-cloudconfig branch November 10, 2020 05:00
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants