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

Cpi plugin support #662

Merged
merged 2 commits into from
Sep 20, 2024
Merged

Cpi plugin support #662

merged 2 commits into from
Sep 20, 2024

Conversation

klakin-pivotal
Copy link
Contributor

This PR adds CLI support for deployment manifests that specify a CPI with multiple Jobs.

A CPI's Job has traditionally been specified in a Director manifest with the {cloud_provider: {template: {name: "name", release: "release"}}} manifest construction. This commit extends the cloud_provider definition with a new templates key whose value is an array of zero or more {name:, release:} hashes/mappings/dictionaries.

Supporting this change required relaxing some explicit requirements in sections of the code that expected that a CPI could only ever have one Job. However, the requirement that exactly one of the CPI's Jobs have a template that creates a bin/cpi executable is retained.

These changes are backwards-compatible, so existing manifests that use cloud_provider: template... syntax will work just fine.

Why have we made these changes? To support upcoming work to permit the creation of "sidecar" or "plugin" Jobs for CPIs that can be used by the CPI during both CLI-driven CPI invocations (like create-env) and also Director-driven CPI invocations.

Changes to the vSphere CPI that will actually make use of this change to the CLI will be coming soon.

This change fixes mockgen by changing the package name to the reflect
the module name github.com/cloudfoundry/bosh-cli/v7. Presumably, the
module name changed at some point, but the mockgen script wasn't updated
to reflect it. So it would try to add github.com/cloudfoundry/bosh-cli
as a go mod dependency and complain vendoring is inconsistent (you can't
vendor bosh-cli in its own repo).

We will be regenerating mocks using the updated script, so it's good to
have it in working order.

