Skip to content

added resource details in deprecation message #13060

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

added resource details in deprecation message #13060

wants to merge 22 commits into from

Conversation

gnana997
Copy link

Motivation

#13030 : Helps user debugging in the usage of deprecated resources

Implementation information

Added resource name and resource meta information along with the deprecation resource errors

Supporting documentation

Fix #13030

@gnana997 gnana997 requested a review from a team as a code owner March 11, 2025 19:05
@github-actions GitHub Actions
Copy link
Contributor

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

  • Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
  • PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
  • IPv6 is taken into account (.e.g: no string concatenation of host port)
  • Tests (Unit test, E2E tests, manual test on universal and k8s)
    • Don't forget ci/ labels to run additional/fewer tests
  • Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

gnana997 and others added 5 commits March 12, 2025 11:51
@gnana997
added resource details in deprecation message
4348c29 
Signed-off-by: gnana997 <gnana097@gmail.com>
@slonka @gnana997
chore(api): revert bool to boolValue PR (kumahq#13057)
17d2c18 
## Motivation

**We only need `mesh` resource revert.**

Seems that syncing is broken:

```
type: Mesh
name: default
mtls:
  enabledBackend: ca-1
  backends:
    - name: ca-1
      type: builtin
      dpCert:
        rotation:
          expiration: 1d
networking:
  outbound:
    passthrough: false
routing:
  localityAwareLoadBalancing: true
  zoneEgress: true
```

becomes:

```
  spec:
    mtls:
      backends:
        - dpCert:
          rotation:
            expiration: 1d
        name: ca-1
        type: builtin
      enabledBackend: ca-1
    networking:
      outbound:
        passthrough: false
    routing: {}
```

on the zone

## Implementation information

revert Mesh part of kumahq#12895

---------

Signed-off-by: slonka <slonka@users.noreply.github.com>
Signed-off-by: gnana997 <gnana097@gmail.com>
@jijiechen @gnana997
chore(dev): add notes for debugging E2E in IDE (kumahq#13005)
265874a 
## Motivation

Adding notes for debugging E2E in IDE

## Implementation information

<!-- Explain how this was done and potentially alternatives considered
and discarded -->

## Supporting documentation

<!-- Is there a MADR? An Issue? A related PR? -->

Fix #XX

> Changelog: skip

<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

Signed-off-by: Jay Chen <1180092+jijiechen@users.noreply.github.com>
Signed-off-by: gnana997 <gnana097@gmail.com>
@jijiechen @gnana997
feat(xds): add internal address config onto HttpConnectionManager (ku…
5c4e642 
…mahq#12986)

## Motivation

add internal address config onto HttpConnectionManager, more details are
available in issue kumahq#12190

## Implementation information

introducing a new configuration item `ipam.knownInternalCIDRs` on zonal
CP to allow users specify their known internal address pool and we
assign these values when generating Envoy config for
HttpConnectionManager

## Supporting documentation

<!-- Is there a MADR? An Issue? A related PR? -->

fixes kumahq#12190

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Jay Chen <1180092+jijiechen@users.noreply.github.com>
Signed-off-by: gnana997 <gnana097@gmail.com>
@jijiechen @lukidzi
@Icarus9913 @gnana997
chore(upgrade.md): improve wording (kumahq#13046)
04e1e7b 
## Motivation

Minor improvements in `UPGRADE.md`

## Implementation information

change wording inline

> Changelog: skip

<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Jay Chen <1180092+jijiechen@users.noreply.github.com>
Signed-off-by: Jay Jijie Chen <1180092+jijiechen@users.noreply.github.com>
Co-authored-by: Lukasz Dziedziak <lukidzi@gmail.com>
Co-authored-by: Icarus Wu <icaruswu66@qq.com>
Signed-off-by: gnana997 <gnana097@gmail.com>
@gnana997 gnana997 force-pushed the deprecation-reource-details branch from 3e1792b to 04e1e7b Compare March 12, 2025 06:21
@gnana997 gnana997 requested a review from lahabana March 17, 2025 12:13
bartsmykla
bartsmykla previously approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@bartsmykla bartsmykla left a comment

Choose a reason for hiding this comment

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

lgtm

@Icarus9913
Copy link
Contributor

Icarus9913 commented Mar 26, 2025 •
edited
Loading

/golden_files

The CI ran over time

@Icarus9913
Copy link
Contributor

@gnana997 Could you fix the unit test?
The command make test UPDATE_GOLDEN_FILES=true may help you

@gnana997
Copy link
Author

@gnana997 Could you fix the unit test? The command make test UPDATE_GOLDEN_FILES=true may help you

Hi @lahabana , I tried the tests and all of them passed however it has updated json and yaml files with warnings of deprecated resources.
Just wanted to confirm that I don't need to push those changes right?
Thanks!

@bartsmykla
Copy link
Contributor

@gnana997 after new golden files were generated via make test UPDATE_GOLDEN_FILES=true please commit and push them

@gnana997
updated goldenfiles with make test
0a75162 
Signed-off-by: gnana997 <gnana097@gmail.com>
@gnana997 gnana997 force-pushed the deprecation-reource-details branch from 3a30e02 to 0a75162 Compare March 27, 2025 17:47
@gnana997
Copy link
Author

@gnana997 after new golden files were generated via make test UPDATE_GOLDEN_FILES=true please commit and push them

Oh ok. Got it. I have pushed the changes.
Thanks!

@gnana997 gnana997 requested a review from bartsmykla March 27, 2025 17:49
@lahabana
Copy link
Contributor

It's a little odd to return the resource name in the deprecation message. What are we trying to achieve here? Readability in kumactl apply?

If that's the case adding the resource type and name in the response should be done in kumactl not in the response of the api

1 similar comment
@lahabana
Copy link
Contributor

It's a little odd to return the resource name in the deprecation message. What are we trying to achieve here? Readability in kumactl apply?

If that's the case adding the resource type and name in the response should be done in kumactl not in the response of the api

@Icarus9913
Copy link
Contributor

Icarus9913 commented Mar 28, 2025 •
edited
Loading

It's a little odd to return the resource name in the deprecation message.

Because I noticed that the Deprecation function is also called in the CP, and the corresponding log lost subject object.(PS: the deprecation is called in the Create interface) Here's the logs:

2025-03-07T02:59:56.261Z	INFO	KubeAPIWarningLogger	'from' field is deprecated, use 'rules' instead
---
2025-03-06T10:38:16.085Z    INFO    KubeAPIWarningLogger    Name that doesn't conform DNS (RFC 1035) format is deprecated: a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')

@gnana997 gnana997 requested a review from Icarus9913 March 28, 2025 17:53
Icarus9913
Icarus9913 requested changes Mar 29, 2025
View reviewed changes
pkg/api-server/testdata/resources/crud/put_meshaccesslog_00.golden.json
Comment on lines +1 to +5
{
"warnings": [
"MeshAccessLog type resource \"my-access-log\" has the following deprecated fields: "
]
}
Copy link
Contributor

@Icarus9913 Icarus9913 Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@gnana997 Hi, you can see there are no deprecation details in this sentence as I said before. It's an obvious unexpected output, and we need to figure out why. I'll have a look at it later.

Icarus9913
Icarus9913 reviewed Mar 30, 2025
View reviewed changes
Copy link
Contributor

@Icarus9913 Icarus9913 left a comment

Choose a reason for hiding this comment

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

@gnana997 Please apply my new changes, then update the golden files again.

@gnana997 @Icarus9913
Updated deprication message
6a58ac9 
Co-authored-by: Icarus Wu <icaruswu66@qq.com>
Signed-off-by: Gnana Siva Sai V <gnana097@gmail.com>
@lahabana
Copy link
Contributor

It's a little odd to return the resource name in the deprecation message.

Because I noticed that the Deprecation function is also called in the CP, and the corresponding log lost subject object.(PS: the deprecation is called in the Create interface) Here's the logs:

2025-03-07T02:59:56.261Z	INFO	KubeAPIWarningLogger	'from' field is deprecated, use 'rules' instead
---
2025-03-06T10:38:16.085Z    INFO    KubeAPIWarningLogger    Name that doesn't conform DNS (RFC 1035) format is deprecated: a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')

The I believe that the problem is in the log message not in the deprecation warning message.

lahabana
lahabana requested changes Apr 1, 2025
View reviewed changes
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

We need to get background on why KubeAPIWarningLogger doesn't add context. Maybe there's a workaround that's nicer. Worth looking at how other projects to deprecations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Icarus9913 Icarus9913 Icarus9913 requested changes

@lahabana lahabana lahabana requested changes

@bartsmykla bartsmykla Awaiting requested review from bartsmykla

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the resource type and name as deprecation title
6 participants
@gnana997 @Icarus9913 @bartsmykla @lahabana @slonka @jijiechen