Skip to content
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

filter lastAppliedConfig annotation for describe secret #34664

Merged
merged 1 commit into from
Feb 17, 2017
Merged

filter lastAppliedConfig annotation for describe secret #34664

merged 1 commit into from
Feb 17, 2017

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Oct 13, 2016 •
edited by pwittrock
Loading

Temporarily addresses: #23564.
This patch filters out the lastAppliedConfig annotation when describing a secret.

kubectl describe no longer prints the last-applied-configuration annotation for secrets.

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 13, 2016
@mengqiy mengqiy force-pushed the filter_annotation_for_describe_secret branch from 7cf00f5 to 13fa69b Compare October 24, 2016 17:30
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 24, 2016
@mengqiy mengqiy force-pushed the filter_annotation_for_describe_secret branch from 13fa69b to 82e2d00 Compare October 24, 2016 17:37
@mengqiy
Copy link
Member Author

mengqiy commented Oct 24, 2016

@brendandburns Rebased. PTAL

@pwittrock
Copy link
Member

@fabianofranz Reassigned to you. LMK if you don't have time.

fabianofranz
fabianofranz suggested changes Oct 26, 2016
View reviewed changes
pkg/kubectl/describe.go Outdated
@@ -1380,7 +1381,8 @@ func describeSecret(secret *api.Secret) (string, error) {
fmt.Fprintf(out, "Name:\t%s\n", secret.Name)
fmt.Fprintf(out, "Namespace:\t%s\n", secret.Namespace)
printLabelsMultiline(out, "Labels", secret.Labels)
printLabelsMultiline(out, "Annotations", secret.Annotations)
filterMap := map[string]bool{annotations.LastAppliedConfigAnnotation: true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, name it (here and in method arg) as something that makes it clear what the bool means, e.g. skipMap or skipAnnotations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@mengqiy mengqiy force-pushed the filter_annotation_for_describe_secret branch from 82e2d00 to bd9e4f8 Compare October 26, 2016 22:47
@mengqiy
Copy link
Member Author

mengqiy commented Oct 31, 2016

@fabianofranz PTAL

@fabianofranz
Copy link
Contributor

@kubernetes/kubectl @lavalamp PTAL at the proposed strategy since there was some debate in the original issue. This fixes describe but the issue still exists once it's exposed by the api, returns in get -o yaml, etc.

@mengqiy
Copy link
Member Author

mengqiy commented Nov 4, 2016

Any update?

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 10, 2016
@fabianofranz
Copy link
Contributor

@deads2k @lavalamp comments? Trying to figure out if we should go on with this, or not consider an issue based on #23564 (comment).

@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@andronat
Copy link
Contributor

andronat commented Nov 11, 2016 •
edited
Loading

Avoiding printing a field in kubectl hides the real problem. I'm afraid that we might forget this issue if this patch is merged. I don't think this is a good approach.

@lavalamp
Copy link
Member

I agree, I don't think we should do this. @pwittrock's call, I guess.

@pwittrock
Copy link
Member

@andronat @lavalamp kubectl already hides the secret value field in describe, but just missed the value stored in the annotation. IMHO we should be consistent w.r.t whether or not we hide the secret body in describe. This wasn't intended as a general fix to access control of secret data, but instead to make the implementation of kubectl describe secret match the original intent - which is don't print the body of the secret data to a user's console when it is returned by the server.

I don't think this is really urgent. I plan to revisit in a few days.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2016
@mengqiy mengqiy force-pushed the filter_annotation_for_describe_secret branch from bd9e4f8 to f6ef8b1 Compare February 3, 2017 23:02
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 3, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Feb 3, 2017

Addressed comments. PTAL

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2017
@mengqiy mengqiy force-pushed the filter_annotation_for_describe_secret branch from f6ef8b1 to aadcd9b Compare February 4, 2017 01:17
@pwittrock
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2017
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 4, 2017
@adohe-zz
Copy link

adohe-zz commented Feb 4, 2017

LGTM. Will add the approval label.

@adohe-zz
Copy link

adohe-zz commented Feb 4, 2017

/approve

@mengqiy
Copy link
Member Author

mengqiy commented Feb 4, 2017 •
edited
Loading

@adohe @pwittrock Any idea why /approve is not working?

@adohe-zz
Copy link

adohe-zz commented Feb 5, 2017

@ymqytw could be the case sensitivity problem? my id is AdoHe

@pwittrock
Copy link
Member

/approved

@pwittrock
Copy link
Member

@adohe Missing a 'd': approved vs approve

@pwittrock
Copy link
Member

Hm, maybe I am wrong.

@pwittrock
Copy link
Member

/approve

@pwittrock
Copy link
Member

@apelisse Approval doesn't seem to be working :(

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: AdoHe, pwittrock, ymqytw

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2017
@fabianofranz
Copy link
Contributor

@k8s-bot cvm gce e2e test this

@fabianofranz
Copy link
Contributor

@k8s-bot cri e2e test this

@fabianofranz
Copy link
Contributor

@k8s-bot bazel test this

@fabianofranz
Copy link
Contributor

@k8s-bot gce etcd3 e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 40505, 34664, 37036, 40726, 41595)

@k8s-github-robot k8s-github-robot merged commit 4f6b229 into kubernetes:master Feb 17, 2017
@mengqiy mengqiy deleted the filter_annotation_for_describe_secret branch February 21, 2017 19:21
@jtigger jtigger mentioned this pull request Mar 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwittrock pwittrock pwittrock left review comments

@fabianofranz fabianofranz fabianofranz approved these changes

Assignees

@fabianofranz fabianofranz

@adohe-zz adohe-zz

@pwittrock pwittrock

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.