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

feature. support tls #35

Merged
merged 4 commits into from
Mar 22, 2022
Merged

feature. support tls #35

merged 4 commits into from
Mar 22, 2022

Conversation

ktkfree
Copy link
Contributor

@ktkfree ktkfree commented Mar 18, 2022

server 레벨 TLS 인증을 위해 tls 인증 기능을 추가합니다.

관련하여 tks-common package 를 사용하도록 변경하였기 때문에, 기존 코드의 refactoring 도 진행되었습니다.

dependency PR : openinfradev/tks-common#4

 . support tls
 . refactoring by tks-common
@ktkfree ktkfree changed the title feature. feature. support tls Mar 18, 2022
@@ -59,20 +77,44 @@ func main() {
log.Info("argoPort : ", argoPort)
log.Info("revision : ", revision)
log.Info("gitAccount : ", gitAccount)
log.Info("****************** ")

if gitToken = os.Getenv("TOKEN"); gitToken == "" {
Copy link
Contributor

@robertchoi80 robertchoi80 Mar 21, 2022

Choose a reason for hiding this comment

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

이 환경변수가 자주 사용될 것으로 보이는데, TOKEN은 너무 general한 이름이니 좀더 구체적인 변수명을 사용하는게 좋겠습니다. 어떤 목적으로 사용되는 TOKEN인지 알 수 있도록이요

Copy link
Contributor

Choose a reason for hiding this comment

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

@robertchoi80 주신 의견과 더불어서 gitAccount, gitToken 변수 이름 역시 githubAccount, githubToken (혹은 ghAccount, ghToken) 과 같이 사용하는 것이 좀 더 명확해 보입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertchoi80 이건 secret 의 데이터를 사용하고 있는데요. secret 내 변수가 아래와 같이 되어 있어서 TOKEN 으로 받았습니다. 혹시 chart에서 해당값을 application 에서 다른 값으로 binding 할 수 있을가요?

data:
  TOKEN: <TOKEN>
  USERNAME: <NAME>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zugwan 변경하였습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

@robertchoi80 이건 secret 의 데이터를 사용하고 있는데요. secret 내 변수가 아래와 같이 되어 있어서 TOKEN 으로 받았습니다. 혹시 chart에서 해당값을 application 에서 다른 값으로 binding 할 수 있을가요?

data:
  TOKEN: <TOKEN>
  USERNAME: <NAME>

무슨 말씀인지 이해를 잘 못했습니다. 어느 secret과 어느 chart를 말씀하시는 건지요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아래와 같이 차트에서 환경변수로 secret을 읽어들이고 있고요.
. https://github.com/openinfradev/helm-charts/blob/main/tks-cluster-lcm/templates/deployment.yaml#L46

이 secret 내에 data field 가 TOKEN 입니다.

tks-cluster-lcm 내에서는 TOKEN 이라는 string 으로 환경변수를 가져�오고 있는데, TOKEN 변수를 변경할 수 있는 방안이 있는건지를 문의 드렸습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

그 말씀이셨군요. (만약 적절하다면) original secret의 key값을 변경하던지,, 아니면 envFrom 대신 아래와 같이 valueFrom 을 쓰면 가능하지 않을까 생각됩니다.

    env:
      - name: SECRET_USERNAME
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username

Copy link
Contributor

Choose a reason for hiding this comment

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

근데 이렇게 고치려면 차트와 lcm 소스를 다 고쳐야 할테니, 일단은 그냥 TODO comment 정도만 남겨두고 이대로 릴리즈 따고, 차후에 수정해도 될거 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아하 이런 방법이 있었군요. 넵, 릴리즈 QA시 반영하도록 하겠습니다.

@zugwan zugwan self-requested a review March 21, 2022 08:55
@ktkfree ktkfree merged commit 82e7ad5 into main Mar 22, 2022
@ktkfree ktkfree deleted the support_tls branch March 22, 2022 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants