-
Notifications
You must be signed in to change notification settings - Fork 143
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
fix: add missing owner on knative-serving-cert #1185
base: incubation
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Namespace: namespace, | ||
Name: newSecretName, | ||
Namespace: namespace, | ||
OwnerReferences: secret.OwnerReferences, |
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.
This won't work if source and target secrets are in different namespaces. OwnerReferences
can only be for local k8s resources.
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.
not sure i understand your comment here
the change is for this function to create a new Secret in the namespace with the Data and Type from the old secret also to set the new Secret with OwnerReferences as the origin FeatureTracker
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.
Unless I missed something, it looks like the new secret will have the same owner reference as the old secret with the line:
OwnerReferences: secret.OwnerReferences,
Which, won't work if secret
and newSecret
are in different namespaces. For example, if the source secret is the default load balancer ingress cert from the ingress operator namespace being copied to another namespace.
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.
my idea is to set the OwnerReferences: secret.OwnerReferences,
when we create the new secret.
but this value of secret.OwnerReference
is not really the owner from old secret but set in the caller by
if err := ApplyMetaOptions(defaultIngressSecret, metaOptions...); err != nil {
return err
}
and passed from feature.OwnedBy(f))
In kserve's case:
ServingCertificateResource
() if it is using type "OpenshiftDefaultIngress"=> f *feature.Feature is added in metaOptions by ApplyMetaOptions
=> copySecretToNamespace
use it as owner on new secret.
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.
some updates done after above discussion :
owner is only set within copySecretToNamespace
() and passed from upper callers: PropagateDefaultIngressCertificate
=>copySecretToNamespace
=>copySecretToNamespace
bf1b7ae
to
fdd0cd0
Compare
f808649
to
e2d4cf1
Compare
- why using default ingress cert, owner is not set to FTer: serverless-serving-gateway Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
Description
(out of this comment i had in another PR #1165 (comment))
How Has This Been Tested?
local build: quay.io/wenzhou/opendatahub-operator:2.16.98
Screenshot or short clip
Merge criteria