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.

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

Secret API resource #4514

Merged
merged 1 commit into from
Feb 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/api/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ func init() {
&ResourceQuotaUsage{},
&Namespace{},
&NamespaceList{},
&Secret{},
&SecretList{},
)
// Legacy names are supported
Scheme.AddKnownTypeWithName("", "Minion", &Node{})
Expand Down Expand Up @@ -85,3 +87,5 @@ func (*ResourceQuotaList) IsAnAPIObject() {}
func (*ResourceQuotaUsage) IsAnAPIObject() {}
func (*Namespace) IsAnAPIObject() {}
func (*NamespaceList) IsAnAPIObject() {}
func (*Secret) IsAnAPIObject() {}
func (*SecretList) IsAnAPIObject() {}
6 changes: 6 additions & 0 deletions pkg/api/testing/fuzzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,12 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer {
c.Fuzz(&e.Count)
}
},
func(s *api.Secret, c fuzz.Continue) {
c.Fuzz(&s.TypeMeta)
c.Fuzz(&s.ObjectMeta)

s.Type = api.SecretTypeOpaque
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor thing, but it would really be nice to have a comment here explaining why this is sufficient. It took me a while to think about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't think it IS sufficient. I'll accumulate fixups in a PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thockin Agree, this is a gap. It needs to fuzz the data map too.

},
)
return f
}
32 changes: 32 additions & 0 deletions pkg/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ type VolumeSource struct {
GCEPersistentDisk *GCEPersistentDisk `json:"persistentDisk"`
// GitRepo represents a git repository at a particular revision.
GitRepo *GitRepo `json:"gitRepo"`
// Secret represents a secret that should populate this volume.
Secret *SecretSource `json:"secret"`
}

// HostPath represents bare host directory volume.
Expand Down Expand Up @@ -228,6 +230,12 @@ type GitRepo struct {
// TODO: Consider credentials here.
}

// Adapts a Secret into a VolumeSource
type SecretSource struct {
// Reference to a Secret
Target ObjectReference `json:"target"`
}

// Port represents a network port in a single container
type Port struct {
// Optional: If specified, this must be a DNS_LABEL. Each named port
Expand Down Expand Up @@ -1309,3 +1317,27 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota `json:"items"`
}

