fix: Skip Resolving Secrets From Restricted Namespace #4645

Indresh2410 · 2023-06-05T04:29:42Z

tomkerkhove

Thanks for the PR! Can you please update the release notes and add some tests?

JorTurFer

Shouldn't we check also the current namespace? I mean, if the ScaledObject/ScaledJob is in the allowed namespace, we should read the secret

zroubalik · 2023-06-06T16:57:57Z

Shouldn't we check also the current namespace? I mean, if the ScaledObject/ScaledJob is in the allowed namespace, we should read the secret

Well probably not "current" but the one that is restricted.

JorTurFer · 2023-06-11T23:28:30Z

/run-e2e
Update: You can check the progress here

tobotg

Thanks @Indresh2410 for this PR. Just a few comments.

pkg/scaling/resolver/scale_resolvers.go

pkg/scaling/resolver/scale_resolvers_test.go

Indresh2410 · 2023-06-20T16:49:08Z

Sorry for delay in addressing review comments. Will address it soon

@tobotg / @JorTurFer based on this issue
#3668

If KEDA_RESTRICT_SECRET_ACCESS is enabled, we can remove secrets from clusterrole
https://keda.sh/docs/2.10/operate/cluster/#restrict-secret-access

But we would still require list/watch access to secrets right?
https://github.com/kedacore/keda/blob/main/cmd/operator/main.go#L192-L193

which won't be available for secrets once ENV variable is set

Apologies if something doesn't make sense here

JorTurFer · 2023-06-20T17:03:31Z

If KEDA_RESTRICT_SECRET_ACCESS is enabled, we can remove secrets from clusterrole keda.sh/docs/2.10/operate/cluster/#restrict-secret-access

But we would still require list/watch access to secrets right? main/cmd/operator/main.go#L192-L193

yeah, this is because as default, KEDA can read secrets from all the cluster, restricting the secret access we don't block the access to the secrets, we just limit it to keda's namespace. In fact, list/watch isn't enough because the certificate management requires more verbs, but that's deployed in other role/rolebinding: https://github.com/kedacore/keda/blob/main/config/rbac/role.yaml#L122-L139

KEDA will always have access to the secrets inside its own namespace

zroubalik

@Indresh2410 any update on this? we are about to release a new version.

Indresh2410 · 2023-06-21T10:10:24Z

Hi. Sorry for delay. Will push changes by EOD

…

On Wed, Jun 21, 2023, 12:54 PM Zbynek Roubalik ***@***.***> wrote: ***@***.**** commented on this pull request. @Indresh2410 <https://github.com/Indresh2410> any update on this? we are about to release a new version. — Reply to this email directly, view it on GitHub <#4645 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7A3MRZWCHK4MAPWWEDK6ZLXMKORPANCNFSM6AAAAAAY2NKJCA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

zroubalik · 2023-06-21T10:12:21Z

Hi. Sorry for delay. Will push changes by EOD
…
On Wed, Jun 21, 2023, 12:54 PM Zbynek Roubalik @.> wrote: @.* commented on this pull request. @Indresh2410 https://github.com/Indresh2410 any update on this? we are about to release a new version. — Reply to this email directly, view it on GitHub <#4645 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/A7A3MRZWCHK4MAPWWEDK6ZLXMKORPANCNFSM6AAAAAAY2NKJCA . You are receiving this because you were mentioned.Message ID: @.***>

Thanks!

Indresh2410 · 2023-06-21T17:28:15Z

If KEDA_RESTRICT_SECRET_ACCESS is enabled, we can remove secrets from clusterrole keda.sh/docs/2.10/operate/cluster/#restrict-secret-access
But we would still require list/watch access to secrets right? main/cmd/operator/main.go#L192-L193

yeah, this is because as default, KEDA can read secrets from all the cluster, restricting the secret access we don't block the access to the secrets, we just limit it to keda's namespace. In fact, list/watch isn't enough because the certificate management requires more verbs, but that's deployed in other role/rolebinding: https://github.com/kedacore/keda/blob/main/config/rbac/role.yaml#L122-L139

KEDA will always have access to the secrets inside its own namespace

Thanks @JorTurFer .This helps 😄

pkg/scaling/resolver/scale_resolvers_test.go

zroubalik · 2023-06-22T12:17:51Z

@Indresh2410 could you please rebase this PR and fix the conflict in Changelog?

zroubalik

@Indresh2410 could you please fix the path here? pkg/mock/mock_secretLister/mock_interfaces.go

make it lowercase only: pkg/mock/mock_secretlister/mock_interfaces.go

Makefile

zroubalik · 2023-06-22T13:36:22Z

