Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

Commit

Permalink
Code review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
viovanov authored and Vlad Iovanov committed Dec 13, 2019
1 parent 326b2f4 commit e5c7d2c
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 43 deletions.
8 changes: 4 additions & 4 deletions pkg/bosh/converter/quarks_links.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ const (
VolumeLinksPath = "/var/run/secrets/links/"
)

// LinkInfo Specify link provider and its secret name from Kube native components
// LinkInfo specifies the link provider and its secret name from Kube native components
type LinkInfo struct {
SecretName string
ProviderName string
}

// LinkInfos a list of LinkInfo
// LinkInfos is a list of LinkInfo
type LinkInfos []LinkInfo

// Volumes return a list of volumes from LinkInfos
// Volumes returns a list of volumes from LinkInfos
func (q *LinkInfos) Volumes() []corev1.Volume {
volumes := []corev1.Volume{}
for _, l := range *q {
Expand All @@ -26,7 +26,7 @@ func (q *LinkInfos) Volumes() []corev1.Volume {
return volumes
}

// VolumeMounts return a list of volumeMounts from LinkInfos
// VolumeMounts returns a list of volumeMounts from LinkInfos
func (q *LinkInfos) VolumeMounts() []corev1.VolumeMount {
volumeMounts := []corev1.VolumeMount{}
for _, l := range *q {
Expand Down
19 changes: 8 additions & 11 deletions pkg/bosh/manifest/cmd_instance_group_resolver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,31 +176,31 @@ func (igr *InstanceGroupResolver) resolveManifest(initialRollout bool) error {
return nil
}

// CollectQuarksLinks collect all links from path
// CollectQuarksLinks collects all links from a directory specified by path
func (igr *InstanceGroupResolver) CollectQuarksLinks(linksPath string) error {
exist, err := afero.DirExists(igr.fs, linksPath)
if err != nil {
return errors.Wrapf(err, "could not check if a path exists")
return errors.Wrapf(err, "could not check if path '%s' exists", linksPath)
}
if !exist {
return nil
}

links, err := afero.ReadDir(igr.fs, linksPath)
if err != nil {
return errors.Wrapf(err, "could not read links directory")
return errors.Wrapf(err, "could not read links directory '%s'", linksPath)
}

quarksLinks, ok := igr.manifest.Properties["quarks_links"]
if !ok {
return fmt.Errorf("missing quarks_links")
return fmt.Errorf("missing quarks_links key in manifest properties")
}
qs, ok := quarksLinks.(map[string]interface{})
if !ok {
return fmt.Errorf("could not get a map of QuarksLink")
}

// Assumed we have secrets path and link secrets named as providerName
// Assume we have a list of files named as the provider names of a link
for _, l := range links {
if l.IsDir() {
linkName := l.Name()
Expand All @@ -218,7 +218,7 @@ func (igr *InstanceGroupResolver) CollectQuarksLinks(linksPath string) error {
properties := map[string]interface{}{}
properties[linkName] = map[string]interface{}{}
linkP := map[string]interface{}{}
err = afero.Walk(igr.fs, filepath.Clean(linksPath+"/"+l.Name()), func(path string, info os.FileInfo, err error) error {
err = afero.Walk(igr.fs, filepath.Clean(filepath.Join(linksPath, l.Name())), func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
Expand All @@ -239,14 +239,11 @@ func (igr *InstanceGroupResolver) CollectQuarksLinks(linksPath string) error {
return nil
})
if err != nil {
return errors.Wrapf(err, "Walking links path")
return errors.Wrapf(err, "walking links path")
}

properties[linkName] = linkP
err = igr.jobProviderLinks.AddExternalLink(linkName, linkType, q.Address, q.Instances, properties)
if err != nil {
return errors.Wrapf(err, "Collecting external link failed for %s", linkName)
}
igr.jobProviderLinks.AddExternalLink(linkName, linkType, q.Address, q.Instances, properties)
}
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/bosh/manifest/job_provider_links.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ func (jpl jobProviderLinks) Add(igName string, job Job, spec JobSpec, jobsInstan
return nil
}

// AddExternalLink add link info from external
func (jpl jobProviderLinks) AddExternalLink(linkName string, linkType string, linkAddress string, jobsInstances []JobInstance, properties JobLinkProperties) error {
// AddExternalLink adds link info from an external (non-BOSH) source
func (jpl jobProviderLinks) AddExternalLink(linkName string, linkType string, linkAddress string, jobsInstances []JobInstance, properties JobLinkProperties) {
if _, ok := jpl.links[linkType]; !ok {
jpl.links[linkType] = map[string]JobLink{}
}
Expand All @@ -117,5 +117,4 @@ func (jpl jobProviderLinks) AddExternalLink(linkName string, linkType string, li
Instances: jobsInstances,
Properties: properties,
}
return nil
}
4 changes: 2 additions & 2 deletions pkg/kube/controllers/boshdeployment/deployment_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func AddDeployment(ctx context.Context, config *config.Config, mgr manager.Manag

}

// Watch Services routing to external link provider
// Watch Services that route (select) pods that are external link providers
servicesPredicates := predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
service := e.Object.(*corev1.Service)
Expand Down Expand Up @@ -191,7 +191,7 @@ func AddDeployment(ctx context.Context, config *config.Config, mgr manager.Manag
}),
}, servicesPredicates)
if err != nil {
return errors.Wrapf(err, "Watching services failed in bosh deployment controller.")
return errors.Wrapf(err, "watching services failed in bosh deployment controller.")

}

Expand Down
42 changes: 21 additions & 21 deletions pkg/kube/controllers/boshdeployment/deployment_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (r *ReconcileBOSHDeployment) Reconcile(request reconcile.Request) (reconcil
}

return reconcile.Result{},
log.WithEvent(instance, "GetBOSHDeploymentError").Errorf(ctx, "Failed to get BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "GetBOSHDeploymentError").Errorf(ctx, "failed to get BOSHDeployment '%s': %v", request.NamespacedName, err)
}

if meltdown.NewWindow(r.config.MeltdownDuration, instance.Status.LastReconcile).Contains(time.Now()) {
Expand All @@ -98,64 +98,64 @@ func (r *ReconcileBOSHDeployment) Reconcile(request reconcile.Request) (reconcil
manifest, err := r.resolveManifest(ctx, instance)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "WithOpsManifestError").Errorf(ctx, "Failed to get with-ops manifest for BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "WithOpsManifestError").Errorf(ctx, "failed to get with-ops manifest for BOSHDeployment '%s': %v", request.NamespacedName, err)
}

// Get link infos containing provider name and its secret name
linkInfos, err := r.listLinkInfos(instance, manifest)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "InstanceGroupManifestError").Errorf(ctx, "Failed to listing link secrets for BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "InstanceGroupManifestError").Errorf(ctx, "failed to list quarks-link secrets for BOSHDeployment '%s': %v", request.NamespacedName, err)
}

// Apply the "with-ops" manifest secret
log.Debug(ctx, "Creating with-ops manifest secret")
err = r.createManifestWithOps(ctx, instance, *manifest)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "WithOpsManifestError").Errorf(ctx, "Failed to create with-ops manifest secret for BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "WithOpsManifestError").Errorf(ctx, "failed to create with-ops manifest secret for BOSHDeployment '%s': %v", request.NamespacedName, err)
}

log.Debug(ctx, "Rendering manifest")

// Apply the "Variable Interpolation" QuarksJob, which creates the desired manifest secret
qJob, err := r.jobFactory.VariableInterpolationJob(*manifest)
if err != nil {
return reconcile.Result{}, log.WithEvent(instance, "DesiredManifestError").Errorf(ctx, "Failed to build the desired manifest qJob: %v", err)
return reconcile.Result{}, log.WithEvent(instance, "DesiredManifestError").Errorf(ctx, "failed to build the desired manifest qJob: %v", err)
}

log.Debug(ctx, "Creating desired manifest QuarksJob")
err = r.createQuarksJob(ctx, instance, qJob)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "DesiredManifestError").Errorf(ctx, "Failed to create desired manifest qJob for BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "DesiredManifestError").Errorf(ctx, "failed to create desired manifest qJob for BOSHDeployment '%s': %v", request.NamespacedName, err)
}

// Apply the "Instance group manifest" QuarksJob, which creates instance group manifests (ig-resolved) secrets
qJob, err = r.jobFactory.InstanceGroupManifestJob(*manifest, linkInfos, instance.ObjectMeta.Generation == 1)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "InstanceGroupManifestError").Errorf(ctx, "Failed to build instance group manifest qJob: %v", err)
log.WithEvent(instance, "InstanceGroupManifestError").Errorf(ctx, "failed to build instance group manifest qJob: %v", err)
}

log.Debug(ctx, "Creating instance group manifest QuarksJob")
err = r.createQuarksJob(ctx, instance, qJob)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "InstanceGroupManifestError").Errorf(ctx, "Failed to create instance group manifest qJob for BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "InstanceGroupManifestError").Errorf(ctx, "failed to create instance group manifest qJob for BOSHDeployment '%s': %v", request.NamespacedName, err)
}

// Apply the "BPM Configs" QuarksJob, which creates BPM config secrets
qJob, err = r.jobFactory.BPMConfigsJob(*manifest, linkInfos, instance.ObjectMeta.Generation == 1)
if err != nil {
return reconcile.Result{}, log.WithEvent(instance, "BPMConfigsError").Errorf(ctx, "Failed to build BPM configs qJob: %v", err)
return reconcile.Result{}, log.WithEvent(instance, "BPMConfigsError").Errorf(ctx, "failed to build BPM configs qJob: %v", err)

}
log.Debug(ctx, "Creating BPM configs QuarksJob")
err = r.createQuarksJob(ctx, instance, qJob)
if err != nil {
return reconcile.Result{},
log.WithEvent(instance, "BPMConfigsError").Errorf(ctx, "Failed to create BPM configs qJob for BOSHDeployment '%s': %v", request.NamespacedName, err)
log.WithEvent(instance, "BPMConfigsError").Errorf(ctx, "failed to create BPM configs qJob for BOSHDeployment '%s': %v", request.NamespacedName, err)
}

// Update status of bdpl with the timestamp of the last reconcile
Expand All @@ -164,7 +164,7 @@ func (r *ReconcileBOSHDeployment) Reconcile(request reconcile.Request) (reconcil

err = r.client.Status().Update(ctx, instance)
if err != nil {
log.WithEvent(instance, "UpdateError").Errorf(ctx, "Failed to update reconcile timestamp on bdpl '%s' (%v): %s", instance.Name, instance.ResourceVersion, err)
log.WithEvent(instance, "UpdateError").Errorf(ctx, "failed to update reconcile timestamp on bdpl '%s' (%v): %s", instance.Name, instance.ResourceVersion, err)
return reconcile.Result{Requeue: false}, nil
}

Expand Down Expand Up @@ -210,13 +210,13 @@ func (r *ReconcileBOSHDeployment) createManifestWithOps(ctx context.Context, ins

// Set ownership reference
if err := r.setReference(instance, manifestSecret, r.scheme); err != nil {
return log.WithEvent(instance, "ManifestWithOpsRefError").Errorf(ctx, "Failed to set ownerReference for Secret '%s': %v", manifestSecretName, err)
return log.WithEvent(instance, "ManifestWithOpsRefError").Errorf(ctx, "failed to set ownerReference for Secret '%s': %v", manifestSecretName, err)
}

// Apply the secret
op, err := controllerutil.CreateOrUpdate(ctx, r.client, manifestSecret, mutate.SecretMutateFn(manifestSecret))
if err != nil {
return log.WithEvent(instance, "ManifestWithOpsApplyError").Errorf(ctx, "Failed to apply Secret '%s': %v", manifestSecretName, err)
return log.WithEvent(instance, "ManifestWithOpsApplyError").Errorf(ctx, "failed to apply Secret '%s': %v", manifestSecretName, err)
}

log.Debugf(ctx, "ResourceReference secret '%s' has been %s", manifestSecret.Name, op)
Expand Down Expand Up @@ -245,10 +245,10 @@ func (r *ReconcileBOSHDeployment) createQuarksJob(ctx context.Context, instance
func (r *ReconcileBOSHDeployment) listLinkInfos(instance *bdv1.BOSHDeployment, manifest *bdm.Manifest) (converter.LinkInfos, error) {
linkInfos := converter.LinkInfos{}

// find all missing providers in the manifest to track duplicated secrets then
// find all missing providers in the manifest, so we can look for secrets
missingProviders := listMissingProviders(*manifest)

// quarksLinks store missing provider name with type from secrets
// quarksLinks store for missing provider names with types read from secrets
quarksLinks := map[string]bdm.QuarksLink{}
if len(missingProviders) != 0 {
// list secrets and services from target deployment
Expand All @@ -259,7 +259,7 @@ func (r *ReconcileBOSHDeployment) listLinkInfos(instance *bdv1.BOSHDeployment, m
crc.InNamespace(instance.Namespace),
)
if err != nil {
return linkInfos, errors.Wrapf(err, "listing secrets for seeking links from '%s':", instance.Name)
return linkInfos, errors.Wrapf(err, "listing secrets for link in deployment '%s':", instance.Name)
}

servicesLabels := map[string]string{bdv1.LabelDeploymentName: instance.Name}
Expand All @@ -269,7 +269,7 @@ func (r *ReconcileBOSHDeployment) listLinkInfos(instance *bdv1.BOSHDeployment, m
crc.InNamespace(instance.Namespace),
)
if err != nil {
return linkInfos, errors.Wrapf(err, "listing services for seeking links from '%s':", instance.Name)
return linkInfos, errors.Wrapf(err, "listing services for link in deployment '%s':", instance.Name)
}

for _, s := range secrets.Items {
Expand All @@ -296,14 +296,14 @@ func (r *ReconcileBOSHDeployment) listLinkInfos(instance *bdv1.BOSHDeployment, m

serviceRecords, err := r.getServiceRecords(instance.Namespace, services.Items)
if err != nil {
return linkInfos, errors.Wrap(err, fmt.Sprintf("Failed to get link services for: %s", instance.Name))
return linkInfos, errors.Wrapf(err, "failed to get link services for: %s", instance.Name)
}

for qName := range quarksLinks {
if svcRecord, ok := serviceRecords[qName]; ok {
pods, err := r.listPodsFromSelector(instance.Namespace, svcRecord.selector)
if err != nil {
return linkInfos, errors.Wrap(err, fmt.Sprintf("Failed to get link pods for: %s", instance.Name))
return linkInfos, errors.Wrapf(err, "Failed to get link pods for: %s", instance.Name)
}

var jobsInstances []bdm.JobInstance
Expand Down Expand Up @@ -338,7 +338,7 @@ func (r *ReconcileBOSHDeployment) listLinkInfos(instance *bdv1.BOSHDeployment, m
}

if len(missingPs) != 0 {
return linkInfos, errors.New(fmt.Sprintf("missing secrets of providers: %s", strings.Join(missingPs, ", ")))
return linkInfos, errors.New(fmt.Sprintf("missing link secrets for providers: %s", strings.Join(missingPs, ", ")))
}

if len(quarksLinks) != 0 {
Expand All @@ -363,7 +363,7 @@ func (r *ReconcileBOSHDeployment) getServiceRecords(namespace string, svcs []cor

svcRecords[providerName] = serviceRecord{
selector: svc.Spec.Selector,
dnsRecord: fmt.Sprintf("%s.%s.svc.cluster.local", svc.Name, namespace),
dnsRecord: fmt.Sprintf("%s.%s.svc.%s", svc.Name, namespace, bdm.GetClusterDomain()),
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/kube/controllers/boshdeployment/deployment_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,12 @@ var _ = Describe("ReconcileBoshDeployment", func() {
})

_, err := reconciler.Reconcile(request)
Expect(err.Error()).To(ContainSubstring("listing secrets for seeking links"))
Expect(err.Error()).To(ContainSubstring("listing secrets for link in deployment"))
})
It("handles an error on missing providers when the secret doesn't have the annotation", func() {
bazSecret.Annotations = nil
_, err := reconciler.Reconcile(request)
Expect(err.Error()).To(ContainSubstring("missing secrets of providers"))
Expect(err.Error()).To(ContainSubstring("missing link secrets for providers"))
})

It("handles an error on duplicated secrets of provider when duplicated secrets match the annotation", func() {
Expand Down

0 comments on commit e5c7d2c

Please sign in to comment.