// Secret holds secret data of a certain type
type Secret struct {
TypeMeta `json:",inline"`
ObjectMeta `json:"metadata,omitempty"`

Data map[string][]byte `json:"data,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a bit more comments to describe the string format?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a subsequent comment here about DNS_SUBDOMAIN. Is that flexible enough for producing arbitrary secrets' filenames?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value of the map must be base64 encoded, why is this []byte rather than string? Alternately, why the base64 rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thockin DNS_SUBDOMAIN isn't flexible enough. I expect to make a new format for filenames and/or adapt names in SecretSource

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, commenting late.

First of all: All fields in v1beta1 and v1beta2 need description tags. This will also soon be true for v1beta3. The Travis check was broken, but was fixed today. Please ensure future fields have descriptions.

As discussed in the recent PR to update api-conventions.md, it is acceptable for an object to not distinguish Spec and Status if we're confident it will only support one or the other, as in this case.

As discussed in #1627 and #1553, we will want a very similar object for dynamic configuration distribution. But, similar to the distinction between labels and annotations, I agree that it's useful to separate the two use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see. The comment led me astray. I though people you were taking strings and base64ing them and then storing that in []byte. But you're storing arbitrary secret data in []byte, and JSON serializes it to base64. I'm going to update the comments in my PR, see if it is any better when I am done. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @bgrant0607 This uses a map - more fodder for maps being the OBVIOUS api for things like environment variables.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thockin @bgrant0607, I was thinking about the Data field this weekend -
I think there's a case to be made for using a list of a type like:

type SecretCell struct {
    Name  string
    Value string
    Binary bool

I don't think you should have to base64 encode everything - it makes for
kind of a crummy experience IMO, especially when the serialized form is a
string anyway. The binary bit would indicate whether the string is just a
normal string or base64 encoded binary data.

On Mon, Feb 23, 2015 at 1:54 PM, Tim Hockin notifications@github.com
wrote:

In pkg/api/types.go
#4514 (comment)
:

@@ -1309,3 +1317,27 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota json:"items"
}
+
+// Secret holds secret data of a certain type
+type Secret struct {

  • TypeMeta json:",inline"
  • ObjectMeta json:"metadata,omitempty"
  • Data map[string][]byte json:"data,omitempty"

Also, @bgrant0607 https://github.com/bgrant0607 This uses a map - more
fodder for maps being the OBVIOUS api for things like environment variables.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/4514/files#r25190058
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that the "always base64" crumminess is worse than the "sometimes base64" crumminess :)

Anyway, I'd like to get #4653 in before any significant retool, please. I just want it off the balance sheet, and it fixes many of the concerns here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thockin go for it, I was not proposing that you address that in #4653

On Mon, Feb 23, 2015 at 2:25 PM, Tim Hockin notifications@github.com
wrote:

In pkg/api/types.go
#4514 (comment)
:

@@ -1309,3 +1317,27 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota json:"items"
}
+
+// Secret holds secret data of a certain type
+type Secret struct {

  • TypeMeta json:",inline"
  • ObjectMeta json:"metadata,omitempty"
  • Data map[string][]byte json:"data,omitempty"

I'm not sure that the "always base64" crumminess is worse than the
"sometimes base64" crumminess :)

Anyway, I'd like to get #4653
#4653 in before
any significant retool, please. I just want it off the balance sheet, and
it fixes many of the concerns here.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/4514/files#r25192983
.

Type SecretType `json:"type,omitempty"`
}

type SecretType string

const (
SecretTypeOpaque SecretType = "opaque" // Default; arbitrary user-defined data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should follow Go-style caps - Opaque

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgrant0607 @thockin will collect these i to a PR soon (might not be until
Monday).
On Sat, Feb 21, 2015 at 1:33 AM Brian Grant notifications@github.com
wrote:

In pkg/api/types.go
#4514 (comment)
:

@@ -1309,3 +1317,27 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota json:"items"
}
+
+// Secret holds secret data of a certain type
+type Secret struct {

  • TypeMeta json:",inline"
  • ObjectMeta json:"metadata,omitempty"
  • Data map[string][]byte json:"data,omitempty"
  • Type SecretType json:"type,omitempty"
    +}

+type SecretType string
+
+const (

  • SecretTypeOpaque SecretType = "opaque" // Default; arbitrary user-defined data

+1


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/4514/files#r25120223
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmorie /cc me on the PR if it modifies the API that is checked in. I am working on something that uses this already so I would need to update.

Sent from my iPhone

On Feb 21, 2015, at 12:28 PM, Paul Morie notifications@github.com wrote:

In pkg/api/types.go:

@@ -1309,3 +1317,27 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota json:"items"
}
+
+// Secret holds secret data of a certain type
+type Secret struct {

  • TypeMeta json:",inline"
  • ObjectMeta json:"metadata,omitempty"
  • Data map[string][]byte json:"data,omitempty"
  • Type SecretType json:"type,omitempty"
    +}

+type SecretType string
+
+const (

  • SecretTypeOpaque SecretType = "opaque" // Default; arbitrary user-defined data
    @bgrant0607 @thockin will collect these i to a PR soon (might not be until Monday).


    Reply to this email directly or view it on GitHub.

)

type SecretList struct {
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`

Items []Secret `json:"items"`
}

const MaxSecretSize = 1 * 1024 * 1024
6 changes: 6 additions & 0 deletions pkg/api/v1beta1/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,9 @@ func init() {
if err := s.Convert(&in.HostPath, &out.HostDir, 0); err != nil {
return err
}
if err := s.Convert(&in.Secret, &out.Secret, 0); err != nil {
return err
}
return nil
},
func(in *VolumeSource, out *newer.VolumeSource, s conversion.Scope) error {
Expand All @@ -1043,6 +1046,9 @@ func init() {
if err := s.Convert(&in.HostDir, &out.HostPath, 0); err != nil {
return err
}
if err := s.Convert(&in.Secret, &out.Secret, 0); err != nil {
return err
}
return nil
},

Expand Down
5 changes: 5 additions & 0 deletions pkg/api/v1beta1/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,10 @@ func init() {
obj.TimeoutSeconds = 1
}
},
func(obj *Secret) {
if obj.Type == "" {
obj.Type = SecretTypeOpaque
}
},
)
}
10 changes: 10 additions & 0 deletions pkg/api/v1beta1/defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,13 @@ func TestSetDefaultContainer(t *testing.T) {
current.ProtocolTCP, container.Ports[0].Protocol)
}
}

func TestSetDefaultSecret(t *testing.T) {
s := &current.Secret{}
obj2 := roundTrip(t, runtime.Object(s))
s2 := obj2.(*current.Secret)

if s2.Type != current.SecretTypeOpaque {
t.Errorf("Expected secret type %v, got %v", current.SecretTypeOpaque, s2.Type)
}
}
4 changes: 4 additions & 0 deletions pkg/api/v1beta1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ func init() {
&ResourceQuotaUsage{},
&Namespace{},
&NamespaceList{},
&Secret{},
&SecretList{},
)
// Future names are supported
api.Scheme.AddKnownTypeWithName("v1beta1", "Node", &Minion{})
Expand Down Expand Up @@ -86,3 +88,5 @@ func (*ResourceQuotaList) IsAnAPIObject() {}
func (*ResourceQuotaUsage) IsAnAPIObject() {}
func (*Namespace) IsAnAPIObject() {}
func (*NamespaceList) IsAnAPIObject() {}
func (*Secret) IsAnAPIObject() {}
func (*SecretList) IsAnAPIObject() {}
27 changes: 27 additions & 0 deletions pkg/api/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ type VolumeSource struct {
GCEPersistentDisk *GCEPersistentDisk `json:"persistentDisk" description:"GCE disk resource attached to the host machine on demand"`
// GitRepo represents a git repository at a particular revision.
GitRepo *GitRepo `json:"gitRepo" description:"git repository at a particular revision"`
// Secret represents a secret to populate the volume with
Secret *SecretSource `json:"secret" description:"secret to populate volume with"`
}

// HostPath represents bare host directory volume.
Expand Down Expand Up @@ -153,6 +155,12 @@ type GitRepo struct {
Revision string `json:"revision" description:"commit hash for the specified revision"`
}

// Adapts a Secret into a VolumeSource
type SecretSource struct {
// Reference to a Secret
Target ObjectReference `json:"target"`
}

// Port represents a network port in a single container
type Port struct {
// Optional: If specified, this must be a DNS_LABEL. Each named port
Expand Down Expand Up @@ -1091,3 +1099,22 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota `json:"items"`
}

