Issue 1296 fix cf labels not working

[xiaocongli]

According to the Cloud Foundry documentation on manifest metadata, labels and annotations must be nested under the metadata section. In the current cf-java-client, they are placed directly under the application node, which is incorrect. As a result, attribute values are lost when using CF labels to identify extra component attributes.

--- applications: - name: test-appmemory: 1Ginstances: 1metadata: labels: test-label: "test-value"

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: xiaocongli / name: Xiaocong Li (033eceb, 4c85487, 74ca1d6, 7959661, a892ebc, bb9b252)

[JNKielmann]

This is the relevant issue describing the problem this PR tries to fix: #1296

[theghost5800]

Looks good to me. @Kehrlann Could you please take a look at this PR and run integration tests with this change, thanks!

[Kehrlann]

Got it. I'm away this week. Will pick this up on Monday.

[Kehrlann]

@xiaocongli
Code looks good.
The integration tests pass as is; but they just test that a push succeeds. It'd be nice to test that the annotations and labels are actually present on the created object.

Please update the verification for the test, for example something like:

this.cloudFoundryOperations .applications() .pushManifestV3(PushManifestV3Request.builder().manifest(manifest).build()) .then( this.cloudFoundryOperations .applications() .get(GetApplicationRequest.builder().name(applicationName).build()) ) .map(ApplicationDetail::getId) .flatMap(id -> this.client .applicationsV3() .get( org.cloudfoundry.client.v3.applications.GetApplicationRequest.builder() .applicationId(id) .build() ) ) .as(StepVerifier::create) .expectNextMatches(createdApp -> labels.equals(createdApp.getMetadata().getLabels()) && annotations.equals(createdApp.getMetadata().getAnnotations())) .expectComplete() .verify(Duration.ofMinutes(5));

Feel free to write your own verification.

[xiaocongli]

Hi @Kehrlann,

Here is my understanding:

• The operations-layer integration tests in operations/ApplicationsTest.java are meant to validate user-facing behaviors. Regardless of whether a V2 or V3 manifest is pushed, the read-back in operations returns the common manifest model (ApplicationManifest), whose fields come from _ApplicationManifestCommon (e.g., name, memory, buildpacks, routes, etc.). It does not expose V3-only fields like metadata (labels/annotations inside). To verify V3-specific elements, we have to bring in the CloudFoundryClient like your code this.client.applicationsV3().get(...).
• The CloudFoundryClient is not injected in operations-layer tests; it is available in the V2/V3 client-layer tests. see v3/ApplicationsTest.java#L113C24-L113C42.

  @AutowiredprivateCloudFoundryClientcloudFoundryClient;

• A metadata/labels test is already exists in the V3 client integration tests, validating the API-level behavior directly. see v3/ApplicationsTest.java#L119-L163

My points:

• We probably should not autowire CloudFoundryClient into operations-layer tests; it belongs in the V2/V3 client-layer tests.
• See the last point in my understanding part, the metadata/labels integration test are already covered in the V3 API layer using the client directly (independent of my operations-level changes).
• In operations-layer tests, we can validate pushing with a V3 manifest, but we cannot read back V3-specific fields.
• I looked into the existing V3 manifest related in operations layer test, they didn't verify V3 fields as well.

If V3 metadata have to be verified in operations layer, I guess the only way is to introduce a cross-layer read-back by injecting CloudFoundryClient into the operations test and calling cloudFoundryClient.applicationsV3().get(...) which might not be a good idea.

I've also done a local test with help from @JNKielmann via a small app

ManifestV3manifest = ApplicationManifestUtilsV3.read(input)

Input yaml:

--- applications: - name: demo-appmemory: 256Minstances: 1buildpacks: - java_buildpackenv: JAVA_OPTS: "-Xss512k"metadata: labels: owner: team-afeature: manifest-labelsexample.com/component: webapp.kubernetes.io/name: demo-appannotations: description: "Sample app to reproduce manifest metadata behavior"

Depending on latest 5.14.0.RELEASE, i got manifest as below:

--- applications: - buildpacks: - java_buildpackenv: JAVA_OPTS: -Xss512kinstances: 1memory: 256Mname: demo-appversion: 1

With my local generated 4 cloudfoundry-xxx-5.15.0.BUILD-SNAPSHOT.jar, the output manifest was below(the different order of annotations and labels should not be a problem):

--- applications: - buildpacks: - java_buildpackenv: JAVA_OPTS: -Xss512kinstances: 1memory: 256Mmetadata: annotations: description: Sample app to reproduce manifest metadata behaviorlabels: owner: team-afeature: manifest-labelsapp.kubernetes.io/name: demo-appexample.com/component: webname: demo-appversion: 1

Please correct me if anything is wrong. :)

[xiaocongli]

Like some of the other tests, what I am not sure is the version number in IfCloudFoundryVersion annotation. I'm currently giving similar as some of the other values - PCF_4_v2(Version.forIntegers(2, 209, 0)). The latest release version is v5.14.0.RELEASE, I guess it is something like Version.forIntegers(5, 14, 0). But I don't think it is matter as it is calling V3 code in the test if it runs :) Right?

[Kehrlann]

Hey @xiaocongli ! Thanks for the detailed answer.

We probably should not autowire CloudFoundryClient into operations-layer tests; it belongs in the V2/V3 client-layer tests.

Ideally, that would be true, and we would only be testing black-box style. That's what we would strive for if we started from scratch and had more bandwidth to work on this. But that's not the case, and not all APIs are implemented using the Operations abstraction. Currently, it's inconvenient to test the full behavior at that layer. So either we implement what we need to test it (probably too much work), or we accept that there is some mixing of concerns in our integration tests.

You'll notice that the client is already injected in operations tests multiple times:

• NetworkPoliciesTests
• ServiceAdminTests
• ServicesTests
• UserAdminTests

Given the shape of the project, "mixing layers" is a tradeoff I'll make gladly.

See the last point in my understanding part, the metadata/labels integration test are already covered in the V3 API layer using the client directly (independent of my operations-level changes).

Yes, some metadata tests exist in client/v3/Applications, but they don't cover the manifest (and not your changes). Technically, your changes are closer to v3/Spaces through #applyManifest; but that takes a file (a serialized manifest) ; so it does not cover your change either.

---

what I am not sure is the version number in IfCloudFoundryVersion annotation

Ah, that's a remnant from older times from this project. I don't think it's super relevant anymore. The name mentionsPCF versions, which are "Pivotal Cloud Foundry" versions (currently called "Tanzu Platform for CloudFoundry) ; and the numbers refers the Cloud Controller v2 API version. The PCF version (here, 4.2) made it easy for Pivotal/VMware/Broadcom folks to understand which environment they'd need for a test. To check which version of CC was deployed, they needed to use some data from CC, and so they chose the v2 API version (here 2.209.0).
This is completely uncorrelated to the cf-java-client version.

That's not great, now that this project is maintained beyond just Pivotal/... we should probably rename these PCF numbers to CAPI_RELEASE - for example, PCF_4_v2 would be CAPI release 1.158.

And, ideally, for v3 tests we'd have an annotation that checks for a specific v3 API, version but that's not implemented. Currently we use the v2 info request (source)

[xiaocongli]

Hi @Kehrlann, I added the integration test code accordingly :)

[Kehrlann]

Thank you for your contribution, and your patience!
The test passes on my test environment.