From bb2725fba25c2486e9babe1d0f2b223e241229d0 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Tue, 17 Jan 2023 11:56:11 -0600 Subject: [PATCH] add a deletion approval flow with a validation webhook (#3678) --- commands/alpha/repo/reg/command.go | 2 +- commands/alpha/repo/unreg/command.go | 2 +- commands/alpha/rpkg/clone/command.go | 2 +- commands/alpha/rpkg/copy/command.go | 2 +- commands/alpha/rpkg/del/command.go | 2 +- commands/alpha/rpkg/init/command.go | 2 +- commands/alpha/rpkg/propose/command.go | 2 +- commands/alpha/rpkg/update/command.go | 2 +- e2e/testdata/porch/rpkg-lifecycle/config.yaml | 13 +- internal/util/porch/client.go | 16 +- porch/api/porch/types_packagerevisions.go | 7 +- .../porch/v1alpha1/types_packagerevisions.go | 7 +- porch/api/porch/v1alpha1/util.go | 19 + .../downstreampackage_controller.go | 4 +- .../controllers/rootsyncrollout/controller.go | 2 +- porch/deployments/porch/3-porch-server.yaml | 11 + porch/deployments/porch/5-rbac.yaml | 5 +- porch/pkg/apiserver/apiserver.go | 11 + porch/pkg/apiserver/webhooks.go | 371 ++++++++++++++++++ porch/pkg/apiserver/webhooks_test.go | 114 ++++++ porch/pkg/cache/repository.go | 11 +- porch/pkg/cache/util.go | 2 +- porch/pkg/engine/engine.go | 19 +- porch/pkg/engine/fake/packagerevision.go | 4 + porch/pkg/git/draft.go | 4 +- porch/pkg/git/git.go | 55 +++ porch/pkg/git/package.go | 63 ++- porch/pkg/git/ref.go | 24 +- porch/pkg/oci/loader.go | 2 + porch/pkg/oci/mutate.go | 2 +- porch/pkg/oci/oci.go | 25 ++ .../porch/packagerevision_approval_test.go | 15 +- .../porch/packagerevisions_approval.go | 25 +- porch/pkg/registry/porch/strategy.go | 16 +- porch/pkg/repository/repository.go | 5 + porch/pkg/repository/util.go | 6 +- porch/test/e2e/e2e_test.go | 80 +++- 37 files changed, 883 insertions(+), 71 deletions(-) create mode 100644 porch/api/porch/v1alpha1/util.go create mode 100644 porch/pkg/apiserver/webhooks.go create mode 100644 porch/pkg/apiserver/webhooks_test.go diff --git a/commands/alpha/repo/reg/command.go b/commands/alpha/repo/reg/command.go index 53c7c72ad3..11d0f0b969 100644 --- a/commands/alpha/repo/reg/command.go +++ b/commands/alpha/repo/reg/command.go @@ -88,7 +88,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/repo/unreg/command.go b/commands/alpha/repo/unreg/command.go index c235e004fc..1ef8dbc34c 100644 --- a/commands/alpha/repo/unreg/command.go +++ b/commands/alpha/repo/unreg/command.go @@ -71,7 +71,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/rpkg/clone/command.go b/commands/alpha/rpkg/clone/command.go index 969f40e814..c25f5b991f 100644 --- a/commands/alpha/rpkg/clone/command.go +++ b/commands/alpha/rpkg/clone/command.go @@ -92,7 +92,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/rpkg/copy/command.go b/commands/alpha/rpkg/copy/command.go index ee5f90c311..a47b87b27c 100644 --- a/commands/alpha/rpkg/copy/command.go +++ b/commands/alpha/rpkg/copy/command.go @@ -69,7 +69,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/rpkg/del/command.go b/commands/alpha/rpkg/del/command.go index 2318a2f593..ed2476d188 100644 --- a/commands/alpha/rpkg/del/command.go +++ b/commands/alpha/rpkg/del/command.go @@ -71,7 +71,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/rpkg/init/command.go b/commands/alpha/rpkg/init/command.go index 158785e597..43ae99deb9 100644 --- a/commands/alpha/rpkg/init/command.go +++ b/commands/alpha/rpkg/init/command.go @@ -80,7 +80,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/rpkg/propose/command.go b/commands/alpha/rpkg/propose/command.go index 45e361486e..1829a93a21 100644 --- a/commands/alpha/rpkg/propose/command.go +++ b/commands/alpha/rpkg/propose/command.go @@ -69,7 +69,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - client, err := porch.CreateClient(r.cfg) + client, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/commands/alpha/rpkg/update/command.go b/commands/alpha/rpkg/update/command.go index 1e5cca06e6..4d67d4882d 100644 --- a/commands/alpha/rpkg/update/command.go +++ b/commands/alpha/rpkg/update/command.go @@ -72,7 +72,7 @@ type runner struct { func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" - c, err := porch.CreateClient(r.cfg) + c, err := porch.CreateClientWithFlags(r.cfg) if err != nil { return errors.E(op, err) } diff --git a/e2e/testdata/porch/rpkg-lifecycle/config.yaml b/e2e/testdata/porch/rpkg-lifecycle/config.yaml index 8383b257b5..f7f477963d 100644 --- a/e2e/testdata/porch/rpkg-lifecycle/config.yaml +++ b/e2e/testdata/porch/rpkg-lifecycle/config.yaml @@ -90,13 +90,8 @@ commands: - delete - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 - --namespace=rpkg-lifecycle - stderr: | - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 deleted - - args: - - alpha - - rpkg - - get - - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 - - --namespace=rpkg-lifecycle - stderr: "Error: the server could not find the requested resource (get packagerevisions.porch.kpt.dev git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034) \n" exitCode: 1 + stderr: | + git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 failed (admission webhook "packagerevdeletion.google.com" denied the request: failed to delete package revision "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion) + Error: errors: + admission webhook "packagerevdeletion.google.com" denied the request: failed to delete package revision "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion diff --git a/internal/util/porch/client.go b/internal/util/porch/client.go index ac403eb8c0..87b2b62ed4 100644 --- a/internal/util/porch/client.go +++ b/internal/util/porch/client.go @@ -30,12 +30,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func CreateClient(flags *genericclioptions.ConfigFlags) (client.Client, error) { - config, err := flags.ToRESTConfig() - if err != nil { - return nil, err - } - +func CreateClient(config *rest.Config) (client.Client, error) { scheme, err := createScheme() if err != nil { return nil, err @@ -52,6 +47,15 @@ func CreateClient(flags *genericclioptions.ConfigFlags) (client.Client, error) { return c, nil } +func CreateClientWithFlags(flags *genericclioptions.ConfigFlags) (client.Client, error) { + config, err := flags.ToRESTConfig() + if err != nil { + return nil, err + } + + return CreateClient(config) +} + func CreateDynamicClient(flags *genericclioptions.ConfigFlags) (client.WithWatch, error) { config, err := flags.ToRESTConfig() if err != nil { diff --git a/porch/api/porch/types_packagerevisions.go b/porch/api/porch/types_packagerevisions.go index 3c65483d11..50eff63a8b 100644 --- a/porch/api/porch/types_packagerevisions.go +++ b/porch/api/porch/types_packagerevisions.go @@ -44,9 +44,10 @@ type PackageRevisionList struct { type PackageRevisionLifecycle string const ( - PackageRevisionLifecycleDraft PackageRevisionLifecycle = "Draft" - PackageRevisionLifecycleProposed PackageRevisionLifecycle = "Proposed" - PackageRevisionLifecyclePublished PackageRevisionLifecycle = "Published" + PackageRevisionLifecycleDraft PackageRevisionLifecycle = "Draft" + PackageRevisionLifecycleProposed PackageRevisionLifecycle = "Proposed" + PackageRevisionLifecyclePublished PackageRevisionLifecycle = "Published" + PackageRevisionLifecycleDeletionProposed PackageRevisionLifecycle = "DeletionProposed" ) type WorkspaceName string diff --git a/porch/api/porch/v1alpha1/types_packagerevisions.go b/porch/api/porch/v1alpha1/types_packagerevisions.go index 1f018d71ed..cb0f55f58f 100644 --- a/porch/api/porch/v1alpha1/types_packagerevisions.go +++ b/porch/api/porch/v1alpha1/types_packagerevisions.go @@ -52,9 +52,10 @@ type PackageRevisionList struct { type PackageRevisionLifecycle string const ( - PackageRevisionLifecycleDraft PackageRevisionLifecycle = "Draft" - PackageRevisionLifecycleProposed PackageRevisionLifecycle = "Proposed" - PackageRevisionLifecyclePublished PackageRevisionLifecycle = "Published" + PackageRevisionLifecycleDraft PackageRevisionLifecycle = "Draft" + PackageRevisionLifecycleProposed PackageRevisionLifecycle = "Proposed" + PackageRevisionLifecyclePublished PackageRevisionLifecycle = "Published" + PackageRevisionLifecycleDeletionProposed PackageRevisionLifecycle = "DeletionProposed" ) type WorkspaceName string diff --git a/porch/api/porch/v1alpha1/util.go b/porch/api/porch/v1alpha1/util.go new file mode 100644 index 0000000000..78ff92fb5c --- /dev/null +++ b/porch/api/porch/v1alpha1/util.go @@ -0,0 +1,19 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License + +package v1alpha1 + +func LifecycleIsPublished(lifecycle PackageRevisionLifecycle) bool { + return lifecycle == PackageRevisionLifecyclePublished || lifecycle == PackageRevisionLifecycleDeletionProposed +} diff --git a/porch/controllers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go b/porch/controllers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go index a9d4e54611..4c5da72f87 100644 --- a/porch/controllers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go +++ b/porch/controllers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go @@ -187,7 +187,7 @@ func (r *DownstreamPackageReconciler) findAndUpdateExistingRevision(ctx context. return downstream, nil } - if downstream.Spec.Lifecycle == porchapi.PackageRevisionLifecyclePublished { + if porchapi.LifecycleIsPublished(downstream.Spec.Lifecycle) { var err error downstream, err = r.copyPublished(ctx, downstream, dp) if err != nil { @@ -231,7 +231,7 @@ func (r *DownstreamPackageReconciler) getDownstreamPR(dp *api.DownstreamPackage, // Check if this PR is a draft. We should only have one draft created by this controller at a time, // so we can just return it. - if pr.Spec.Lifecycle != porchapi.PackageRevisionLifecyclePublished { + if !porchapi.LifecycleIsPublished(pr.Spec.Lifecycle) { return &pr } else { latestPublished, latestVersion = compare(&pr, latestPublished, latestVersion) diff --git a/porch/controllers/rootsyncrollouts/pkg/controllers/rootsyncrollout/controller.go b/porch/controllers/rootsyncrollouts/pkg/controllers/rootsyncrollout/controller.go index fb6b91642c..711bd933bc 100644 --- a/porch/controllers/rootsyncrollouts/pkg/controllers/rootsyncrollout/controller.go +++ b/porch/controllers/rootsyncrollouts/pkg/controllers/rootsyncrollout/controller.go @@ -320,7 +320,7 @@ func (r *RootSyncRolloutReconciler) getPackages(ctx context.Context, rollout *v1 packageRevision := packageRevisionList.Items[i] packageName := packageRevision.Spec.PackageName // TODO: See if we can handle this with a FieldSelector on packagerevisions. - if packageRevision.Spec.Lifecycle != porchapi.PackageRevisionLifecyclePublished { + if !porchapi.LifecycleIsPublished(packageRevision.Spec.Lifecycle) { continue } if _, found := packageMap[packageName]; !found { diff --git a/porch/deployments/porch/3-porch-server.yaml b/porch/deployments/porch/3-porch-server.yaml index a1f5fd29ea..3fad607f70 100644 --- a/porch/deployments/porch/3-porch-server.yaml +++ b/porch/deployments/porch/3-porch-server.yaml @@ -38,6 +38,8 @@ spec: volumes: - name: cache-volume emptyDir: {} + - name: webhook-certs + emptyDir: {} containers: - name: porch-server # Update image to the image of your porch apiserver build. @@ -52,12 +54,16 @@ spec: volumeMounts: - mountPath: /cache name: cache-volume + - mountPath: /etc/webhook/certs + name: webhook-certs env: # Uncomment to enable trace-reporting to jaeger #- name: OTEL # value: otel://jaeger-oltp:4317 - name: OTEL_SERVICE_NAME value: porch-server + - name: CERT_STORAGE_DIR + value: "/etc/webhook/certs" args: - --function-runner=function-runner:9445 - --cache-directory=/cache @@ -73,5 +79,10 @@ spec: - port: 443 protocol: TCP targetPort: 443 + name: api + - port: 8443 + protocol: TCP + targetPort: 8443 + name: webhooks selector: app: porch-server diff --git a/porch/deployments/porch/5-rbac.yaml b/porch/deployments/porch/5-rbac.yaml index c0aa38d008..b0ea833705 100644 --- a/porch/deployments/porch/5-rbac.yaml +++ b/porch/deployments/porch/5-rbac.yaml @@ -23,13 +23,16 @@ rules: - apiGroups: ["admissionregistration.k8s.io"] resources: ["mutatingwebhookconfigurations", "validatingwebhookconfigurations"] - verbs: ["get", "watch", "list"] + verbs: ["get", "watch", "list", "create", "patch", "delete"] - apiGroups: ["porch.kpt.dev"] resources: ["functions"] verbs: ["get", "list", "watch", "create", "update", "patch"] - apiGroups: ["config.porch.kpt.dev"] resources: ["repositories", "repositories/status"] verbs: ["get", "list", "watch", "create", "update", "patch"] + - apiGroups: ["porch.kpt.dev"] + resources: ["packagerevisions", "packagerevisions/status"] + verbs: ["get", "list"] - apiGroups: ["config.porch.kpt.dev"] resources: ["packagerevs", "packagerevs/status"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] diff --git a/porch/pkg/apiserver/apiserver.go b/porch/pkg/apiserver/apiserver.go index 0f70ba3d66..0e4634d027 100644 --- a/porch/pkg/apiserver/apiserver.go +++ b/porch/pkg/apiserver/apiserver.go @@ -17,6 +17,7 @@ package apiserver import ( "context" "fmt" + "os" "github.com/GoogleContainerTools/kpt/internal/fnruntime" "github.com/GoogleContainerTools/kpt/porch/api/porch/install" @@ -39,6 +40,7 @@ import ( corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -277,5 +279,14 @@ func (c completedConfig) New() (*PorchServer, error) { func (s *PorchServer) Run(ctx context.Context) error { porch.RunBackground(ctx, s.coreClient, s.cache) + certStorageDir, found := os.LookupEnv("CERT_STORAGE_DIR") + if found && certStorageDir != "" { + if err := setupWebhooks(ctx, certStorageDir); err != nil { + klog.Errorf("%v\n", err) + return err + } + } else { + klog.Infoln("Cert storage dir not provided, skipping webhook setup") + } return s.GenericAPIServer.PrepareRun().Run(ctx.Done()) } diff --git a/porch/pkg/apiserver/webhooks.go b/porch/pkg/apiserver/webhooks.go new file mode 100644 index 0000000000..ac12708d71 --- /dev/null +++ b/porch/pkg/apiserver/webhooks.go @@ -0,0 +1,371 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package apiserver + +import ( + "bytes" + "context" + cryptorand "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/json" + "encoding/pem" + "fmt" + "io/ioutil" + "math/big" + "net/http" + "os" + "path/filepath" + "time" + + "github.com/GoogleContainerTools/kpt/internal/util/porch" + "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1" + admissionv1 "k8s.io/api/admission/v1" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/client-go/kubernetes" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/config" +) + +const ( + webhookServicePort = 8443 + serverEndpoint = "/validate-deletion" +) + +func setupWebhooks(ctx context.Context, certStorageDir string) error { + caBytes, err := createCerts(certStorageDir) + if err != nil { + return err + } + if err := createValidatingWebhook(ctx, caBytes); err != nil { + return err + } + if err := runWebhookServer(certStorageDir); err != nil { + return err + } + return nil +} + +func createCerts(certStorageDir string) ([]byte, error) { + klog.Infoln("creating self-signing TLS cert and key ") + dnsNames := []string{"api", + "api.porch-system", "api.porch-system.svc"} + commonName := "api.porch-system.svc" + + var caPEM, serverCertPEM, serverPrivateKeyPEM *bytes.Buffer + // CA config + ca := &x509.Certificate{ + SerialNumber: big.NewInt(2020), + Subject: pkix.Name{ + Organization: []string{"google.com"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().AddDate(1, 0, 0), + IsCA: true, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + BasicConstraintsValid: true, + } + + privateKey, err := rsa.GenerateKey(cryptorand.Reader, 4096) + if err != nil { + return nil, err + } + caBytes, err := x509.CreateCertificate(cryptorand.Reader, ca, ca, &privateKey.PublicKey, privateKey) + if err != nil { + return nil, err + } + caPEM = new(bytes.Buffer) + _ = pem.Encode(caPEM, &pem.Block{ + Type: "CERTIFICATE", + Bytes: caBytes, + }) + + // server cert config + cert := &x509.Certificate{ + DNSNames: dnsNames, + SerialNumber: big.NewInt(1658), + Subject: pkix.Name{ + CommonName: commonName, + Organization: []string{"google.com"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().AddDate(1, 0, 0), + SubjectKeyId: []byte{1, 2, 3, 4, 6}, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature, + } + + serverPrivateKey, err := rsa.GenerateKey(cryptorand.Reader, 4096) + if err != nil { + return nil, err + } + serverCertBytes, err := x509.CreateCertificate(cryptorand.Reader, cert, ca, &serverPrivateKey.PublicKey, privateKey) + if err != nil { + return nil, err + } + serverCertPEM = new(bytes.Buffer) + _ = pem.Encode(serverCertPEM, &pem.Block{ + Type: "CERTIFICATE", + Bytes: serverCertBytes, + }) + serverPrivateKeyPEM = new(bytes.Buffer) + _ = pem.Encode(serverPrivateKeyPEM, &pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(serverPrivateKey), + }) + + err = os.MkdirAll(certStorageDir, 0666) + if err != nil { + return nil, err + } + err = WriteFile(filepath.Join(certStorageDir, "tls.crt"), serverCertPEM.Bytes()) + if err != nil { + return nil, err + } + err = WriteFile(filepath.Join(certStorageDir, "tls.key"), serverPrivateKeyPEM.Bytes()) + if err != nil { + return nil, err + } + + return caPEM.Bytes(), nil +} + +// WriteFile writes data in the file at the given path +func WriteFile(filepath string, c []byte) error { + f, err := os.Create(filepath) + if err != nil { + return err + } + defer f.Close() + + _, err = f.Write(c) + if err != nil { + return err + } + return nil +} + +func createValidatingWebhook(ctx context.Context, caCert []byte) error { + klog.Infoln("Creating validating webhook") + + cfg := ctrl.GetConfigOrDie() + kubeClient, err := kubernetes.NewForConfig(cfg) + if err != nil { + return fmt.Errorf("failed to setup kubeClient: %v", err) + } + + var ( + webhookNamespace = "porch-system" + validationCfgName = "packagerev-deletion-validating-webhook" + webhookService = "api" + path = serverEndpoint + fail = admissionregistrationv1.Fail + none = admissionregistrationv1.SideEffectClassNone + port = int32(webhookServicePort) + ) + + validateConfig := &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: validationCfgName, + }, + Webhooks: []admissionregistrationv1.ValidatingWebhook{{ + Name: "packagerevdeletion.google.com", + ClientConfig: admissionregistrationv1.WebhookClientConfig{ + CABundle: caCert, // CA bundle created earlier + Service: &admissionregistrationv1.ServiceReference{ + Name: webhookService, + Namespace: webhookNamespace, + Path: &path, + Port: &port, + }, + }, + Rules: []admissionregistrationv1.RuleWithOperations{{Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Delete}, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"porch.kpt.dev"}, + APIVersions: []string{"v1alpha1"}, + Resources: []string{"packagerevisions"}, + }, + }}, + AdmissionReviewVersions: []string{"v1", "v1beta1"}, + SideEffects: &none, + FailurePolicy: &fail, + }}, + } + + if err := kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(ctx, validationCfgName, metav1.DeleteOptions{}); err != nil { + klog.Error("failed to delete existing webhook: %w", err) + } + + if _, err := kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(ctx, validateConfig, + metav1.CreateOptions{}); err != nil { + klog.Infoln("failed to create validating webhook for package revision deletion: %s\n", err.Error()) + return err + } + + return nil +} + +func runWebhookServer(certStorageDir string) error { + certFile := filepath.Join(certStorageDir, "tls.crt") + keyFile := filepath.Join(certStorageDir, "tls.key") + + cert, err := tls.LoadX509KeyPair(certFile, keyFile) + if err != nil { + return err + } + klog.Infoln("Starting webhook server") + http.HandleFunc(serverEndpoint, validateDeletion) + server := http.Server{ + Addr: fmt.Sprintf(":%d", webhookServicePort), + TLSConfig: &tls.Config{ + Certificates: []tls.Certificate{cert}, + }, + } + go func() { + err = server.ListenAndServeTLS("", "") + if err != nil { + klog.Errorf("could not start server: %w", err) + } + }() + return err +} + +func validateDeletion(w http.ResponseWriter, r *http.Request) { + klog.Infoln("received request to validate deletion") + + admissionReviewRequest, err := decodeAdmissionReview(r) + if err != nil { + errMsg := fmt.Sprintf("error getting admission review from request: %v", err) + writeErr(errMsg, &w) + return + } + + // Verify that we have a PackageRevision resource + pkgRevGVK := metav1.GroupVersionResource{Group: "porch.kpt.dev", Version: "v1alpha1", Resource: "packagerevisions"} + if admissionReviewRequest.Request.Resource != pkgRevGVK { + errMsg := fmt.Sprintf("did not receive PackageRevision, got %s", admissionReviewRequest.Request.Resource.Resource) + writeErr(errMsg, &w) + return + } + + // Get the package revision using the name and namespace from the request. + porchClient, err := createPorchClient() + if err != nil { + errMsg := fmt.Sprintf("could not create porch client: %v", err) + writeErr(errMsg, &w) + return + } + pr := v1alpha1.PackageRevision{} + if err := porchClient.Get(context.Background(), client.ObjectKey{ + Namespace: admissionReviewRequest.Request.Namespace, + Name: admissionReviewRequest.Request.Name, + }, &pr); err != nil { + klog.Errorf("could not get package revision: %s", err.Error()) + } + + admissionResponse := &admissionv1.AdmissionResponse{} + if pr.Spec.Lifecycle == v1alpha1.PackageRevisionLifecyclePublished { + admissionResponse.Allowed = false + admissionResponse.Result = &metav1.Status{ + Status: "Failure", + Message: fmt.Sprintf("failed to delete package revision %q: published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion", pr.Name), + Reason: "Published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion.", + } + } else { + admissionResponse.Allowed = true + admissionResponse.Result = &metav1.Status{ + Status: "Success", + Message: fmt.Sprintf("Successfully deleted package revision %q", pr.Name), + } + } + + resp, err := constructResponse(admissionResponse, admissionReviewRequest) + if err != nil { + errMsg := fmt.Sprintf("error constructing response: %v", err) + writeErr(errMsg, &w) + return + } + + w.Header().Set("Content-Type", "application/json") + w.Write(resp) +} + +func decodeAdmissionReview(r *http.Request) (*admissionv1.AdmissionReview, error) { + if r.Header.Get("Content-Type") != "application/json" { + return nil, fmt.Errorf("expected Content-Type 'application/json'") + } + var requestData []byte + if r.Body != nil { + var err error + requestData, err = ioutil.ReadAll(r.Body) + if err != nil { + return nil, err + } + } + admissionReviewRequest := &admissionv1.AdmissionReview{} + deserializer := serializer.NewCodecFactory(runtime.NewScheme()).UniversalDeserializer() + if _, _, err := deserializer.Decode(requestData, nil, admissionReviewRequest); err != nil { + return nil, err + } + if admissionReviewRequest.Request == nil { + return nil, fmt.Errorf("admission review request is empty") + } + return admissionReviewRequest, nil +} + +func constructResponse(response *admissionv1.AdmissionResponse, + request *admissionv1.AdmissionReview) ([]byte, error) { + var admissionReviewResponse admissionv1.AdmissionReview + admissionReviewResponse.Response = response + admissionReviewResponse.SetGroupVersionKind(request.GroupVersionKind()) + admissionReviewResponse.Response.UID = request.Request.UID + + resp, err := json.Marshal(admissionReviewResponse) + if err != nil { + return nil, fmt.Errorf("error marshalling response json: %v", err) + } + return resp, nil +} + +func createPorchClient() (client.Client, error) { + cfg, err := config.GetConfig() + if err != nil { + klog.Errorf("could not get config: %s", err.Error()) + return nil, err + } + porchClient, err := porch.CreateClient(cfg) + if err != nil { + klog.Errorf("could not get porch client: %s", err.Error()) + return nil, err + } + return porchClient, nil +} + +func writeErr(errMsg string, w *http.ResponseWriter) { + klog.Errorf(errMsg) + (*w).WriteHeader(500) + if _, err := (*w).Write([]byte(errMsg)); err != nil { + klog.Errorf("could not write error message: %v", err) + } +} diff --git a/porch/pkg/apiserver/webhooks_test.go b/porch/pkg/apiserver/webhooks_test.go new file mode 100644 index 0000000000..2add450dfd --- /dev/null +++ b/porch/pkg/apiserver/webhooks_test.go @@ -0,0 +1,114 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package apiserver + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + admissionv1 "k8s.io/api/admission/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestCreateCerts(t *testing.T) { + dir := t.TempDir() + defer func() { + require.NoError(t, os.RemoveAll(dir)) + }() + + caCert, err := createCerts(dir) + require.NoError(t, err) + + caStr := strings.TrimSpace(string(caCert)) + require.True(t, strings.HasPrefix(caStr, "-----BEGIN CERTIFICATE-----\n")) + require.True(t, strings.HasSuffix(caStr, "\n-----END CERTIFICATE-----")) + + crt, err := os.ReadFile(filepath.Join(dir, "tls.crt")) + require.NoError(t, err) + + key, err := os.ReadFile(filepath.Join(dir, "tls.key")) + require.NoError(t, err) + + crtStr := strings.TrimSpace(string(crt)) + require.True(t, strings.HasPrefix(crtStr, "-----BEGIN CERTIFICATE-----\n")) + require.True(t, strings.HasSuffix(crtStr, "\n-----END CERTIFICATE-----")) + + keyStr := strings.TrimSpace(string(key)) + require.True(t, strings.HasPrefix(keyStr, "-----BEGIN RSA PRIVATE KEY-----\n")) + require.True(t, strings.HasSuffix(keyStr, "\n-----END RSA PRIVATE KEY-----")) +} + +func TestValidateDeletion(t *testing.T) { + t.Run("invalid content-type", func(t *testing.T) { + request, err := http.NewRequest(http.MethodPost, serverEndpoint, nil) + require.NoError(t, err) + request.Header.Set("Content-Type", "foo") + response := httptest.NewRecorder() + + validateDeletion(response, request) + require.Equal(t, + "error getting admission review from request: expected Content-Type 'application/json'", + response.Body.String()) + }) + t.Run("valid content-type, but no body", func(t *testing.T) { + request, err := http.NewRequest(http.MethodPost, serverEndpoint, nil) + require.NoError(t, err) + request.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + + validateDeletion(response, request) + require.Equal(t, + "error getting admission review from request: admission review request is empty", + response.Body.String()) + }) + t.Run("wrong GVK in request", func(t *testing.T) { + request, err := http.NewRequest(http.MethodPost, serverEndpoint, nil) + require.NoError(t, err) + + request.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + + admissionReviewRequest := admissionv1.AdmissionReview{ + TypeMeta: v1.TypeMeta{ + Kind: "AdmissionReview", + APIVersion: "admission.k8s.io/v1", + }, + Request: &admissionv1.AdmissionRequest{ + Resource: v1.GroupVersionResource{ + Group: "porch.kpt.dev", + Version: "v1alpha1", + Resource: "not-a-package-revision", + }, + }, + } + + body, err := json.Marshal(admissionReviewRequest) + require.NoError(t, err) + + request.Body = io.NopCloser(bytes.NewReader(body)) + validateDeletion(response, request) + require.Equal(t, + "did not receive PackageRevision, got not-a-package-revision", + response.Body.String()) + }) +} diff --git a/porch/pkg/cache/repository.go b/porch/pkg/cache/repository.go index d4e6f7f8d0..1ca22401cd 100644 --- a/porch/pkg/cache/repository.go +++ b/porch/pkg/cache/repository.go @@ -22,6 +22,7 @@ import ( "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1" configapi "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1" + "github.com/GoogleContainerTools/kpt/porch/pkg/git" "github.com/GoogleContainerTools/kpt/porch/pkg/meta" "github.com/GoogleContainerTools/kpt/porch/pkg/repository" "go.opentelemetry.io/otel" @@ -147,6 +148,14 @@ func (r *cachedRepository) getCachedPackages(ctx context.Context, forceRefresh b if forceRefresh { packages = nil packageRevisions = nil + + if gitRepo, isGitRepo := r.repo.(git.GitRepository); isGitRepo { + // TODO: Figure out a way to do this without the cache layer + // needing to know what type of repo we are working with. + if err := gitRepo.UpdateDeletionProposedCache(); err != nil { + return nil, nil, err + } + } } if packages == nil { @@ -225,7 +234,7 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag k := updated.Key() // previous := r.cachedPackageRevisions[k] - if updated.Lifecycle() == v1alpha1.PackageRevisionLifecyclePublished { + if v1alpha1.LifecycleIsPublished(updated.Lifecycle()) { oldKey := repository.PackageRevisionKey{ Repository: k.Repository, Package: k.Package, diff --git a/porch/pkg/cache/util.go b/porch/pkg/cache/util.go index c086502868..644328ab43 100644 --- a/porch/pkg/cache/util.go +++ b/porch/pkg/cache/util.go @@ -35,7 +35,7 @@ func identifyLatestRevisions(result map[repository.PackageRevisionKey]*cachedPac // Check if the current package revision is more recent than the one seen so far. // Only consider Published packages - if current.Lifecycle() != v1alpha1.PackageRevisionLifecyclePublished { + if !v1alpha1.LifecycleIsPublished(current.Lifecycle()) { continue } diff --git a/porch/pkg/engine/engine.go b/porch/pkg/engine/engine.go index b9260ec132..951d00b380 100644 --- a/porch/pkg/engine/engine.go +++ b/porch/pkg/engine/engine.go @@ -118,6 +118,7 @@ func (p *PackageRevision) GetPackageRevision(ctx context.Context) (*api.PackageR repoPkgRev.Finalizers = p.packageRevisionMeta.Finalizers repoPkgRev.OwnerReferences = p.packageRevisionMeta.OwnerReferences repoPkgRev.DeletionTimestamp = p.packageRevisionMeta.DeletionTimestamp + return repoPkgRev, nil } @@ -278,7 +279,7 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * obj.Spec.Lifecycle = api.PackageRevisionLifecycleDraft case api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed: // These values are ok - case api.PackageRevisionLifecyclePublished: + case api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed: // TODO: generate errors that can be translated to correct HTTP responses return nil, fmt.Errorf("cannot create a package revision with lifecycle value 'Final'") default: @@ -564,8 +565,14 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * return nil, fmt.Errorf("invalid original lifecycle value: %q", lifecycle) case api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed: // Draft or proposed can be updated. - case api.PackageRevisionLifecyclePublished: - // Only metadata (currently labels and annotations) can be updated for published packages. + case api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed: + // Only metadata (currently labels and annotations) and lifecycle can be updated for published packages. + if oldObj.Spec.Lifecycle != newObj.Spec.Lifecycle { + if err := oldPackage.repoPackageRevision.UpdateLifecycle(ctx, newObj.Spec.Lifecycle); err != nil { + return nil, err + } + } + pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) if err != nil { return nil, err @@ -577,7 +584,7 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * switch lifecycle := newObj.Spec.Lifecycle; lifecycle { default: return nil, fmt.Errorf("invalid desired lifecycle value: %q", lifecycle) - case api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished: + case api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed: // These values are ok } @@ -955,8 +962,8 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj default: return nil, nil, fmt.Errorf("invalid original lifecycle value: %q", lifecycle) case api.PackageRevisionLifecycleDraft: - // Only draf can be updated. - case api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished: + // Only drafts can be updated. + case api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed: // TODO: generate errors that can be translated to correct HTTP responses return nil, nil, fmt.Errorf("cannot update a package revision with lifecycle value %q; package must be Draft", lifecycle) } diff --git a/porch/pkg/engine/fake/packagerevision.go b/porch/pkg/engine/fake/packagerevision.go index eddfafd8f0..3f2b7aec9c 100644 --- a/porch/pkg/engine/fake/packagerevision.go +++ b/porch/pkg/engine/fake/packagerevision.go @@ -74,3 +74,7 @@ func (f *PackageRevision) GetUpstreamLock(context.Context) (kptfile.Upstream, kp func (f *PackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, error) { return *f.Kptfile.Upstream, *f.Kptfile.UpstreamLock, nil } + +func (f *PackageRevision) UpdateLifecycle(context.Context, v1alpha1.PackageRevisionLifecycle) error { + return nil +} diff --git a/porch/pkg/git/draft.go b/porch/pkg/git/draft.go index 3e45c1d2b9..a3c81fd56a 100644 --- a/porch/pkg/git/draft.go +++ b/porch/pkg/git/draft.go @@ -130,7 +130,7 @@ func (r *gitRepository) closeDraft(ctx context.Context, d *gitPackageDraft) (*gi var newRef *plumbing.Reference switch d.lifecycle { - case v1alpha1.PackageRevisionLifecyclePublished: + case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed: // Finalize the package revision. Assign it a revision number of latest + 1. revisions, err := r.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ Package: d.path, @@ -187,7 +187,7 @@ func (r *gitRepository) closeDraft(ctx context.Context, d *gitPackageDraft) (*gi refSpecs.AddRefToDelete(base) } - // Update package referemce (commit and tree hash stay the same) + // Update package reference (commit and tree hash stay the same) newRef = plumbing.NewHashReference(draftBranch.RefInLocal(), d.commit) default: diff --git a/porch/pkg/git/git.go b/porch/pkg/git/git.go index d7f45d7457..bf0d4b7119 100644 --- a/porch/pkg/git/git.go +++ b/porch/pkg/git/git.go @@ -51,6 +51,7 @@ const ( type GitRepository interface { repository.Repository GetPackageRevision(ctx context.Context, ref, path string) (repository.PackageRevision, kptfilev1.GitLock, error) + UpdateDeletionProposedCache() error } //go:generate go run golang.org/x/tools/cmd/stringer -type=MainBranchStrategy -linecomment @@ -163,6 +164,11 @@ type gitRepository struct { // a git repository. credential repository.Credential mutex sync.Mutex + + // deletionProposedCache contains the deletionProposed branches that + // exist in the repo so that we can easily check them without iterating + // through all the refs each time + deletionProposedCache map[BranchName]bool } var _ GitRepository = &gitRepository{} @@ -382,6 +388,24 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor // Delete the tag refSpecs.AddRefToDelete(ref) + // If this revision was proposed for deletion, we need to delete the associated branch. + refSpecsForDeletionProposed := newPushRefSpecBuilder() + deletionProposedBranch := createDeletionProposedName(oldGit.path, oldGit.revision) + refSpecsForDeletionProposed.AddRefToDelete(plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), oldGit.commit)) + if err := r.pushAndCleanup(ctx, refSpecsForDeletionProposed); err != nil { + if strings.HasPrefix(err.Error(), + fmt.Sprintf("remote ref %s%s required to be", branchPrefixInRemoteRepo, deletionProposedBranch)) && + strings.HasSuffix(err.Error(), "but is absent") { + + // the deletionProposed branch might not have existed, so we ignore this error + klog.Warningf("branch %s does not exist", deletionProposedBranch) + + } else { + klog.Errorf("unexpected error while removing deletionProposed branch: %v", err) + return err + } + } + case isDraftBranchNameInLocal(rn), isProposedBranchNameInLocal(rn): // PackageRevision is proposed or draft; delete the branch directly. refSpecs.AddRefToDelete(ref) @@ -595,6 +619,37 @@ func (r *gitRepository) loadDraft(ctx context.Context, ref *plumbing.Reference) return packageRevision, nil } +func (r *gitRepository) UpdateDeletionProposedCache() error { + r.deletionProposedCache = make(map[BranchName]bool) + + err := r.fetchRemoteRepository(context.Background()) + if err != nil { + return err + } + refs, err := r.repo.References() + if err != nil { + return err + } + + for { + ref, err := refs.Next() + if err == io.EOF { + break + } + if err != nil { + klog.Errorf("error getting next ref: %v", err) + break + } + + branch, isDeletionProposedBranch := getdeletionProposedBranchNameInLocal(ref.Name()) + if isDeletionProposedBranch { + r.deletionProposedCache[deletionProposedPrefix+branch] = true + } + } + + return nil +} + func parseDraftName(draft *plumbing.Reference) (name string, workspaceName v1alpha1.WorkspaceName, err error) { refName := draft.Name() var suffix string diff --git a/porch/pkg/git/package.go b/porch/pkg/git/package.go index e5ed0c7f5e..5dede84a35 100644 --- a/porch/pkg/git/package.go +++ b/porch/pkg/git/package.go @@ -18,6 +18,7 @@ import ( "context" "crypto/sha1" "encoding/hex" + "errors" "fmt" "strings" "time" @@ -26,9 +27,11 @@ import ( kptfile "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1" "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1" "github.com/GoogleContainerTools/kpt/porch/pkg/repository" + "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" ) type gitPackageRevision struct { @@ -124,7 +127,7 @@ func (p *gitPackageRevision) GetPackageRevision(ctx context.Context) (*v1alpha1. Conditions: repository.ToApiConditions(kf), } - if p.Lifecycle() == v1alpha1.PackageRevisionLifecyclePublished { + if v1alpha1.LifecycleIsPublished(p.Lifecycle()) { if !p.updated.IsZero() { status.PublishedAt = metav1.Time{Time: p.updated} } @@ -263,14 +266,68 @@ func (p *gitPackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, func (p *gitPackageRevision) Lifecycle() v1alpha1.PackageRevisionLifecycle { switch ref := p.ref; { case ref == nil: - return v1alpha1.PackageRevisionLifecyclePublished + return p.checkPublishedLifecycle() case isDraftBranchNameInLocal(ref.Name()): return v1alpha1.PackageRevisionLifecycleDraft case isProposedBranchNameInLocal(ref.Name()): return v1alpha1.PackageRevisionLifecycleProposed default: - return v1alpha1.PackageRevisionLifecyclePublished + return p.checkPublishedLifecycle() } + +} + +func (p *gitPackageRevision) checkPublishedLifecycle() v1alpha1.PackageRevisionLifecycle { + if p.repo.deletionProposedCache == nil { + if err := p.repo.UpdateDeletionProposedCache(); err != nil { + klog.Errorf("failed to update deletionProposed cache: %v", err) + return v1alpha1.PackageRevisionLifecyclePublished + } + } + + branchName := createDeletionProposedName(p.path, p.revision) + if _, found := p.repo.deletionProposedCache[branchName]; found { + return v1alpha1.PackageRevisionLifecycleDeletionProposed + } + + return v1alpha1.PackageRevisionLifecyclePublished +} + +func (p *gitPackageRevision) UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error { + old := p.Lifecycle() + if !v1alpha1.LifecycleIsPublished(old) { + return fmt.Errorf("cannot update lifecycle for draft package revision") + } + refSpecs := newPushRefSpecBuilder() + deletionProposedBranch := createDeletionProposedName(p.path, p.revision) + + if old == v1alpha1.PackageRevisionLifecyclePublished { + if new != v1alpha1.PackageRevisionLifecycleDeletionProposed { + return fmt.Errorf("invalid new lifecycle value: %q", new) + } + + // Push the package revision into a deletionProposed branch. + p.repo.deletionProposedCache[deletionProposedBranch] = true + refSpecs.AddRefToPush(p.commit, deletionProposedBranch.RefInLocal()) + } + if old == v1alpha1.PackageRevisionLifecycleDeletionProposed { + if new != v1alpha1.PackageRevisionLifecyclePublished { + return fmt.Errorf("invalid new lifecycle value: %q", new) + } + + // Delete the deletionProposed branch + delete(p.repo.deletionProposedCache, deletionProposedBranch) + ref := plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), p.commit) + refSpecs.AddRefToDelete(ref) + } + + if err := p.repo.pushAndCleanup(ctx, refSpecs); err != nil { + if !errors.Is(err, git.NoErrAlreadyUpToDate) { + return err + } + } + + return nil } // TODO: Define a type `gitPackage` to implement the Repository.Package interface diff --git a/porch/pkg/git/ref.go b/porch/pkg/git/ref.go index f6377a5342..9fc19c5823 100644 --- a/porch/pkg/git/ref.go +++ b/porch/pkg/git/ref.go @@ -34,12 +34,17 @@ const ( branchRefSpec config.RefSpec = config.RefSpec("+" + branchPrefixInRemoteRepo + "*:" + branchPrefixInLocalRepo + "*") tagRefSpec config.RefSpec = config.RefSpec("+" + tagsPrefixInRemoteRepo + "*:" + tagsPrefixInLocalRepo + "*") - draftsPrefix = "drafts/" - draftsPrefixInLocalRepo = branchPrefixInLocalRepo + draftsPrefix - draftsPrefixInRemoteRepo = branchPrefixInRemoteRepo + draftsPrefix + draftsPrefix = "drafts/" + draftsPrefixInLocalRepo = branchPrefixInLocalRepo + draftsPrefix + draftsPrefixInRemoteRepo = branchPrefixInRemoteRepo + draftsPrefix + proposedPrefix = "proposed/" proposedPrefixInLocalRepo = branchPrefixInLocalRepo + proposedPrefix proposedPrefixInRemoteRepo = branchPrefixInRemoteRepo + proposedPrefix + + deletionProposedPrefix = "deletionProposed/" + deletionProposedPrefixInLocalRepo = branchPrefixInLocalRepo + deletionProposedPrefix + deletionProposedPrefixInRemoteRepo = branchPrefixInRemoteRepo + deletionProposedPrefix ) var ( @@ -94,6 +99,15 @@ func getDraftBranchNameInLocal(n plumbing.ReferenceName) (BranchName, bool) { return BranchName(b), ok } +func isDeletionProposedBranchNameInLocal(n plumbing.ReferenceName) bool { + return strings.HasPrefix(n.String(), deletionProposedPrefixInLocalRepo) +} + +func getdeletionProposedBranchNameInLocal(n plumbing.ReferenceName) (BranchName, bool) { + b, ok := trimOptionalPrefix(n.String(), deletionProposedPrefixInLocalRepo) + return BranchName(b), ok +} + func isBranchInLocalRepo(n plumbing.ReferenceName) bool { return strings.HasPrefix(n.String(), branchPrefixInLocalRepo) } @@ -118,6 +132,10 @@ func createProposedName(pkg string, wn v1alpha1.WorkspaceName) BranchName { return BranchName(proposedPrefix + pkg + "/" + string(wn)) } +func createDeletionProposedName(pkg string, revision string) BranchName { + return BranchName(deletionProposedPrefix + pkg + "/" + revision) +} + func trimOptionalPrefix(s, prefix string) (string, bool) { if strings.HasPrefix(s, prefix) { return strings.TrimPrefix(s, prefix), true diff --git a/porch/pkg/oci/loader.go b/porch/pkg/oci/loader.go index 6c3bcde9e8..a7095c7ad4 100644 --- a/porch/pkg/oci/loader.go +++ b/porch/pkg/oci/loader.go @@ -66,6 +66,8 @@ func (r *ociRepository) getLifecycle(ctx context.Context, imageRef oci.ImageDige return v1alpha1.PackageRevisionLifecycleProposed, nil case string(v1alpha1.PackageRevisionLifecyclePublished): return v1alpha1.PackageRevisionLifecyclePublished, nil + case string(v1alpha1.PackageRevisionLifecycleDeletionProposed): + return v1alpha1.PackageRevisionLifecycleDeletionProposed, nil default: return "", fmt.Errorf("unknown package revision lifecycle %q", lifecycleValue) } diff --git a/porch/pkg/oci/mutate.go b/porch/pkg/oci/mutate.go index 6588efbb83..2bf9735fca 100644 --- a/porch/pkg/oci/mutate.go +++ b/porch/pkg/oci/mutate.go @@ -225,7 +225,7 @@ func (p *ociPackageDraft) Close(ctx context.Context) (repository.PackageRevision } addendum.Annotations[annotationKeyLifecycle] = string(p.lifecycle) - if p.lifecycle == v1alpha1.PackageRevisionLifecyclePublished { + if v1alpha1.LifecycleIsPublished(p.lifecycle) { r := p.parent // Finalize the package revision. Assign it a revision number of latest + 1. revisions, err := r.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ diff --git a/porch/pkg/oci/oci.go b/porch/pkg/oci/oci.go index 2a05b33591..2729ad0f1e 100644 --- a/porch/pkg/oci/oci.go +++ b/porch/pkg/oci/oci.go @@ -467,3 +467,28 @@ func (p *ociPackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, func (p *ociPackageRevision) Lifecycle() v1alpha1.PackageRevisionLifecycle { return p.lifecycle } + +// UpdateLifecycle should update the package revision lifecycle from DeletionProposed to Published or vice versa. +// This function is currently only partially implemented; it still needs to store whether the package has been +// proposed for deletion somewhere in OCI, probably as another OCI image with a "deletionProposed" tag. +func (p *ociPackageRevision) UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error { + old := p.Lifecycle() + + if old == v1alpha1.PackageRevisionLifecyclePublished { + if new != v1alpha1.PackageRevisionLifecycleDeletionProposed { + return fmt.Errorf("invalid new lifecycle value: %q", new) + } + + // TODO: Create a "deletionProposed" OCI image tag. + p.lifecycle = v1alpha1.PackageRevisionLifecycleDeletionProposed + } + if old == v1alpha1.PackageRevisionLifecycleDeletionProposed { + if new != v1alpha1.PackageRevisionLifecyclePublished { + return fmt.Errorf("invalid new lifecycle value: %q", new) + } + + // TODO: Delete the "deletionProposed" OCI image tag. + p.lifecycle = v1alpha1.PackageRevisionLifecyclePublished + } + return nil +} diff --git a/porch/pkg/registry/porch/packagerevision_approval_test.go b/porch/pkg/registry/porch/packagerevision_approval_test.go index 1968d7e48b..8c7fa58f02 100644 --- a/porch/pkg/registry/porch/packagerevision_approval_test.go +++ b/porch/pkg/registry/porch/packagerevision_approval_test.go @@ -33,27 +33,32 @@ func TestApprovalUpdateStrategy(t *testing.T) { { old: "", valid: []api.PackageRevisionLifecycle{}, - invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished}, + invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed}, }, { old: "Wrong", valid: []api.PackageRevisionLifecycle{}, - invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished}, + invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed}, }, { old: api.PackageRevisionLifecycleDraft, valid: []api.PackageRevisionLifecycle{}, - invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished}, + invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed}, }, { old: api.PackageRevisionLifecyclePublished, - valid: []api.PackageRevisionLifecycle{}, + valid: []api.PackageRevisionLifecycle{api.PackageRevisionLifecycleDeletionProposed}, invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished}, }, + { + old: api.PackageRevisionLifecycleDeletionProposed, + valid: []api.PackageRevisionLifecycle{api.PackageRevisionLifecyclePublished}, + invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecycleDeletionProposed}, + }, { old: api.PackageRevisionLifecycleProposed, valid: []api.PackageRevisionLifecycle{api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecyclePublished}, - invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleProposed}, + invalid: []api.PackageRevisionLifecycle{"", "Wrong", api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecycleDeletionProposed}, }, } { for _, new := range tc.valid { diff --git a/porch/pkg/registry/porch/packagerevisions_approval.go b/porch/pkg/registry/porch/packagerevisions_approval.go index af5b00fc39..0a2e8d6771 100644 --- a/porch/pkg/registry/porch/packagerevisions_approval.go +++ b/porch/pkg/registry/porch/packagerevisions_approval.go @@ -72,7 +72,24 @@ func (s packageRevisionApprovalStrategy) ValidateUpdate(ctx context.Context, obj oldRevision := old.(*api.PackageRevision) newRevision := obj.(*api.PackageRevision) - if lifecycle := oldRevision.Spec.Lifecycle; lifecycle != api.PackageRevisionLifecycleProposed { + switch lifecycle := oldRevision.Spec.Lifecycle; lifecycle { + + case api.PackageRevisionLifecyclePublished: + if newRevision.Spec.Lifecycle != api.PackageRevisionLifecycleDeletionProposed { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, + fmt.Sprintf("package with %s lifecycle value can only be updated to 'ProposeDeletion'", lifecycle))) + } + + case api.PackageRevisionLifecycleDeletionProposed: + if newRevision.Spec.Lifecycle != api.PackageRevisionLifecyclePublished { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, + fmt.Sprintf("package with %s lifecycle value can only be updated to 'Published'", lifecycle))) + } + + case api.PackageRevisionLifecycleProposed: + // valid + + default: allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, fmt.Sprintf("cannot approve package with %s lifecycle value; only Proposed packages can be approved", lifecycle))) } @@ -82,6 +99,12 @@ func (s packageRevisionApprovalStrategy) ValidateUpdate(ctx context.Context, obj case api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecyclePublished: // valid + case api.PackageRevisionLifecycleDeletionProposed: + if oldRevision.Spec.Lifecycle != api.PackageRevisionLifecyclePublished { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, + fmt.Sprintf("cannot update lifecycle %s; only Published packages require approval for deletion", lifecycle))) + } + default: allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, fmt.Sprintf("value for approval can be only one of %s", diff --git a/porch/pkg/registry/porch/strategy.go b/porch/pkg/registry/porch/strategy.go index d2dc8367a7..7b3801e98c 100644 --- a/porch/pkg/registry/porch/strategy.go +++ b/porch/pkg/registry/porch/strategy.go @@ -41,7 +41,7 @@ func (s packageRevisionStrategy) ValidateUpdate(ctx context.Context, obj, old ru // Verify that the new lifecycle value is valid. switch lifecycle := newRevision.Spec.Lifecycle; lifecycle { - case "", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished: + case "", api.PackageRevisionLifecycleDraft, api.PackageRevisionLifecycleProposed, api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed: // valid default: allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, fmt.Sprintf("value can be only updated to %s", @@ -49,6 +49,7 @@ func (s packageRevisionStrategy) ValidateUpdate(ctx context.Context, obj, old ru string(api.PackageRevisionLifecycleDraft), string(api.PackageRevisionLifecycleProposed), string(api.PackageRevisionLifecyclePublished), + string(api.PackageRevisionLifecycleDeletionProposed), }, ",")), )) } @@ -67,9 +68,16 @@ func (s packageRevisionStrategy) ValidateUpdate(ctx context.Context, obj, old ru }, ",")), )) } - case api.PackageRevisionLifecyclePublished: + case api.PackageRevisionLifecyclePublished, api.PackageRevisionLifecycleDeletionProposed: // We don't allow any updates to the spec for packagerevision that have been published. That includes updates of the lifecycle. But - // we allow updates to metadata and status. + // we allow updates to metadata and status. The only exception is that the lifecycle + // can change between Published and DeletionProposed and vice versa. + newLifecycle := newRevision.Spec.Lifecycle + if api.LifecycleIsPublished(newLifecycle) { + // copy the lifecycle value over before calling reflect.DeepEqual to allow comparison + // of all other fields without error + newRevision.Spec.Lifecycle = oldRevision.Spec.Lifecycle + } if !equality.Semantic.DeepEqual(oldRevision.Spec, newRevision.Spec) { allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), newRevision.Spec, fmt.Sprintf("spec can only update package with lifecycle value one of %s", strings.Join([]string{ @@ -78,12 +86,14 @@ func (s packageRevisionStrategy) ValidateUpdate(ctx context.Context, obj, old ru }, ",")), )) } + newRevision.Spec.Lifecycle = newLifecycle default: allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "lifecycle"), lifecycle, fmt.Sprintf("can only update package with lifecycle value one of %s", strings.Join([]string{ string(api.PackageRevisionLifecycleDraft), string(api.PackageRevisionLifecycleProposed), string(api.PackageRevisionLifecyclePublished), + string(api.PackageRevisionLifecycleDeletionProposed), }, ",")), )) } diff --git a/porch/pkg/repository/repository.go b/porch/pkg/repository/repository.go index b476b07e74..0748b135f8 100644 --- a/porch/pkg/repository/repository.go +++ b/porch/pkg/repository/repository.go @@ -70,6 +70,11 @@ type PackageRevision interface { // Lifecycle returns the current lifecycle state of the package. Lifecycle() v1alpha1.PackageRevisionLifecycle + // UpdateLifecycle updates the desired lifecycle of the package. This can only + // be used for Published package revisions to go from Published to DeletionProposed + // or vice versa. Draft revisions should use PackageDraft.UpdateLifecycle. + UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error + // GetPackageRevision returns the PackageRevision ("DRY") API representation of this package-revision GetPackageRevision(context.Context) (*v1alpha1.PackageRevision, error) diff --git a/porch/pkg/repository/util.go b/porch/pkg/repository/util.go index dbd706605e..795016a1cd 100644 --- a/porch/pkg/repository/util.go +++ b/porch/pkg/repository/util.go @@ -66,21 +66,17 @@ func toApiConditionStatus(s kptfile.ConditionStatus) api.ConditionStatus { } } -const revisionRegex = "^v[0-9]+$" - func NextRevisionNumber(revs []PackageRevision) (string, error) { // Computes the next revision number as the latest revision number + 1. // This function only understands strict versioning format, e.g. v1, v2, etc. It will // ignore all revision numbers it finds that do not adhere to this format. // If there are no published revisions (in the recognized format), the revision // number returned here will be "v1". - latestRev := "v0" for _, current := range revs { - // Check if the current package revision is more recent than the one seen so far. // Only consider Published packages - if current.Lifecycle() != v1alpha1.PackageRevisionLifecyclePublished { + if !v1alpha1.LifecycleIsPublished(current.Lifecycle()) { continue } diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go index 67979f4c09..ff48a45709 100644 --- a/porch/test/e2e/e2e_test.go +++ b/porch/test/e2e/e2e_test.go @@ -811,7 +811,6 @@ func (t *PorchSuite) TestDeleteFinal(ctx context.Context) { const ( repository = "delete-final" packageName = "test-delete-final" - revision = "v1" workspace = "workspace" ) @@ -834,7 +833,75 @@ func (t *PorchSuite) TestDeleteFinal(ctx context.Context) { t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg) - // Delete the package + // Try to delete the package. This should fail because it hasn't been proposed for deletion. + t.DeleteL(ctx, &porchapi.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + Name: created.Name, + }, + }) + t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg) + + // Propose deletion and then delete the package + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed + t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) + + t.DeleteE(ctx, &porchapi.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + Name: created.Name, + }, + }) + + t.mustNotExist(ctx, &pkg) +} + +func (t *PorchSuite) TestProposeDeleteAndUndo(ctx context.Context) { + const ( + repository = "test-propose-delete-and-undo" + packageName = "test-propose-delete-and-undo" + workspace = "workspace" + ) + + // Register the repository + t.registerMainGitRepositoryF(ctx, repository) + + // Create a draft package + created := t.createPackageDraftF(ctx, repository, packageName, workspace) + + // Check the package exists + var pkg porchapi.PackageRevision + t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg) + + // Propose the package revision to be finalized + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(ctx, &pkg) + + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) + t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg) + + // Propose deletion + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed + t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) + + // Undo proposal of deletion + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) + + // Try to delete the package. This should fail because the lifecycle should be changed back to Published. + t.DeleteL(ctx, &porchapi.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + Name: created.Name, + }, + }) + t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg) + + // Propose deletion and then delete the package + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed + t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) + t.DeleteE(ctx, &porchapi.PackageRevision{ ObjectMeta: metav1.ObjectMeta{ Namespace: t.namespace, @@ -867,16 +934,15 @@ func (t *PorchSuite) TestDeleteAndRecreate(ctx context.Context) { pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed t.UpdateF(ctx, &pkg) - // Assign a revision number - pkg.Spec.Revision = "v1" - t.UpdateF(ctx, &pkg) - pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg) - // Delete the package + // Propose deletion and then delete the package + pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed + t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{}) + t.DeleteE(ctx, &porchapi.PackageRevision{ ObjectMeta: metav1.ObjectMeta{ Namespace: t.namespace,