type Secret struct {
TypeMeta `json:",inline"`

Data map[string][]byte `json:"data,omitempty"`
Type SecretType `json:"type,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere else in the API, we use "Kind". For example:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/v1beta3/types.go#L506

Could we please change this to SecretKind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please put the kind above the Data field.

}

type SecretType string

const (
SecretTypeOpaque SecretType = "opaque" // Default; arbitrary user-defined data
)

type SecretList struct {
TypeMeta `json:",inline"`

Items []Secret `json:"items"`
}
6 changes: 6 additions & 0 deletions pkg/api/v1beta2/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,9 @@ func init() {
if err := s.Convert(&in.HostPath, &out.HostDir, 0); err != nil {
return err
}
if err := s.Convert(&in.Secret, &out.Secret, 0); err != nil {
return err
}
return nil
},
func(in *VolumeSource, out *newer.VolumeSource, s conversion.Scope) error {
Expand All @@ -958,6 +961,9 @@ func init() {
if err := s.Convert(&in.HostDir, &out.HostPath, 0); err != nil {
return err
}
if err := s.Convert(&in.Secret, &out.Secret, 0); err != nil {
return err
}
return nil
},

Expand Down
7 changes: 7 additions & 0 deletions pkg/api/v1beta2/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ import (

"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
"github.com/golang/glog"
)

func init() {
api.Scheme.AddDefaultingFuncs(
func(obj *Volume) {
if util.AllPtrFieldsNil(&obj.Source) {
glog.Errorf("Defaulting volume source for %v", obj)
obj.Source = VolumeSource{
EmptyDir: &EmptyDir{},
}
Expand Down Expand Up @@ -80,5 +82,10 @@ func init() {
obj.TimeoutSeconds = 1
}
},
func(obj *Secret) {
if obj.Type == "" {
obj.Type = SecretTypeOpaque
}
},
)
}
10 changes: 10 additions & 0 deletions pkg/api/v1beta2/defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,13 @@ func TestSetDefaultContainer(t *testing.T) {
current.ProtocolTCP, container.Ports[0].Protocol)
}
}

func TestSetDefaultSecret(t *testing.T) {
s := &current.Secret{}
obj2 := roundTrip(t, runtime.Object(s))
s2 := obj2.(*current.Secret)

if s2.Type != current.SecretTypeOpaque {
t.Errorf("Expected secret type %v, got %v", current.SecretTypeOpaque, s2.Type)
}
}
4 changes: 4 additions & 0 deletions pkg/api/v1beta2/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ func init() {
&ResourceQuotaUsage{},
&Namespace{},
&NamespaceList{},
&Secret{},
&SecretList{},
)
// Future names are supported
api.Scheme.AddKnownTypeWithName("v1beta2", "Node", &Minion{})
Expand Down Expand Up @@ -86,3 +88,5 @@ func (*ResourceQuotaList) IsAnAPIObject() {}
func (*ResourceQuotaUsage) IsAnAPIObject() {}
func (*Namespace) IsAnAPIObject() {}
func (*NamespaceList) IsAnAPIObject() {}
func (*Secret) IsAnAPIObject() {}
func (*SecretList) IsAnAPIObject() {}
28 changes: 28 additions & 0 deletions pkg/api/v1beta2/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ type VolumeSource struct {
GCEPersistentDisk *GCEPersistentDisk `json:"persistentDisk" description:"GCE disk resource attached to the host machine on demand"`
// GitRepo represents a git repository at a particular revision.
GitRepo *GitRepo `json:"gitRepo" description:"git repository at a particular revision"`
// Secret is a secret to populate the volume with
Secret *SecretSource `json:"secret" description:"secret to populate volume"`
}

// HostPath represents bare host directory volume.
Expand All @@ -81,6 +83,12 @@ type HostPath struct {

type EmptyDir struct{}

// Adapts a Secret into a VolumeSource
type SecretSource struct {
// Reference to a Secret
Target ObjectReference `json:"target"`
}

// Protocol defines network protocols supported for things like conatiner ports.
type Protocol string

Expand Down Expand Up @@ -1094,3 +1102,23 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota `json:"items"`
}

// Secret holds secret data of a certain type
type Secret struct {
TypeMeta `json:",inline"`

Data map[string][]byte `json:"data,omitempty"`
Type SecretType `json:"type,omitempty"`
}

type SecretType string

const (
SecretTypeOpaque SecretType = "opaque" // Default; arbitrary user-defined data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please capitalize. All constants in the API are CamelCase.

Please tell me if api-conventions.md is not clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgrant0607 I hadn't read it prior to your comment, but it's not clear. There's no treatment given to constants or casing (viz: 'CamelCase' and 'constant' don't appear in the text of the doc). I'll make an issue to clarify.

)

type SecretList struct {
TypeMeta `json:",inline"`

Items []Secret `json:"items"`
}
5 changes: 5 additions & 0 deletions pkg/api/v1beta3/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,10 @@ func init() {
obj.TimeoutSeconds = 1
}
},
func(obj *Secret) {
if obj.Type == "" {
obj.Type = SecretTypeOpaque
}
},
)
}
10 changes: 10 additions & 0 deletions pkg/api/v1beta3/defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,13 @@ func TestSetDefaultContainer(t *testing.T) {
current.ProtocolTCP, container.Ports[0].Protocol)
}
}

func TestSetDefaultSecret(t *testing.T) {
s := &current.Secret{}
obj2 := roundTrip(t, runtime.Object(s))
s2 := obj2.(*current.Secret)

if s2.Type != current.SecretTypeOpaque {
t.Errorf("Expected secret type %v, got %v", current.SecretTypeOpaque, s2.Type)
}
}
4 changes: 4 additions & 0 deletions pkg/api/v1beta3/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ func init() {
&ResourceQuotaUsage{},
&Namespace{},
&NamespaceList{},
&Secret{},
&SecretList{},
)
// Legacy names are supported
api.Scheme.AddKnownTypeWithName("v1beta3", "Minion", &Node{})
Expand Down Expand Up @@ -86,3 +88,5 @@ func (*ResourceQuotaList) IsAnAPIObject() {}
func (*ResourceQuotaUsage) IsAnAPIObject() {}
func (*Namespace) IsAnAPIObject() {}
func (*NamespaceList) IsAnAPIObject() {}
func (*Secret) IsAnAPIObject() {}
func (*SecretList) IsAnAPIObject() {}
31 changes: 31 additions & 0 deletions pkg/api/v1beta3/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ type VolumeSource struct {
GCEPersistentDisk *GCEPersistentDisk `json:"gcePersistentDisk"`
// GitRepo represents a git repository at a particular revision.
GitRepo *GitRepo `json:"gitRepo"`
// Secret represents a secret that should populate this volume.
Secret *SecretSource `json:"secret"`
}

// HostPath represents bare host directory volume.
Expand Down Expand Up @@ -246,6 +248,12 @@ type GitRepo struct {
Revision string `json:"revision"`
}

// Adapts a Secret into a VolumeSource
type SecretSource struct {
// Reference to a Secret
Target ObjectReference `json:"target"`
}

// Port represents a network port in a single container.
type Port struct {
// Optional: If specified, this must be a DNS_LABEL. Each named port
Expand Down Expand Up @@ -1234,3 +1242,26 @@ type ResourceQuotaList struct {
// Items is a list of ResourceQuota objects
Items []ResourceQuota `json:"items"`
}

// Secret holds mappings between paths and secret data
// TODO: shouldn't "Secret" be a plural?
type Secret struct {
TypeMeta `json:",inline"`
ObjectMeta `json:"metadata,omitempty"`

Data map[string][]byte `json:"data,omitempty"`
Type SecretType `json:"type,omitempty"`
}

type SecretType string

const (
SecretTypeOpaque SecretType = "opaque" // Default; arbitrary user-defined data
)

type SecretList struct {
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`

Items []Secret `json:"items"`
}