@Indresh2410 do you think you'll be able to tackle the rest of the problems today? We plan to release a new version in a few hours.

Indresh2410 · 2023-06-22T13:43:34Z

Yes. Will address changes in next 1 hour. Hope it's fine for you

…

On Thu, Jun 22, 2023, 7:06 PM Zbynek Roubalik ***@***.***> wrote: @Indresh2410 <https://github.com/Indresh2410> do you think you'll be able to tackle the rest of the problems today? We plan to release a new version in a few hours. — Reply to this email directly, view it on GitHub <#4645 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7A3MR7XJAKUA7PDIP6KRR3XMRC6FANCNFSM6AAAAAAY2NKJCA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

zroubalik · 2023-06-22T13:50:26Z

Yes. Will address changes in next 1 hour. Hope it's fine for you
…
On Thu, Jun 22, 2023, 7:06 PM Zbynek Roubalik @.> wrote: @Indresh2410 https://github.com/Indresh2410 do you think you'll be able to tackle the rest of the problems today? We plan to release a new version in a few hours. — Reply to this email directly, view it on GitHub <#4645 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/A7A3MR7XJAKUA7PDIP6KRR3XMRC6FANCNFSM6AAAAAAY2NKJCA . You are receiving this because you were mentioned.Message ID: @.>

Thanks!

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Indresh2410 · 2023-06-22T14:51:44Z

Yes. Will address changes in next 1 hour. Hope it's fine for you
…
On Thu, Jun 22, 2023, 7:06 PM Zbynek Roubalik @.> wrote: @Indresh2410 https://github.com/Indresh2410 do you think you'll be able to tackle the rest of the problems today? We plan to release a new version in a few hours. — Reply to this email directly, view it on GitHub <#4645 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/A7A3MR7XJAKUA7PDIP6KRR3XMRC6FANCNFSM6AAAAAAY2NKJCA . You are receiving this because you were mentioned.Message ID: _@**.**_>

Thanks!

HI @zroubalik . I've addressed all review comments. Please let me know if any other change is required

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

zroubalik

@Indresh2410 now there are both pkg/mock/mock_secretLister/mock_interfaces.go and pkg/mock/mock_secretlister/mock_interfaces.go.

Could you please fix that?

Signed-off-by: Indresh2410 <130135623+Indresh2410@users.noreply.github.com>

Signed-off-by: Indresh2410 <130135623+Indresh2410@users.noreply.github.com> Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

…2410/keda into fix-restricted-namespace-secrets Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Indresh2410 · 2023-06-22T15:11:24Z

@Indresh2410 now there are both pkg/mock/mock_secretLister/mock_interfaces.go and pkg/mock/mock_secretlister/mock_interfaces.go.

Could you please fix that?

Have fixed it @zroubalik . Thanks

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

zroubalik

LGTM

zroubalik

@Indresh2410 unfortunately there are some static errors and also errors in the unit test. Could you please check?

Signed-off-by: Benjamin Herbert <benjamin@herbert.cc>

CHANGELOG.md

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

…2410/keda into fix-restricted-namespace-secrets

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Indresh2410 · 2023-06-22T15:55:59Z

Hi @zroubalik . Looks like static check is still failing and I would like to take some time to fix the issues. Hope we can merge it to next release ?

Sorry for inconvenience caused

zroubalik · 2023-06-22T15:56:17Z

@Indresh2410 are you able to run the test locally?

=== RUN   TestEnvWithRestrictSecretAccess
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_key
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_name
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
--- FAIL: TestEnvWithRestrictSecretAccess (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_name (0.00s)
=== RUN   TestEnvWithRestrictedNamespace
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_key
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"test-env-key": "secretDataHere"},
          )
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_name
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"mysecretkey": "secretDataHere"},
          )
--- FAIL: TestEnvWithRestrictedNamespace (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_name (0.00s)
FAIL
	github.com/kedacore/keda/v2/pkg/scaling/resolver	coverage: 35.2% of statements
FAIL	github.com/kedacore/keda/v2/pkg/scaling/resolver	0.104s

Indresh2410 · 2023-06-22T15:59:39Z

@Indresh2410 are you able to run the test locally?

=== RUN   TestEnvWithRestrictSecretAccess
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_key
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_name
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
--- FAIL: TestEnvWithRestrictSecretAccess (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_name (0.00s)
=== RUN   TestEnvWithRestrictedNamespace
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_key
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"test-env-key": "secretDataHere"},
          )
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_name
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"mysecretkey": "secretDataHere"},
          )
--- FAIL: TestEnvWithRestrictedNamespace (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_name (0.00s)
FAIL
	github.com/kedacore/keda/v2/pkg/scaling/resolver	coverage: 35.2% of statements
FAIL	github.com/kedacore/keda/v2/pkg/scaling/resolver	0.104s

Exporting variables and running tests worked fine for me @zroubalik

zroubalik · 2023-06-22T16:06:35Z

@Indresh2410 are you able to run the test locally?

=== RUN   TestEnvWithRestrictSecretAccess
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_key
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_name
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
--- FAIL: TestEnvWithRestrictSecretAccess (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_name (0.00s)
=== RUN   TestEnvWithRestrictedNamespace
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_key
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"test-env-key": "secretDataHere"},
          )
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_name
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"mysecretkey": "secretDataHere"},
          )
--- FAIL: TestEnvWithRestrictedNamespace (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_name (0.00s)
FAIL
	github.com/kedacore/keda/v2/pkg/scaling/resolver	coverage: 35.2% of statements
FAIL	github.com/kedacore/keda/v2/pkg/scaling/resolver	0.104s

Exporting variables and running tests worked fine for me @zroubalik

@Indresh2410 your branch is faling (the test) for me locally as well. It is fine if you don't have a time to fix it now, we can postpone it to the next release. Just let me know.

Indresh2410 · 2023-06-22T16:09:27Z

@Indresh2410 are you able to run the test locally?

=== RUN   TestEnvWithRestrictSecretAccess
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_key
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
=== RUN   TestEnvWithRestrictSecretAccess/env_reference_secret_name
    scale_resolvers_test.go:592: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{},
          )
--- FAIL: TestEnvWithRestrictSecretAccess (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_name (0.00s)
=== RUN   TestEnvWithRestrictedNamespace
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_key
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"test-env-key": "secretDataHere"},
          )
=== RUN   TestEnvWithRestrictedNamespace/env_reference_secret_name
    scale_resolvers_test.go:655: Returned env map is different:   map[string]string(
        - 	nil,
        + 	{"mysecretkey": "secretDataHere"},
          )
