[Helped]so many TODO in kops/pkg/ , can we solve some of them?

tanjunchen commented 4 years ago

There are so many TODO in kops , I guess that we can solve or improve some of them. I open an issue to record them. Here are TODO under /pkg folder.

If the following TODO does not exist, I hope you can reply this TODO in here, so I can update this list in time. Or you have solved one of the TODOs and hope you can associate this issue for easy tracking.

grep -rs --exclude-dir="vendor/"   "TODO"  | wc -l
767

[ ] pkg/apis/kops/validation/legacy.go: // TODO: Auto choose zones from region?
[ ] pkg/apis/kops/validation/legacy.go:// @TODO semvar package doesn't appear to ignore a 'v' in v1.1.1 should could be a problem later down the line
[ ] pkg/apis/kops/validation/aws.go:// TODO: make image validation smarter? graduate from jessie to stretch? This is quick and dirty because we keep getting reports
[ ] pkg/apis/kops/validation/aws.go: // TODO: how can we put this list somewhere better?
[ ] pkg/apis/kops/util/versions.go:// TODO: Convert to our own KubernetesVersion type?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha1/cluster.go: // TODO #3070
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused TLSCertFile
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused TLSPrivateKeyFile
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Better type ?
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused BasicAuthFile
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused ClientCAFile
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused TLSCertFile
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused TLSPrivateKeyFile
[ ] pkg/apis/kops/v1alpha1/componentconfig.go: // TODO: Remove unused TokenAuthFile
[ ] pkg/apis/kops/cluster.go: // TODO #3070
[ ] pkg/apis/kops/cluster.go: // TODO move this into networking.go :(
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/zz_generated.conversion.go: // TODO: Inefficient conversion - can we improve it?
[ ] pkg/apis/kops/v1alpha2/keyset.go: // TODO: Move CA to use these values
[ ] pkg/apis/kops/v1alpha2/cluster.go: // TODO #3070
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused TLSCertFile
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused TLSPrivateKeyFile
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Better type ?
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused BasicAuthFile
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused ClientCAFile
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused TLSCertFile
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused TLSPrivateKeyFile
[ ] pkg/apis/kops/v1alpha2/componentconfig.go: // TODO: Remove unused TokenAuthFile
[ ] pkg/apis/kops/v1alpha2/CHANGES.md:TODO
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused TLSCertFile
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused TLSPrivateKeyFile
[ ] pkg/apis/kops/componentconfig.go: // TODO: Better type ?
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused BasicAuthFile
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused ClientCAFile
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused TLSCertFile
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused TLSPrivateKeyFile
[ ] pkg/apis/kops/componentconfig.go: // TODO: Remove unused TokenAuthFile
[ ] pkg/apis/kops/install/roundtrip_test.go: // TODO: Reenable
[ ] pkg/apis/kops/instancegroup.go: // MountOptions is a collection of mount options - @TODO need to be added
[ ] pkg/apis/nodeup/config.go: // TODO: Remove once everything is in containers?
[ ] pkg/acls/gce/storage.go: // TODO: Cache?
[ ] pkg/acls/gce/storage.go: // TODO: Cache?
[ ] pkg/assets/builder.go: // TODO we'd like to use cloudup.Phase here, but that introduces a go cyclic dependency
[ ] pkg/assets/builder.go:// TODO - remove this method as CNI does now have a SHA file
[ ] pkg/validation/validate_cluster_test.go: // TODO assert nodes contains all the nodes in the mock kubernetes.Interface?
[ ] pkg/kubemanifest/manifest.go: // TODO: Support more separators?
[ ] pkg/nodeidentity/aws/identify.go: // TODO: Should we traverse to the ASG to confirm the tags there?
[ ] pkg/instancegroups/instancegroups.go: // TODO should remove the need to have rollingupdate struct and add:
[ ] pkg/instancegroups/instancegroups.go: // TODO - the kubernetes client
[ ] pkg/instancegroups/instancegroups.go: // TODO - the cluster name
[ ] pkg/instancegroups/instancegroups.go: // TODO - the client config
[ ] pkg/instancegroups/instancegroups.go: // TODO - fail on validate
[ ] pkg/instancegroups/instancegroups.go: // TODO - fail on drain
[ ] pkg/instancegroups/instancegroups.go: // TODO - cloudonly
[ ] pkg/instancegroups/instancegroups.go: // TODO check more values in cloudGroup that they are set properly
[ ] pkg/instancegroups/instancegroups.go:// TODO: Temporarily increase size of ASG?
[ ] pkg/instancegroups/instancegroups.go:// TODO: Remove from ASG first so status is immediately updated?
[ ] pkg/instancegroups/instancegroups.go:// TODO: Batch termination, like a rolling-update
[ ] pkg/instancegroups/instancegroups.go: // TODO: Leaving func in place in order to cordon and drain nodes
[ ] pkg/instancegroups/rollingupdate.go: // TODO: Bail on error?
[ ] pkg/instancegroups/delete.go: // TODO should we drain nodes and validate the cluster?
[ ] pkg/instancegroups/rollingupdate_test.go: // TODO: Replace with a virtual clock?
[ ] pkg/instancegroups/rollingupdate_test.go: // TODO: Replace with a virtual clock?
[ ] pkg/drain/drain.go: // TODO(justinsb): unnecessary?
[ ] pkg/drain/drain.go: // TODO(justinsb): unnecessary?
[ ] pkg/drain/default.go: // TODO(justinsb): Ensure we have adequate e2e coverage of this function in library consumers
[ ] pkg/drain/default.go: // TODO(justinsb): Ensure we have adequate e2e coverage of this function in library consumers
[ ] pkg/client/simple/vfsclientset/clientset.go: // TODO: offer an option not to delete backups?
[ ] pkg/client/simple/vfsclientset/cluster.go: // TODO: Split this out into real version updates / schema changes
[ ] pkg/kopscodecs/codecs.go: // TODO: Cache?
[ ] pkg/dns/gossip.go:// TODO: Are .local names necessarily invalid for "real DNS"? Do we need more qualification here?
[ ] pkg/model/gcemodel/api_loadbalancer.go: // TODO: Health check
[ ] pkg/model/gcemodel/api_loadbalancer.go: // TODO: I don't love this technique for finding the task by name & modifying it
[ ] pkg/model/gcemodel/autoscalinggroup.go: // TODO: Support preemptible nodes?
[ ] pkg/model/gcemodel/autoscalinggroup.go: // TODO: Duplicated from aws - move to defaults?
[ ] pkg/model/gcemodel/autoscalinggroup.go: // TODO: Switch to regional managed instance group
[ ] pkg/model/gcemodel/external_access.go: klog.Warningf("TODO: Harmonize gcemodel ExternalAccessModelBuilder with awsmodel")
[ ] pkg/model/gcemodel/firewall.go: klog.Warningf("TODO: Harmonize gcemodel with awsmodel for firewall - GCE model is way too open")
[ ] pkg/model/gcemodel/firewall.go: //// TODO: Is this a good idea?
[ ] pkg/model/context.go: // TODO: Remove copy-pasting c.f. https://github.com/kubernetes/kops/blob/master/pkg/model/components/context.go#L32
[x] pkg/model/tests/data/bootstrapscript_1.txt: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/tests/data/bootstrapscript_1.txt: # TODO: Remove?
[x] pkg/model/tests/data/bootstrapscript_0.txt: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/tests/data/bootstrapscript_0.txt: # TODO: Remove?
[x] pkg/model/tests/data/bootstrapscript_2.txt: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/tests/data/bootstrapscript_2.txt: # TODO: Remove?
[x] pkg/model/tests/data/bootstrapscript_3.txt: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/tests/data/bootstrapscript_3.txt: # TODO: Remove?
[x] pkg/model/tests/data/bootstrapscript_4.txt: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/tests/data/bootstrapscript_4.txt: # TODO: Remove?
[x] pkg/model/tests/data/bootstrapscript_5.txt: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/tests/data/bootstrapscript_5.txt: # TODO: Remove?
[ ] pkg/model/alimodel/api_loadbalancer.go: // TODO: Health check
[ ] pkg/model/alimodel/api_loadbalancer.go: // TODO: I don't love this technique for finding the task by name & modifying it
[ ] pkg/model/alimodel/policy_builder.go: // TODO: Make optional only if using autoscalers
[ ] pkg/model/iam/iam_builder.go:// TODO: We have a couple different code paths until we do lifecycles, and
[ ] pkg/model/iam/iam_builder.go:// TODO: when we have a cluster or refactor some s3 code. The only code that
[ ] pkg/model/iam/iam_builder.go:// TODO: is not shared by the different path is the s3 / state store stuff.
[ ] pkg/model/iam/iam_builder.go:// TODO: Initial work has been done to lock down IAM actions based on resources
[ ] pkg/model/iam/iam_builder.go:// TODO: and condition keys, but this can be extended further (with thorough testing).
[ ] pkg/model/iam/iam_builder.go:// TODO: Extend to support Condition Keys
[ ] pkg/model/iam/iam_builder.go:// UseBootstrapTokens check if we are using bootstrap tokens - @TODO, i don't like this we should probably pass in
[ ] pkg/model/iam/iam_builder.go: // TODO - I think we can just have GetAuthorizationToken here, as we are not
[ ] pkg/model/iam/iam_builder.go: // TODO - making any API calls except for GetAuthorizationToken.
[ ] pkg/model/iam/iam_builder.go: // TODO: Route53 currently not supported in China, need to check and fail/return
[ ] pkg/model/iam/iam_builder.go: // TODO could use "kms:ViaService" Condition Key here?
[ ] pkg/model/iam/iam_builder.go: // TODO: Make optional only if using autoscalers
[ ] pkg/model/iam/iam_builder.go: // TODO: Make optional only if using IAM SSL Certs on ELBs
[ ] pkg/model/bootstrapscript.go: // TODO double check that all the code does this
[ ] pkg/model/bootstrapscript.go: // TODO move this into a validate so we can enforce the string syntax
[ ] pkg/model/iam.go: // TODO: I don't love this technique for finding the task by name & modifying it
[ ] pkg/model/master_volumes.go: // TODO: Should no longer be needed because we trim prefixes
[ ] pkg/model/master_volumes.go: // // TODO: If we're still struggling for size, we don't need to put ourselves in the allmembers list
[ ] pkg/model/names.go: // TODO: Support this (arbitrary choice I think, for ELBs)
[ ] pkg/model/names.go: // TODO: Support this
[x] pkg/model/components/context.go: // TODO remove this, as it is an addon now
[x] pkg/model/components/context.go: // TODO: Once we are shipping different versions, start to use them
[x] pkg/model/components/context.go: // TODO path.Join here?
[ ] pkg/model/components/kubescheduler.go: // TODO: No way to set to 0?
[ ] pkg/model/components/docker.go: // TODO(justinsb): figure out whether to use overlay2 on AWS jessie:
[ ] pkg/model/components/kubelet.go: // TODO: Some people recommend 250Mi, but this would hurt small machines
[ ] pkg/model/components/etcdmanager/model.go: // TODO: We need this to match the backup base (currently)
[ ] pkg/model/components/etcdmanager/model.go: # TODO: Would be nice to reduce these permissions; needed for volume mounting
[ ] pkg/model/components/etcdmanager/model.go: # TODO: Would be nice to scope this more tightly, but needed for volume mounting
[ ] pkg/model/components/etcdmanager/model.go: // TODO: pull from bundle
[ ] pkg/model/components/etcdmanager/model.go: // TODO: Use a socket file for the quarantine port
[ ] pkg/model/components/etcdmanager/model.go: // @TODO: This is hacky, but we want it so that we can have a different internal & external name
[ ] pkg/model/components/etcdmanager/model.go: // TODO: We need to wire these into the etcd-manager spec
[ ] pkg/model/components/etcdmanager/model.go: // TODO: Use helper function here
[ ] pkg/model/components/kubeproxy.go: // TODO: No way to set to 0?
[ ] pkg/model/components/apiserver.go: // TODO: We can probably rewrite these more clearly in descending order
[ ] pkg/model/pki.go: // TODO: Only create the CA via this task
[ ] pkg/model/pki.go: // TODO: Can this be "server" now that we're not using it for peer connectivity?
[ ] pkg/model/pki.go: // @TODO this is VERY presumptuous, i'm going on the basis we can make it configurable in the future.
[ ] pkg/model/openstackmodel/servergroup.go: // TODO: I don't love this technique for finding the task by name & modifying it
[ ] pkg/model/openstackmodel/servergroup.go: // TODO: I don't love this technique for finding the task by name & modifying it
[ ] pkg/model/openstackmodel/firewall.go: //TODO: This is the default port for kubelet and may be overridden
[ ] pkg/model/bastion.go: // TODO: Could we get away without an ELB here? Tricky to fix if dns-controller breaks though...
[ ] pkg/model/awsmodel/api_loadbalancer.go: // TODO: I don't love this technique for finding the task by name & modifying it
[ ] pkg/model/awsmodel/autoscalinggroup.go: // @TODO check if there any a better way of doing this .. initially I had a type LaunchTemplate which included
[ ] pkg/model/awsmodel/autoscalinggroup_test.go: // TODO: Mock cloudprovider
[ ] pkg/model/dns.go: // TODO: We can now rationalize the code paths
[ ] pkg/model/network.go: // TODO: would be good to create these as shared, to verify them
[ ] pkg/model/network.go: // TODO: Validate when allSubnetsShared
[ ] pkg/model/network.go: // TODO: validate even if shared?
[x] pkg/model/resources/nodeup.go: # TODO(zmerlynn): Now we REALLY have no excuse not to do the reboot
[ ] pkg/model/resources/nodeup.go: # TODO: Remove?
[ ] pkg/model/firewall.go: // TODO: We need to remove the ALL rule
[ ] pkg/model/firewall.go: // TODO: Remove, replace with etcd in calico manifest
[ ] pkg/model/firewall.go: // TODO: Make less hacky
[ ] pkg/model/firewall.go: // TODO: Fix management - we need a wildcard matcher now
[ ] pkg/model/firewall.go: // TODO: Remove, replace with etcd in calico manifest
[ ] pkg/model/firewall.go: // TODO: UDP vs TCP
[ ] pkg/model/firewall.go: // TODO: Protocol 4 for calico
[ ] pkg/pki/privatekey.go: // TODO: Do we need this? I think we need this only on nodeup, but maybe we could just not base64-it?
[ ] pkg/pki/certificate.go: // TODO: Do we need this? I think we need this only on nodeup, but maybe we could just not base64-it?
[ ] pkg/resources/openstack/dns.go: // TODO: not tested and this should have retry similar to what we have in another resources
[ ] pkg/resources/digitalocean/dns/dns.go: // TODO (andrewsykim): pagination in ListOptions
[ ] pkg/resources/gce/gce.go: // TODO: Only zones in api.Cluster object, if we have one?
[ ] pkg/resources/gce/gce.go: // TODO: Find routes via instances (via instance groups)
[ ] pkg/resources/gce/gce.go: // TODO: Push down tag filter?
[ ] pkg/resources/gce/gce.go: // TODO: Push-down prefix?
[ ] pkg/resources/gce/gce.go: // TODO: When shared resource PR lands, reintroduce
[ ] pkg/resources/ops/delete.go: // TODO: Some form of default ordering based on types?
[ ] pkg/resources/spotinst/resources.go:// TODO(liran): We should fetch Ocean's instances using a query param of ?launchSpecId=foo,
[ ] pkg/resources/aws/securitygroup.go: // TODO: Move to a "pre-execute" phase?
[ ] pkg/resources/aws/filters.go: // TODO: We could look for tag-key on the old & new tags, and then post-filter (we do this in k/k cloudprovider)
[ ] pkg/resources/aws/aws.go: // TODO: We need to match both the name and a prefix
[ ] pkg/resources/aws/aws.go: // TODO: usee 'Filters: []*ec2.Filter{awsup.NewEC2Filter("key-name", keypairName)},'
[ ] pkg/resources/aws/aws.go: // TODO: If we have the zone id in the cluster spec, use it!
[ ] pkg/resources/aws/aws.go: // TODO: Compute the actual set of names?
[ ] pkg/resources/ali/ali.go: // TODO: Should we delete the group softly? Like set ForceDelete to false.
[ ] pkg/resources/ali/ali.go: // TODO: Should we check the tags of loadbalancer?

bittopaz commented 4 years ago

Great initiative!

rifelpet commented 4 years ago

Agreed! Just to make the list a bit shorter, I notice we can ignore any of the matches under docs/apireference/build and any of the Binary file ____ matches too. Some of the markdown files matched but dont have the actual todo text in your list (like docs/tutorial/working-with-instancegroups.md), possibly because they're in markdown comments <!-- --!> which the GH issue will treat as a comment itself?

Beyond that I think we could triage many of these based on the most commonly used packages and create individual GH issues for them, perhaps labeling them with good-starter-issue,

tanjunchen commented 4 years ago

/help /good-starter-issue

k8s-ci-robot commented 4 years ago

@tanjunchen: This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes/kops/issues/8238): >/help >/good-starter-issue Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

tanjunchen commented 4 years ago

Because this issue is a bit big, I will consider listing TODO as a package and excluding some unnecessary files.If possible, I will separate them into several issues .Thank you for your suggestions. @rifelpet

rifelpet commented 4 years ago

Agreed, we can add good-starter-issue to smaller issues that we separate from this big issue.

tanjunchen commented 4 years ago

@rifelpet I have ignore some files . next , how to separate them?welcome to suggestions.

johngmyers commented 4 years ago

Since I'm trying to make substantial changes to pkg/instancegroups, I'd appreciate if people could hold off soliciting low-value conflicting PRs there for now.

ryan-dyer-sp commented 4 years ago

RE: tokenauthfile removal . Removing of this flag is required by CIS benchmarks. I see kops is still setting this flag and the value (known_tokens.csv) does contain entries. So is this safe to actually remove? I'm happy to submit a PR to rip it out, but I don't know anything about this from an actual code and usage perspective.

Nilubkal commented 4 years ago

@ryan-dyer-sp i was dealing with the exact same issue ( reported by CIS benchmarks). It seems that its hard-coded here nodeup/pkg/model/kube_apiserver.go

288 // buildPod is responsible for generating the kube-apiserver pod and thus manifest file
289 func (b *KubeAPIServerBuilder) buildPod() (*v1.Pod, error) {
290   kubeAPIServer := b.Cluster.Spec.KubeAPIServer
291   kubeAPIServer.ClientCAFile = filepath.Join(b.PathSrvKubernetes(), "ca.crt")
292   kubeAPIServer.TLSCertFile = filepath.Join(b.PathSrvKubernetes(), "server.cert")
293   kubeAPIServer.TLSPrivateKeyFile = filepath.Join(b.PathSrvKubernetes(), "server.key")
294   kubeAPIServer.TokenAuthFile = filepath.Join(b.PathSrvKubernetes(), "known_tokens.csv")

Is it foreseen a removal of the flag in future releases ? I was not able to disable it by modifying tokenAuthFile under the kubeAPIServer when i deploy the cluster via a template.

fejta-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

kubernetes / kops

[Helped]so many TODO in kops/pkg/ , can we solve some of them? #8238