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.
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
OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1932
OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1932
Conversation
@p0lyn0mial: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
@p0lyn0mial: This pull request references Jira Issue OCPBUGS-31384, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
/cc @benluddy |
/retest-required |
// | ||
// additionally, it should be noted that default values might also be applied in some cases. | ||
// (https://github.com/openshift/kubernetes/blob/258f1d5fb6491ba65fd8201c827e179432430627/pkg/apis/core/v1/defaults.go#L280-L284) | ||
if oldSecret.Type != core.SecretTypeTLS && newSecret.Type == core.SecretTypeTLS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we care about the type of the old secret object - if newSecret.Type == core.SecretTypeTLS {
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think so, initially we didn't want to allow changes from oldSecret.Type = core.SecretTypeTLS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I think that maybe we should only allow to transition from “”, Opaque and SecretTypeTLS, especially that https://github.com/kubernetes/kubernetes/blob/8628c3c4da6746b1dc967cc520b189a04ebd78d1/pkg/apis/core/validation/validation.go#L6393
t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot)) | ||
} | ||
|
||
// verify that kbernetes.io/tls validation ar enforced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: are
8662730
to
852fcc4
Compare
@p0lyn0mial: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
852fcc4
to
652a9ef
Compare
@p0lyn0mial: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
/remove-label backports/unvalidated-commits /hold /cc @soltysh |
/retest unit |
@p0lyn0mial: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
// "", core.SecretTypeOpaque seems safe because | ||
// https://github.com/kubernetes/kubernetes/blob/8628c3c4da6746b1dc967cc520b189a04ebd78d1/pkg/apis/core/validation/validation.go#L6393 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should never see ""
in an the old secret of an update, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just playing it safe here, considering that these secrets could be created and then never changed, I don't think there is a way to ensure that resources were created only with one type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if a client serializes a Secret that omits the type field or encodes it as ""
, its type would have been defaulted to Opaque. How would someone persist a Secret with type ""?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the default value applied on the server-side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. How would a Secret with type "" be persisted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to an incorrect patch applied to our server :) ?
Would you be okay to allow core.SecretTypeOpaque
and SecretTypeTLS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I just don't want to conceal any other potential issues with type defaulting or validation. If we find out that there are clusters with empty-type secrets that we should migrate then we can deal with that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for your feedback, pushed, PTAL.
squash with the previous PRs during the rebase openshift#1924 openshift#1929
652a9ef
to
4665c05
Compare
@p0lyn0mial: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
/lgtm |
/hold cancel |
@p0lyn0mial: trigger 5 job(s) of type blocking for the ci release of OCP 4.16
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9eb3be20-f296-11ee-850b-9b978875664d-0 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, p0lyn0mial, tkashem The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/remove-label backports/unvalidated-commits |
/retest-required |
2 similar comments
/retest-required |
/retest-required |
/hold cancel No regressions found in payload tests |
@p0lyn0mial: This pull request references Jira Issue OCPBUGS-31384, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@p0lyn0mial: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@p0lyn0mial: Jira Issue OCPBUGS-31384: Some pull requests linked via external trackers have merged:
The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-31384 has not been moved to the MODIFIED state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
[ART PR BUILD NOTIFIER] This PR has been included in build openshift-enterprise-pod-container-v4.16.0-202404050813.p0.ge994e5d.assembly.stream.el9 for distgit openshift-enterprise-pod. |
This PR relaxes the restrictions on allowed type mutation transitions.
initially, the check was stricter.
however, due to the platform's long history (spanning several years)
and the complexity of ensuring that resources were consistently created with only one type,
it is now permissible for (SecretTypeTLS, core.SecretTypeOpaque) type to transition to "kubernetes.io/tls".
additionally, it should be noted that default values might also be applied in some cases.
(
kubernetes/pkg/apis/core/v1/defaults.go
Lines 280 to 284 in 258f1d5