--- FAIL: TestEnvWithRestrictedNamespace (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_key (0.00s)
    --- FAIL: TestEnvWithRestrictedNamespace/env_reference_secret_name (0.00s)
FAIL
	github.com/kedacore/keda/v2/pkg/scaling/resolver	coverage: 35.2% of statements
FAIL	github.com/kedacore/keda/v2/pkg/scaling/resolver	0.104s

Exporting variables and running tests worked fine for me @zroubalik

@Indresh2410 your branch is faling (the test) for me locally as well. It is fine if you don't have a time to fix it now, we can postpone it to the next release. Just let me know.

Yes @zroubalik . We'll push it as part of next release. Will sort out issues sometime in next few days. Thanks for quick reviews 😃

kedacore#4558) --- Accreditation --- Original PR: kedacore#3828 Borrows from the work originally implemented by: https://github.com/keegancwinchester Note: I would have loved to have pulled the commit from the original branch, but I could not be able to. Documentation MR by original implementor: kedacore/keda-docs#932 --- Fixes --- Fixes # kedacore#3656 --- PR Notes --- Introduce annotation to pause ScaledJobs. Signed-off-by: BenJ <benjamin.jessop@fluxfederation.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

…re#4734)

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

…2410/keda into fix-restricted-namespace-secrets

Indresh2410 · 2023-06-24T06:24:57Z

Hi @zroubalik . Sry for inconvenience caused. Have raised another PR with same changes
#4735

Closing this in favour of the above

Indresh2410 requested a review from a team as a code owner June 5, 2023 04:29

tomkerkhove reviewed Jun 5, 2023

View reviewed changes

JorTurFer reviewed Jun 5, 2023

View reviewed changes

tobotg suggested changes Jun 16, 2023

View reviewed changes

zroubalik reviewed Jun 21, 2023

View reviewed changes

tobotg reviewed Jun 22, 2023

View reviewed changes

pkg/scaling/resolver/scale_resolvers_test.go Outdated Show resolved Hide resolved

zroubalik reviewed Jun 22, 2023

View reviewed changes

tobotg reviewed Jun 22, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

fix: Skip Resolving Secrets From Restricted Namespace

02f810a

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Indresh2410 force-pushed the fix-restricted-namespace-secrets branch from b1d2eec to 02f810a Compare June 22, 2023 14:48

fix: Add extra line

aef5b76

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

fix: Fix case sensitive issue in folder

bf31cdd

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

zroubalik reviewed Jun 22, 2023

View reviewed changes

Indresh2410 and others added 3 commits June 22, 2023 20:38

fix: Remove unwanted file

2227964

Signed-off-by: Indresh2410 <130135623+Indresh2410@users.noreply.github.com>

fix: Remove unwanted file

5d8a276

Signed-off-by: Indresh2410 <130135623+Indresh2410@users.noreply.github.com> Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Merge branch 'fix-restricted-namespace-secrets' of github.com:Indresh…

cf820a0

…2410/keda into fix-restricted-namespace-secrets Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

bump deps (kedacore#4726)

b753245

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

zroubalik approved these changes Jun 22, 2023

View reviewed changes

zroubalik reviewed Jun 22, 2023

View reviewed changes

feat: fix metrics not showing and improve graphs (kedacore#4727)

da2ff6e

Signed-off-by: Benjamin Herbert <benjamin@herbert.cc>

zroubalik reviewed Jun 22, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Indresh2410 added 4 commits June 22, 2023 21:02

fix: Address Semantic Errors

a034df9

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

fix: Skip Resolving Secrets From Restricted Namespace

bfca298

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Merge branch 'fix-restricted-namespace-secrets' of github.com:Indresh…

7bc329b

…2410/keda into fix-restricted-namespace-secrets

fix: Address Semantic Errors

5869ff1

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

flux-benj and others added 6 commits June 22, 2023 18:09

prepare release 2.11.0 (kedacore#4728)

26c3451

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

udpate roadmap with 2.14 (kedacore#4729)

107947c

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

Paused ScaledObject continues after removing pause annotation (kedaco…

04ca96e

…re#4734)

fix: Skip Resolving Secrets From Restricted Namespace

08968b7

Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>

Merge branch 'fix-restricted-namespace-secrets' of github.com:Indresh…

161cf82

…2410/keda into fix-restricted-namespace-secrets

Indresh2410 closed this Jun 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Skip Resolving Secrets From Restricted Namespace #4645

fix: Skip Resolving Secrets From Restricted Namespace #4645

Indresh2410 commented Jun 5, 2023

tomkerkhove left a comment

JorTurFer left a comment

zroubalik commented Jun 6, 2023

JorTurFer commented Jun 11, 2023 •

edited by github-actions bot

Loading

tobotg left a comment

Indresh2410 commented Jun 20, 2023

JorTurFer commented Jun 20, 2023 •

edited

Loading

zroubalik left a comment

Indresh2410 commented Jun 21, 2023 via email

zroubalik commented Jun 21, 2023

Indresh2410 commented Jun 21, 2023

zroubalik commented Jun 22, 2023

zroubalik left a comment

zroubalik commented Jun 22, 2023

Indresh2410 commented Jun 22, 2023 via email

zroubalik commented Jun 22, 2023

Indresh2410 commented Jun 22, 2023

zroubalik left a comment

Indresh2410 commented Jun 22, 2023

zroubalik left a comment

zroubalik left a comment

Indresh2410 commented Jun 22, 2023

zroubalik commented Jun 22, 2023

Indresh2410 commented Jun 22, 2023

zroubalik commented Jun 22, 2023 •

edited

Loading

Indresh2410 commented Jun 22, 2023

Indresh2410 commented Jun 24, 2023

fix: Skip Resolving Secrets From Restricted Namespace #4645

fix: Skip Resolving Secrets From Restricted Namespace #4645

Conversation

Indresh2410 commented Jun 5, 2023

tomkerkhove left a comment

Choose a reason for hiding this comment

JorTurFer left a comment

Choose a reason for hiding this comment

zroubalik commented Jun 6, 2023

JorTurFer commented Jun 11, 2023 • edited by github-actions bot Loading

tobotg left a comment

Choose a reason for hiding this comment

Indresh2410 commented Jun 20, 2023

JorTurFer commented Jun 20, 2023 • edited Loading

zroubalik left a comment

Choose a reason for hiding this comment

Indresh2410 commented Jun 21, 2023 via email

zroubalik commented Jun 21, 2023

Indresh2410 commented Jun 21, 2023

zroubalik commented Jun 22, 2023

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Jun 22, 2023

Indresh2410 commented Jun 22, 2023 via email

zroubalik commented Jun 22, 2023

Indresh2410 commented Jun 22, 2023

zroubalik left a comment

Choose a reason for hiding this comment

Indresh2410 commented Jun 22, 2023

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

Indresh2410 commented Jun 22, 2023

zroubalik commented Jun 22, 2023

Indresh2410 commented Jun 22, 2023

zroubalik commented Jun 22, 2023 • edited Loading

Indresh2410 commented Jun 22, 2023

Indresh2410 commented Jun 24, 2023

JorTurFer commented Jun 11, 2023 •

edited by github-actions bot

Loading

JorTurFer commented Jun 20, 2023 •

edited

Loading

zroubalik commented Jun 22, 2023 •

edited

Loading