Signed-off-by: Kenneth Lakin <[email protected]>
@klakin-pivotal klakin-pivotal requested review from selzoc, jpalermo and a team September 13, 2024 18:05
errs = append(errs, bosherr.Error("cloud_provider.template.release must be provided"))
// When there is nothing in templates, return an error. It should have a CPI release.
if len(manifest.Templates) == 0 {
return fmt.Errorf("manifest.Templates cannot be empty and must contain one release")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure manifest.Templates tells them what they need to do since it's not the actual path in the manifest. I'm guessing this is cloud_provider.template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, that is definitely the wrong name to use in that error message.

Does "either cloud_provider.templates or cloud_provider.template must be provided and must contain at a CPI release" seem like a good instruction for that error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone ahead and reworded the message. See the diff I posted in the next "conversation".

Comment on lines 137 to 150
// FIXME: Do we need to deduplicate the templateList? It is illegal to have multiple
// entries with the same Name, but not illegal to have multiple entries with
// different Names but the same Release.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't feel necessary to me, but we certainly could. We should either do it or remove the FIXME though.

Similar to how you can't have multiple jobs with the same name, even if they come from different releases. I would expect you can't have two templates with the same name here either since I'd expect they both end up in the same folder in the "installation" path that is generated.

Copy link
Contributor Author

@klakin-pivotal klakin-pivotal Sep 20, 2024

Choose a reason for hiding this comment

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

Darn, I was hoping whoever reviewed this would have a strong opinion either way.

I'll add in the logic to detect duplicate templates with the same name and fail with a useful error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I've addressed both these change requests. I pushed up a change with the (lightly-doctored) diff below:

diff --git a/installation/manifest/parser.go b/installation/manifest/parser.go
index 1cfa4ee65..4b0e08be2 100644
--- a/installation/manifest/parser.go
+++ b/installation/manifest/parser.go
@@ -119,10 +119,13 @@ func (p *parser) Parse(path string, vars boshtpl.Variables, op patch.Op, release
  }

  templateList := []ReleaseJobRef{}
+ templateNameCount := make(map[string]int)
  if len(comboManifest.CloudProvider.Templates) != 0 {
    for _, template := range comboManifest.CloudProvider.Templates {
      templateName := template.Name
      templateList = append(templateList, ReleaseJobRef{Name: templateName, Release: template.Release})
+     // Check for duplicate names
+     templateNameCount[templateName] = templateNameCount[templateName] + 1
    }

    installationManifest.Templates = templateList
@@ -134,9 +137,16 @@ func (p *parser) Parse(path string, vars boshtpl.Variables, op patch.Op, release
    installationManifest.Templates = templateList
  }

- // FIXME: Do we need to deduplicate the templateList? It is illegal to have multiple
- //        entries with the same Name, but not illegal to have multiple entries with
- //        different Names but the same Release.
+ duplicateTemplateNames := []string{}
+ for templateName, count := range templateNameCount {
+   if count > 1 {
+     duplicateTemplateNames = append(duplicateTemplateNames, "'"+templateName+"'")
+   }
+ }
+
+ if len(duplicateTemplateNames) != 0 {
+   return Manifest{}, bosherr.WrapErrorf(err, "Duplicate templates names are illegal. Duplicate template names found: %v", strings.Join(duplicateTemplateNames, ", "))
+ }

  properties, err := biproperty.BuildMap(comboManifest.CloudProvider.Properties)
  if err != nil {
  diff --git a/installation/manifest/validator.go b/installation/manifest/validator.go
index b287debc7..12934d4c4 100644
--- a/installation/manifest/validator.go
+++ b/installation/manifest/validator.go
@@ -29,7 +29,7 @@ func (v *validator) Validate(manifest Manifest, releaseSetManifest birelsetmanif

  // When there is nothing in templates, return an error. It should have a CPI release.
  if len(manifest.Templates) == 0 {
-   return fmt.Errorf("manifest.Templates cannot be empty and must contain one release")
+   return fmt.Errorf("either cloud_provider.templates or cloud_provider.template must be provided and must contain at least one release")
  }

  for _, template := range manifest.Templates {

diff --git a/installation/manifest/validator_test.go b/installation/manifest/validator_test.go
index 69757e737..b4cde5c79 100644
--- a/installation/manifest/validator_test.go
+++ b/installation/manifest/validator_test.go
@@ -61,7 +61,7 @@ var _ = Describe("Validator", func() {

      err := validator.Validate(manifest, releaseSetManifest)
      Expect(err).To(HaveOccurred())
-     Expect(err.Error()).To(ContainSubstring("manifest.Templates cannot be empty and must contain one release"))
+     Expect(err.Error()).To(ContainSubstring("either cloud_provider.templates or cloud_provider.template must be provided and must contain at least one release"))
    })

    It("validates template must be fully specified", func() {
diff --git a/installation/manifest/parser_test.go b/installation/manifest/parser_test.go
index 4e7a0b47c..bff40d628 100644
--- a/installation/manifest/parser_test.go
+++ b/installation/manifest/parser_test.go
@@ -651,6 +651,42 @@ cloud_provider:
          Mbus: "http://fake-mbus-user:[email protected]:6868",
        }))
      })
+
+     Context("when there are multiple Jobs with the same names in the specified templates", func() {
+       BeforeEach(func() {
+         err := fakeFs.WriteFileString(comboManifestPath, `
+---
+name: fake-deployment-name
+cloud_provider:
+  templates:
+    - name: fake-cpi-job-name
+      release: fake-cpi-release-name
+    - name: fake-cpi-job-name
+      release: fake-cpi-release-other-name
+    - name: fake-cpi-job-name-not-duplicate
+      release: fake-cpi-release
+    - name: fake-cpi-job-name-number-two
+      release: fake-cpi-release
+    - name: fake-cpi-job-name-number-two
+      release: fake-cpi-release
+  mbus: http://fake-mbus-user:[email protected]:6868
+  properties:
+    fake-property-name:
+      nested-property: fake-property-value
+`)
+         Expect(err).ToNot(HaveOccurred())
+       })
+
+       It("detects the duplicate names and returns an error containing only the duplicate names", func() {
+         _, err := parser.Parse(comboManifestPath, boshtpl.StaticVariables{}, patch.Ops{}, releaseSetManifest)
+
+         Expect(err).To(HaveOccurred())
+         Expect(err.Error()).To(ContainSubstring("'fake-cpi-job-name'"))
+         Expect(err.Error()).To(ContainSubstring("'fake-cpi-job-name-number-two'"))
+         Expect(err.Error()).ToNot(ContainSubstring("'fake-cpi-job-name-not-duplicate'"))
+       })
+
+     })
    })

    Context("when the deprecated template key is specified instead of the templates key", func() {

This commit adds CLI support for deployment manifests that specify a CPI
with multiple Jobs.

A CPI's Job has traditionally been specified in a Director manifest with
the `{cloud_provider: {template: {name: "name", release: "release"}}}`
manifest construction. This commit extends the `cloud_provider`
defintion with a new `templates` key whose value is an array of zero or
more `{name:, release:}` hashes/mappings/dictionaries.

Supporting this change required relaxing some explicit requirements in
sections of the code that expected that a CPI could only ever have one
Job. However, the requirement that exactly one of the CPI's Jobs have a
template that creates a `bin/cpi` executable is retained.

These changes are backwards-compatible, so existing manifests that use
`cloud_provider: template...` syntax will work just fine.

Why have we made these changes? To support upcoming work to permit the
creation of "sidecar" or "plugin" Jobs for CPIs that can be used by the
CPI during both CLI-driven CPI invocations (like `create-env`) and also
Director-driven CPI invocations.

Signed-off-by: Kenneth Lakin <[email protected]>
@klakin-pivotal klakin-pivotal merged commit 0e106ba into main Sep 20, 2024
5 checks passed
@klakin-pivotal klakin-pivotal deleted the cpi-plugin-support branch September 20, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants