Add ability to custom handle redirects #5678

abogdanov37 · 2024-04-24T09:46:19Z

linux-foundation-easycla · 2024-04-24T09:46:22Z

The committers listed above are authorized under a signed CLA.

✅ login: abogdanov37 (de1f38e, e39cf04, cf92cd8, f8e3e74)

k8s-ci-robot · 2024-04-24T09:46:27Z

Welcome @abogdanov37!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-24T09:46:28Z

Hi @abogdanov37. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2024-04-24T09:46:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abogdanov37
Once this PR has been reviewed and has the lgtm label, please assign knverey for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abogdanov37 · 2024-05-02T12:39:18Z

@koba1t @varshaprasad96 Please, check the task may be it has more simple solution than thit PR!

koba1t

Hi @abogdanov37
Thanks for your contribution!

I think looks good to me for your codebase.
I add a few comments. Please check that.

So, I think we need to add some tests that check to fix the problem.
Please add some test scenarios to check this problem!

.gitignore

api/internal/target/kusttarget.go

abogdanov37 · 2024-06-21T15:01:38Z

Hi @abogdanov37 Thanks for your contribution!

I think looks good to me for your codebase. I add a few comments. Please check that.

So, I think we need to add some tests that check to fix the problem. Please add some test scenarios to check this problem!

Ok. Next week I'll add tests and work on comments!
Thanks for feedback!

k8s-ci-robot · 2024-06-25T15:37:23Z

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…art working on unit tests.

abogdanov37 · 2024-06-27T12:27:21Z

Hi @abogdanov37 Thanks for your contribution!
I think looks good to me for your codebase. I add a few comments. Please check that.
So, I think we need to add some tests that check to fix the problem. Please add some test scenarios to check this problem!

Ok. Next week I'll add tests and work on comments! Thanks for feedback!

Hi @koba1t! I have been add test for fileloader. Skip localizer and kusttarget because have no idea how to test override loader there. If you have any idea please share it I'll try to implement

koba1t · 2024-06-30T18:29:13Z

api/internal/loader/fileloader_test.go

+		},
+	}
+	for _, x := range testCaseRedirect {
+		expectedLocation := "https://redirect.com/resource.yaml"


It looks like redirect.com is owned by a company.
I understand this fakeHttpClient won't access the global internet, but I prefer to use a reserved example domain instead of something that someone else owns.

Domain Name: REDIRECT.COM Registry Domain ID: 32687398_DOMAIN_COM-VRSN Registrar WHOIS Server: Whois.bigrock.com Registrar URL: http://www.bigrock.com Updated Date: 2023-06-20T20:24:31Z Creation Date: 2000-08-10T10:33:16Z Registry Expiry Date: 2024-08-10T10:33:15Z Registrar: BigRock Solutions Ltd Registrar IANA ID: 1495 Registrar Abuse Contact Email: [email protected] Registrar Abuse Contact Phone: +1.832-295-1535 Domain Status: clientTransferProhibited https://icann.org/epp#clientTransferProhibited Name Server: NS-1118.AWSDNS-11.ORG Name Server: NS-1717.AWSDNS-22.CO.UK Name Server: NS-320.AWSDNS-40.COM Name Server: NS-918.AWSDNS-50.NET DNSSEC: unsigned URL of the ICANN Whois Inaccuracy Complaint Form: https://www.icann.org/wicf/ >>> Last update of whois database: 2024-06-30T18:22:52Z <<<

Ок. I'll fix it!

koba1t

Thank you for adding a test!

So, it looks like your test only checks that the location needs to redirect. I think it's preferable to add a check to ensure that redirect doesn't happen.

api/internal/loader/fileloader_test.go

abogdanov37 · 2024-07-01T10:22:12Z

Thank you for adding a test!

So, it looks like your test only checks that the location needs to redirect. I think it's preferable to add a check to ensure that redirect doesn't happen.

I think that another tests check behavior if redirect is not happened.

abogdanov37 · 2024-07-01T10:30:03Z

I plan fix all issues today. Thanks for review!

abogdanov37 · 2024-07-03T13:45:57Z

Hi @koba1t. All highlited issues have been fixed!

koba1t · 2024-08-06T18:48:42Z

I tried to run CI on GitHub Actions

koba1t · 2024-08-06T19:09:57Z

/ok-to-test

koba1t · 2024-08-06T19:36:06Z

api/internal/target/kusttarget.go

@@ -423,7 +423,12 @@ func (kt *KustTarget) accumulateResources(
 			if errors.Is(errF, load.ErrHTTP) {
 				return nil, errF
 			}
+			var redErr *load.RedirectionError
+			if errors.As(errF, &redErr) {
+				path = redErr.NewPath


I think this code is using redirect Location path directly.
I am worried that newPath will be used by no check.
like the redirected path requires redirecting more, or the redirected path is missing contents.

I investigated in these scenarious

new loader works only with directory and git repo because of that all checks will be done in common way in

kustomize/api/internal/loader/fileloader.go

Line 152 in cc9dd34

func (fl *FileLoader) New(path string) (ifc.Loader, error) {

If newPath will be a simple url in new loader it will be treated as directory and kustomize finish work with error in common way

If some redirects (301 to 309 http codes) will happen in loading process

kustomize/api/internal/loader/fileloader.go

Line 314 in cc9dd34

resp, err := hc.Get(path)

They will be processed by http client out of the box
Following this points I think that all possible checks have been done

Sorry, Maybe I explained it wrong.

The scenario I was concerned about that the redErr.NewPath address returning a 300 status code.

I have understood! Sorry ((( I does not implement this case to avoid recursion. If you think that this case should be implemented I'll do it.

koba1t

Hi @abogdanov37
I'm so sorry to delay the review.

I add comments for implements. Please check and write your comment if you think my worries are trivial matter.

koba1t · 2024-08-06T19:38:14Z

api/internal/loader/fileloader.go

+			var newPath string = resp.Header.Get("Location")
+			return nil, &RedirectionError{
+				Msg:     "Response is redirect",
+				NewPath: newPath,
+			}


Maybe That is better to solve redirects here than return RedirectionError and a new path.

Hi! It does not work in all cases. For example if I return redirect to git. In that case httpClient has no credentials for download content

For example if I return redirect to git. In that case httpClient has no credentials for download content

Sorry, I couldn't understand about this sentence.
In my understanding, that repo uses the HTTPS protocol to download when HTTP redirects to git.

My explanation was not clear. Sorry for that. I'll try again.
Example
We get next url in resource section https://example.com/apps/app1?version=1.1.0
When kustomize (in build proccess) go to that url, it get Redirect 300 with newPath https://customgit.org/somegrop/project-app1.git?refs=1.1.0. The repository is PRIVATE.
In this case if we go to that git repo using httpClient we get page with ask for credential (((
But if we create a new loader kustomize will use console git which already have credentials and can clone repository

abogdanov37 · 2024-08-08T13:02:54Z

Hi @abogdanov37 I'm so sorry to delay the review.

I add comments for implements. Please check and write your comment if you think my worries are trivial matter.

Hi @koba1t.
It's ok.
I add comments for your issues.
Thanks for review

abogdanov37 · 2024-09-02T12:57:02Z

Hi @koba1t! Any other issues in my pr? May be I can help some how to force PR merge?

koba1t · 2024-09-03T21:45:59Z

Hi @abogdanov37

Sorry for the delayed response.
I have added a few replies. Please check my comments!

abogdanov37 · 2024-09-06T07:42:11Z

Hi @abogdanov37

Sorry for the delayed response. I have added a few replies. Please check my comments!

Hi @koba1t! Thanks for explanations. I post answers.

k8s-triage-robot · 2024-12-05T07:45:57Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Add ability to custom handle redirects

de1f38e

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 24, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 24, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 24, 2024

k8s-ci-robot requested review from koba1t and varshaprasad96 April 24, 2024 09:46

abogdanov37 closed this Apr 24, 2024

abogdanov37 mentioned this pull request Apr 24, 2024

Add redirect support for resource loading #5675

Open

2 tasks

abogdanov37 reopened this Apr 24, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 2, 2024

koba1t closed this Jun 20, 2024

koba1t reopened this Jun 20, 2024

koba1t requested changes Jun 20, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

api/internal/target/kusttarget.go Outdated Show resolved Hide resolved

abogdanov37 force-pushed the add-getting-resources-with-redirect-support branch from 52c81db to 1967df5 Compare June 26, 2024 10:31

Add redirection support for localize command. Address PR comments. St…

f8e3e74

…art working on unit tests.

abogdanov37 force-pushed the add-getting-resources-with-redirect-support branch from 1967df5 to f8e3e74 Compare June 26, 2024 11:12

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 26, 2024

fix tests

cf92cd8

koba1t requested changes Jun 30, 2024

View reviewed changes

api/internal/loader/fileloader_test.go Show resolved Hide resolved

api/internal/loader/fileloader_test.go Show resolved Hide resolved

Fix linter issues. Fix domain to test redirect.

e39cf04

koba1t closed this Aug 6, 2024

koba1t reopened this Aug 6, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2024

koba1t reviewed Aug 6, 2024

View reviewed changes

koba1t requested changes Aug 6, 2024

View reviewed changes

abogdanov37 mentioned this pull request Aug 13, 2024

Add target section for all kustomize transformers #5661

Open

2 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to custom handle redirects #5678

Add ability to custom handle redirects #5678

abogdanov37 commented Apr 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 24, 2024 •

edited

Loading

k8s-ci-robot commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

abogdanov37 commented May 2, 2024

koba1t left a comment

abogdanov37 commented Jun 21, 2024

k8s-ci-robot commented Jun 25, 2024

abogdanov37 commented Jun 27, 2024

koba1t Jun 30, 2024 •

edited

Loading

abogdanov37 Jul 1, 2024

koba1t left a comment •

edited

Loading

abogdanov37 commented Jul 1, 2024

abogdanov37 commented Jul 1, 2024

abogdanov37 commented Jul 3, 2024

koba1t commented Aug 6, 2024

koba1t commented Aug 6, 2024

koba1t Aug 6, 2024

abogdanov37 Aug 8, 2024

koba1t Sep 3, 2024

abogdanov37 Sep 6, 2024

koba1t left a comment

koba1t Aug 6, 2024

abogdanov37 Aug 8, 2024 •

edited

Loading

koba1t Sep 3, 2024

abogdanov37 Sep 6, 2024

abogdanov37 commented Aug 8, 2024

abogdanov37 commented Sep 2, 2024

koba1t commented Sep 3, 2024

abogdanov37 commented Sep 6, 2024

k8s-triage-robot commented Dec 5, 2024

Add ability to custom handle redirects #5678

Are you sure you want to change the base?

Add ability to custom handle redirects #5678

Conversation

abogdanov37 commented Apr 24, 2024 • edited Loading

linux-foundation-easycla bot commented Apr 24, 2024 • edited Loading

k8s-ci-robot commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

abogdanov37 commented May 2, 2024

koba1t left a comment

Choose a reason for hiding this comment

abogdanov37 commented Jun 21, 2024

k8s-ci-robot commented Jun 25, 2024

abogdanov37 commented Jun 27, 2024

koba1t Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koba1t left a comment • edited Loading

Choose a reason for hiding this comment

abogdanov37 commented Jul 1, 2024

abogdanov37 commented Jul 1, 2024

abogdanov37 commented Jul 3, 2024

koba1t commented Aug 6, 2024

koba1t commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koba1t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abogdanov37 Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abogdanov37 commented Aug 8, 2024

abogdanov37 commented Sep 2, 2024

koba1t commented Sep 3, 2024

abogdanov37 commented Sep 6, 2024

k8s-triage-robot commented Dec 5, 2024

abogdanov37 commented Apr 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 24, 2024 •

edited

Loading

koba1t Jun 30, 2024 •

edited

Loading

koba1t left a comment •

edited

Loading

abogdanov37 Aug 8, 2024 •

edited

Loading