parse resource name before removing deleted secret #17004
Conversation
openshift-ci-robot
added
the
size/S
pkg/oc/cli/secrets/options.go
Outdated
| } | ||
| return names | ||
| } | ||
|
|
||
| // parseResourceName receives either a resource name as either | ||
| // <resource type> / <name> or <name> and returns only the resource <name>. | ||
| func parseResourceName(name string) string { |
There was a problem hiding this comment.
This isn't meant to be generic, right? Maybe we could strip the secret(s)/ prefix explicitly in that case. That would also be more clear in my opinion even if we are certain at this point that name has secret/ as the prefix if it contains a slash. Otherwise, what happens if i oc secret unlink whatever/steve?
test/cmd/secrets.sh
Outdated
| os::cmd::expect_success 'oc create secret generic deleted-secret' | ||
| os::cmd::expect_success 'oc secrets link deployer deleted-secret' | ||
| # confirm our soon-to-be-deleted secret has been linked | ||
| os::cmd::expect_success 'oc get serviceaccounts/deployer -o yaml |grep -q deleted-secret' |
There was a problem hiding this comment.
Use jsonpath and validate that no secret is linked.
oc get serviceaccount deployer -o jsonpath='{.secrets[?(@.name=="deleted-secret")]}'
juanvallejo
force-pushed
the
jvallejo/parse-rsrs-arg-when-removing-deleted-secret
branch
from
cd1ce5b
to
e04dc8d
Compare
openshift-ci-robot
added
size/M
|
@stevekuznetsov thanks for the feedback, updated tests to use jsonpath; added check to ensure resource type is |
juanvallejo
force-pushed
the
jvallejo/parse-rsrs-arg-when-removing-deleted-secret
branch
from
e04dc8d
to
c0e6b17
Compare
pkg/oc/cli/secrets/options.go
Outdated
| return name | ||
| } | ||
|
|
||
| if segs[0] == "secret" || segs[0] == "secrets" { |
There was a problem hiding this comment.
If you have a slash and this is not true, there is an error, right?
There was a problem hiding this comment.
Right, the name will be returned as-is by this function (e.g. pods/my-secret), and the user will end up seeing
secret "pod/my-secret" not found
error: No valid secrets found or secrets not linked to service account
There was a problem hiding this comment.
OK. I meant more forcefully -- the error really is that a secret name cannot start with pod/ not that pod/my-secret was not found
There was a problem hiding this comment.
Sounds good, added a check for this in the Validate method
test/cmd/secrets.sh
Outdated
| os::cmd::expect_success 'oc secrets link deployer deleted-secret' | ||
| # confirm our soon-to-be-deleted secret has been linked | ||
| os::cmd::expect_success_and_text "oc get serviceaccount deployer -o jsonpath='{.secrets[?(@.name==\"deleted-secret\")]}'" 'deleted\-secret' | ||
| os::cmd::expect_success 'oc get serviceaccounts/deployer -o yaml |grep -q deleted-secret' |
There was a problem hiding this comment.
So when you unlink you should see no text here
There was a problem hiding this comment.
In the JSONpath that is -- there should be no yaml grep in the final tests
There was a problem hiding this comment.
Yeah, this is testing that the secret has linked. A few lines below from this, I test unlinking by calling os::cmd::expect_success_and_not_text
There was a problem hiding this comment.
Also, forgot to remove the old grep check. Will remove that
Although unlinking deleted secrets from a serviceaccount is currently supported, `oc secret unlink` failed to unlink a deleted secret if its name was specified as secrets/deleted-secret-name. This patch parses each secret's name, removing the <secrets/> segment before appending it to a string set of removed secret names.
juanvallejo
force-pushed
the
jvallejo/parse-rsrs-arg-when-removing-deleted-secret
branch
from
c0e6b17
to
a17af0e
Compare
|
/approve |
openshift-merge-robot
added
the
approved
openshift-ci-robot
added
the
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juanvallejo, stevekuznetsov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test extended_conformance_install_update |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue. |
juanvallejo
deleted the
jvallejo/parse-rsrs-arg-when-removing-deleted-secret
branch
| @@ -68,6 +69,16 @@ func (o SecretOptions) Validate() error { | |||
| return errors.New("KubeCoreClient must be present") | |||
| } | |||
|
|
|||
| // if any secret names are of the form <resource>/<name>, | |||
| // ensure <resource> is a secret. | |||
| for _, secretName := range o.SecretNames { | |||
There was a problem hiding this comment.
Why isn't this using standard resource builder?
There was a problem hiding this comment.
Why isn't this using standard resource builder?
The resource builder is used in the GetSecrets method. The code block here validates that a resource kind is secret if it is specified as <kind>/<name>, before getting to the builder, which ignores NotFound errors in order to allow deleted secrets to still be unlinked. Without this check, if a user provides a command such as: oc secret unlink sa_name pods/not_a_secret, this line would create a secret with the name pods/not_a_secret.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1457602
Although unlinking deleted secrets from a serviceaccount is currently
supported,
oc secret unlinkfails to unlink a deleted secret if itsname is specified as
secrets/deleted-secret-name.This patch parses each secret's name, removing the
secrets/segmentbefore appending it to a string set of removed secret names.
cc @openshift/cli-review