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

fix: add missing owner on knative-serving-cert #1185

Open
wants to merge 1 commit into
base: incubation
Choose a base branch
from
Open

fix: add missing owner on knative-serving-cert #1185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions pkg/cluster/cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func generateCertificate(addr string) ([]byte, []byte, error) {
}

// PropagateDefaultIngressCertificate copies ingress cert secrets from openshift-ingress ns to given namespace.
func PropagateDefaultIngressCertificate(ctx context.Context, c client.Client, secretName, namespace string) error {
func PropagateDefaultIngressCertificate(ctx context.Context, c client.Client, secretName, namespace string, metaOptions ...MetaOptions) error {
defaultIngressCtrl, err := FindAvailableIngressController(ctx, c)
if err != nil {
return fmt.Errorf("failed to get ingress controller: %w", err)
Expand All @@ -137,6 +137,9 @@ func PropagateDefaultIngressCertificate(ctx context.Context, c client.Client, se
return err
}

if err := ApplyMetaOptions(defaultIngressSecret, metaOptions...); err != nil {
return err
}
return copySecretToNamespace(ctx, c, defaultIngressSecret, secretName, namespace)
}

Expand Down Expand Up @@ -169,8 +172,9 @@ func GetSecret(ctx context.Context, c client.Client, namespace, name string) (*c
func copySecretToNamespace(ctx context.Context, c client.Client, secret *corev1.Secret, newSecretName, namespace string) error {
newSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: newSecretName,
Namespace: namespace,
Name: newSecretName,
Namespace: namespace,
OwnerReferences: secret.OwnerReferences,
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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

},
Data: secret.Data,
Type: secret.Type,
Expand Down
2 changes: 1 addition & 1 deletion pkg/feature/serverless/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func ServingCertificateResource(ctx context.Context, cli client.Client, f *featu
case infrav1.Provided:
return nil
default:
return cluster.PropagateDefaultIngressCertificate(ctx, cli, secretData.Name, secretData.Namespace)
return cluster.PropagateDefaultIngressCertificate(ctx, cli, secretData.Name, secretData.Namespace, feature.DefaultMetaOptions(f)...)
}
}

Expand Down
Loading