From 054c3960a599abd54f48c73e073ca80ac2ff79b1 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 1 Aug 2023 04:20:44 -0700 Subject: [PATCH] Don't set object-level public ACL in S3 FileRepository --- cmd/kops/util/factory.go | 2 - docs/operations/asset-repository.md | 4 ++ docs/releases/1.28-NOTES.md | 4 ++ pkg/acls/s3/storage.go | 89 ----------------------------- pkg/acls/s3/storage_test.go | 88 ---------------------------- 5 files changed, 8 insertions(+), 179 deletions(-) delete mode 100644 pkg/acls/s3/storage.go delete mode 100644 pkg/acls/s3/storage_test.go diff --git a/cmd/kops/util/factory.go b/cmd/kops/util/factory.go index 8388417ea6..753999f486 100644 --- a/cmd/kops/util/factory.go +++ b/cmd/kops/util/factory.go @@ -32,7 +32,6 @@ import ( "k8s.io/klog/v2" channelscmd "k8s.io/kops/channels/pkg/cmd" gceacls "k8s.io/kops/pkg/acls/gce" - s3acls "k8s.io/kops/pkg/acls/s3" kopsclient "k8s.io/kops/pkg/client/clientset_generated/clientset" "k8s.io/kops/pkg/client/simple" "k8s.io/kops/pkg/client/simple/api" @@ -60,7 +59,6 @@ type Factory struct { func NewFactory(options *FactoryOptions) *Factory { gceacls.Register() - s3acls.Register() return &Factory{ options: options, diff --git a/docs/operations/asset-repository.md b/docs/operations/asset-repository.md index abf1bba113..b0d2a829a8 100644 --- a/docs/operations/asset-repository.md +++ b/docs/operations/asset-repository.md @@ -41,6 +41,10 @@ spec: fileRepository: https://example.com/files ``` +The repository must allow nodes to perform unauthenticated reads. The repository can +be public or it can allow read access through network connectivity, such as access +through a particular AWS Endpoint. + ## Copying assets into repositories {{ kops_feature_table(kops_added_default='1.22') }} diff --git a/docs/releases/1.28-NOTES.md b/docs/releases/1.28-NOTES.md index 302d6a6b2b..3ed9a4c1cb 100644 --- a/docs/releases/1.28-NOTES.md +++ b/docs/releases/1.28-NOTES.md @@ -16,6 +16,10 @@ This is a document to gather the release notes prior to the release. # Breaking changes +## AWS + +* The `kops get assets --copy` command no longer sets object-level public-read ACLs in the destination fileRDoepository. + ## Other breaking changes * Support for Kubernetes version 1.22 has been removed. diff --git a/pkg/acls/s3/storage.go b/pkg/acls/s3/storage.go deleted file mode 100644 index feac66f7bb..0000000000 --- a/pkg/acls/s3/storage.go +++ /dev/null @@ -1,89 +0,0 @@ -/* -Copyright 2017 The Kubernetes Authors. - -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 s3 - -import ( - "context" - "fmt" - "net/url" - "strings" - - "k8s.io/klog/v2" - "k8s.io/kops/pkg/acls" - "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/values" - "k8s.io/kops/util/pkg/vfs" -) - -// s3PublicAclStrategy is the AclStrategy for objects that are written with public read only ACL. -// This strategy is used by custom file assets. -type s3PublicAclStrategy struct{} - -var _ acls.ACLStrategy = &s3PublicAclStrategy{} - -// GetACL creates a s3PublicAclStrategy object for writing public files with assets FileRepository. -// This strategy checks if the files are inside the state store, and if the files are located inside -// the state store, this returns nil and logs a message (level 8) that it will not run. -func (s *s3PublicAclStrategy) GetACL(ctx context.Context, p vfs.Path, cluster *kops.Cluster) (vfs.ACL, error) { - if cluster.Spec.Assets == nil || cluster.Spec.Assets.FileRepository == nil { - return nil, nil - } - - s3Path, ok := p.(*vfs.S3Path) - if !ok { - return nil, nil - } - - fileRepository := values.StringValue(cluster.Spec.Assets.FileRepository) - - u, err := url.Parse(fileRepository) - if err != nil { - return "", fmt.Errorf("unable to parse: %q", fileRepository) - } - - // We are checking that the file repository url is in S3 - _, err = vfs.VFSPath(fileRepository) - if err != nil { - klog.V(8).Infof("path %q is not inside of a s3 bucket", u.String()) - return nil, nil - } - - config, err := url.Parse(cluster.Spec.ConfigStore.Base) - if err != nil { - return "", fmt.Errorf("unable to parse: %q", cluster.Spec.ConfigStore.Base) - } - - // We are checking that the path defined is not the state store, if it is - // we do NOT set the state store as public read. - if strings.Contains(u.Path, config.Path) { - klog.V(8).Infof("path %q is inside of config store %q, not setting public-read acl", u.Path, config.Path) - return nil, nil - } - - if strings.TrimPrefix(u.Path, "/") == s3Path.Bucket() { - return &vfs.S3Acl{ - RequestACL: values.String("public-read"), - }, nil - } - klog.V(8).Infof("path %q is not inside the file registry %q, not setting public-read acl", u.Path, config.Path) - - return nil, nil -} - -func Register() { - acls.RegisterPlugin("k8s.io/kops/acl/s3", &s3PublicAclStrategy{}) -} diff --git a/pkg/acls/s3/storage_test.go b/pkg/acls/s3/storage_test.go deleted file mode 100644 index 737931d546..0000000000 --- a/pkg/acls/s3/storage_test.go +++ /dev/null @@ -1,88 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -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 s3 - -import ( - "context" - "testing" - - "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/values" - "k8s.io/kops/util/pkg/vfs" -) - -func Test_Strategy(t *testing.T) { - ctx := context.TODO() - - context := &vfs.VFSContext{} - path, err := context.BuildVfsPath("s3://test/foo") - if err != nil { - t.Errorf("unable to create path: %v", err) - } - - cluster := &kops.Cluster{ - Spec: kops.ClusterSpec{ - ConfigStore: kops.ConfigStoreSpec{ - Base: "s3://my_state_store/cluster", - }, - Assets: &kops.AssetsSpec{ - FileRepository: values.String("https://s3.amazonaws.com/test"), - }, - }, - } - - s := &s3PublicAclStrategy{} - acl, err := s.GetACL(ctx, path, cluster) - if err != nil { - t.Errorf("error getting ACL: %v", err) - } - - if acl == nil { - t.Errorf("public ro ACL is nil and should not be, this test is a positive test case.") - } -} - -func Test_In_StateStore(t *testing.T) { - ctx := context.TODO() - - context := &vfs.VFSContext{} - stateStore, err := context.BuildVfsPath("s3://my_state_store/cluster") - if err != nil { - t.Errorf("unable to create path: %v", err) - } - - cluster := &kops.Cluster{ - Spec: kops.ClusterSpec{ - ConfigStore: kops.ConfigStoreSpec{ - Base: "s3://my_state_store/cluster", - }, - Assets: &kops.AssetsSpec{ - FileRepository: values.String("https://s3.amazonaws.com/my_state_store/opps"), - }, - }, - } - - s := &s3PublicAclStrategy{} - acl, err := s.GetACL(ctx, stateStore, cluster) - if err != nil { - t.Errorf("error getting ACL: %v", err) - } - - if acl != nil { - t.Errorf("public ro ACL is set but path is in the state store, this test is a negative test case.") - } -}